fix: validate Slack credentials before enabling channels#4582
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Slack bot/app token probing and validation, integrates it into interactive and non-interactive onboarding and sandbox channel addition, and updates unit, integration, and e2e tests and harnesses to assert probe→persist ordering and prevent token leakage. ChangesSlack Token Validation Integration
Sequence DiagramsequenceDiagram
participant User as User/Setup Flow
participant Collector as setupSlackTokens
participant Validator as validateSlackCredentials
participant ProbeBot as validateSlackBotToken
participant ProbeApp as validateSlackAppToken
participant SlackAPI as Slack API
participant Saver as saveCredential
User->>Collector: collect bot token (prompt/env)
Collector->>Collector: collect app token (prompt/env)
Collector->>Validator: validate botToken + appToken
Validator->>ProbeBot: validate bot token (auth.test)
ProbeBot->>SlackAPI: auth.test with Bearer botToken
SlackAPI-->>ProbeBot: response
ProbeBot-->>Validator: SlackTokenValidationResult
alt Bot valid
Validator->>ProbeApp: validate app token (apps.connections.open)
ProbeApp->>SlackAPI: apps.connections.open with Bearer appToken
SlackAPI-->>ProbeApp: response
ProbeApp-->>Validator: SlackTokenValidationResult
Validator-->>Collector: { ok: true }
Collector->>Saver: saveCredential(SLACK_BOT_TOKEN, botToken)
Collector->>Saver: saveCredential(SLACK_APP_TOKEN, appToken)
Collector-->>User: credentials saved, process.env updated
else Validation fails
Validator-->>Collector: { ok: false, credential: 'bot'|'app', message }
Collector-->>User: remove slack from enabled, log redacted error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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. Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/lib/onboard.ts`:
- Around line 5591-5610: The function validateSlackSelection should be moved out
of src/lib/onboard.ts into a new or existing onboarding submodule under
src/lib/onboard/ (e.g., src/lib/onboard/validateSlackSelection.ts) and exported
from that module; in onboard.ts replace the function body with an import and
keep only the call site. Update any imports: export default or named export of
validateSlackSelection from the new file and import it in onboard.ts, and ensure
references to helper utilities used inside (getValidatedMessagingTokenByEnvKey,
MESSAGING_CHANNELS, validateSlackCredentials, formatSlackValidationFailure) are
available via imports in the new module. Also run or update any tests/refs that
import validateSlackSelection so CI stops failing the growth guardrail.
In `@src/lib/onboard/slack-validation.ts`:
- Around line 25-35: Update the TRANSIENT_SLACK_ERRORS set to match Slack’s
documented Web API error codes: keep only "ratelimited" (note spelling) and
"request_timeout" as transient errors and remove "rate_limited", "timeout",
"internal_error", "fatal_error", "service_unavailable", etc.; leave the existing
SLACK_AUTH_TEST_URL and SLACK_APPS_CONNECTIONS_OPEN_URL constants as-is (they
are correct). Use the constant names TRANSIENT_SLACK_ERRORS,
SLACK_AUTH_TEST_URL, and SLACK_APPS_CONNECTIONS_OPEN_URL to locate and modify
the set 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: 0a8610d9-5f1d-49ff-9b37-0f21e9f4c940
📒 Files selected for processing (8)
src/lib/actions/sandbox/policy-channel.tssrc/lib/onboard.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/onboard/slack-validation.test.tssrc/lib/onboard/slack-validation.tstest/channels-add-preset.test.tstest/onboard-messaging.test.ts
Selective E2E Results — ❌ Some jobs failedRun: 26717554171
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/messaging-channel-setup.test.ts (1)
171-198: 💤 Low valueOptionally assert token secrecy for parity with the rejected case.
The sibling "rejected" test (Lines 165-168) captures logs and asserts the raw tokens never appear in output. This indeterminate case skips that check. Since secrecy is a stated PR objective, adding a log capture + negative assertion would close the parity gap.
♻️ Optional: add log capture and secrecy assertions
const enabled = new Set(["slack"]); + const logs: string[] = []; + vi.spyOn(console, "log").mockImplementation((message = "") => { + logs.push(String(message)); + }); await setupSelectedMessagingChannels( ["slack"], enabled, [{ name: "slack", ...KNOWN_CHANNELS.slack }], ); expect(enabled.has("slack")).toBe(false); expect(saveCredential).not.toHaveBeenCalled(); expect(process.env.SLACK_BOT_TOKEN).toBeUndefined(); expect(process.env.SLACK_APP_TOKEN).toBeUndefined(); + const output = logs.join("\n"); + expect(output).not.toContain("xoxb-timeout-bot-token"); + expect(output).not.toContain("xapp-timeout-app-token"); });🤖 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/lib/onboard/messaging-channel-setup.test.ts` around lines 171 - 198, Add the same log-capture and negative token-secrecy assertions used in the "rejected" sibling test to the "does not save prompted Slack credentials when Slack API validation is indeterminate" test: capture console output (e.g., via the test helper used elsewhere or spy on console.log/error) around the call to setupSelectedMessagingChannels, then assert that the literal token strings ("xoxb-timeout-bot-token" and "xapp-timeout-app-token") do not appear in the captured logs; keep the existing assertions for enabled.has("slack"), saveCredential, and process.env unchanged and locate this change in the test function that calls setupSelectedMessagingChannels, prompt, validateSlackCredentials and checks saveCredential/process.env.
🤖 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.
Nitpick comments:
In `@src/lib/onboard/messaging-channel-setup.test.ts`:
- Around line 171-198: Add the same log-capture and negative token-secrecy
assertions used in the "rejected" sibling test to the "does not save prompted
Slack credentials when Slack API validation is indeterminate" test: capture
console output (e.g., via the test helper used elsewhere or spy on
console.log/error) around the call to setupSelectedMessagingChannels, then
assert that the literal token strings ("xoxb-timeout-bot-token" and
"xapp-timeout-app-token") do not appear in the captured logs; keep the existing
assertions for enabled.has("slack"), saveCredential, and process.env unchanged
and locate this change in the test function that calls
setupSelectedMessagingChannels, prompt, validateSlackCredentials and checks
saveCredential/process.env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 321e2fba-9b78-4647-8d9e-0ac0d6050040
📒 Files selected for processing (6)
src/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/slack-channel-validation.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/slack-validation.test.tssrc/lib/onboard/slack-validation.tstest/channels-add-preset.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/channels-add-preset.test.ts
- src/lib/onboard/slack-validation.ts
Selective E2E Results — ❌ Some jobs failedRun: 26717926316
|
|
🌿 Preview your docs: https://nvidia-preview-pr-4582.docs.buildwithfern.com/nemoclaw |
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)
test/channels-add-preset.test.ts (1)
25-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the Slack-validation skip flag in the default child env.
runScript()inheritsNEMOCLAW_SKIP_SLACK_AUTH_VALIDATIONfrom the parent process, so the rejected/indeterminate Slack tests can silently bypass probes and stop exercising the failure path whenever that flag is set in CI or a developer shell. Seed it to""here and let the one opt-out test override it to"1".🧪 Suggested fix
env: { ...process.env, + NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION: "", HOME: tmpDir, NEMOCLAW_NON_INTERACTIVE: "1", TELEGRAM_BOT_TOKEN: "test-telegram-token", SLACK_BOT_TOKEN: "xoxb-slack-bot-token-for-test",🤖 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/channels-add-preset.test.ts` around lines 25 - 34, The child process env object used when calling runScript() is inheriting NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION from the parent, allowing Slack validation to be skipped in tests; update the env block in test/channels-add-preset.test.ts (the env passed into runScript()/child process) to explicitly set NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION: "" so the default tests exercise the validation failure paths, and leave the individual opt-out test to set NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION = "1" as needed to override it.
🧹 Nitpick comments (1)
test/e2e/test-token-rotation.sh (1)
159-164: 💤 Low valueConsider extracting shared helper to a library script.
The
is_fake_slack_tokenhelper is duplicated across 5 E2E scripts. While self-contained E2E tests have value for portability, you could extract this totest/e2e/lib/slack-helpers.shalongside existing shared libraries likesandbox-teardown.shif you find yourself adding more Slack-related test utilities.This is optional—the current duplication is acceptable for E2E tests.
🤖 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/e2e/test-token-rotation.sh` around lines 159 - 164, Extract the duplicated is_fake_slack_token function into a shared shell library (e.g., slack-helpers.sh) and update the E2E scripts to source that library instead of redefining the function; specifically, move the existing function body (the case pattern matching xoxb-fake-*, xoxb-test-*, xapp-fake-*, xapp-test-*) into the new slack-helpers.sh and in each E2E script replace the local definition with a . ./slack-helpers.sh (or source) line so tests call is_fake_slack_token from the shared module and avoid duplication.
🤖 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/lib/onboard/slack-validation.ts`:
- Around line 39-54: The skip-warning is misleading because
shouldSkipSlackAuthValidation (and helper isTruthyEnvFlag) accept "1", "true",
"yes", "on" as skip values but skippedSlackValidationResult always claims the
sentinel was "=1"; update the code so the message accurately reflects the actual
value used or the accepted set: either (1) restrict isTruthyEnvFlag to only
accept "1" (if you want the documented sentinel) or (2) change
skippedSlackValidationResult to read the real env value via
env[SLACK_AUTH_VALIDATION_SKIP_ENV] (or include the accepted list) and include
that in the message; modify the functions isTruthyEnvFlag,
shouldSkipSlackAuthValidation, and skippedSlackValidationResult and reference
SLACK_AUTH_VALIDATION_SKIP_ENV accordingly so the log matches the runtime
behavior.
---
Outside diff comments:
In `@test/channels-add-preset.test.ts`:
- Around line 25-34: The child process env object used when calling runScript()
is inheriting NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION from the parent, allowing
Slack validation to be skipped in tests; update the env block in
test/channels-add-preset.test.ts (the env passed into runScript()/child process)
to explicitly set NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION: "" so the default tests
exercise the validation failure paths, and leave the individual opt-out test to
set NEMOCLAW_SKIP_SLACK_AUTH_VALIDATION = "1" as needed to override it.
---
Nitpick comments:
In `@test/e2e/test-token-rotation.sh`:
- Around line 159-164: Extract the duplicated is_fake_slack_token function into
a shared shell library (e.g., slack-helpers.sh) and update the E2E scripts to
source that library instead of redefining the function; specifically, move the
existing function body (the case pattern matching xoxb-fake-*, xoxb-test-*,
xapp-fake-*, xapp-test-*) into the new slack-helpers.sh and in each E2E script
replace the local definition with a . ./slack-helpers.sh (or source) line so
tests call is_fake_slack_token from the shared module and avoid duplication.
🪄 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: 27b624f9-4412-4b41-90dc-9bb32505ee5b
📒 Files selected for processing (13)
docs/reference/commands.mdxsrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/slack-channel-validation.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/onboard/slack-validation.test.tssrc/lib/onboard/slack-validation.tstest/channels-add-preset.test.tstest/e2e/test-channels-stop-start.shtest/e2e/test-hermes-slack-e2e.shtest/e2e/test-messaging-providers.shtest/e2e/test-openclaw-slack-pairing.shtest/e2e/test-token-rotation.shtest/onboard-messaging.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/actions/sandbox/slack-channel-validation.ts
- test/onboard-messaging.test.ts
- src/lib/onboard/messaging-channel-setup.ts
- src/lib/onboard/slack-validation.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26719155930
|
|
Reviewer update after the Slack validation follow-ups:
Local verification for the pushed follow-up: Current CI/advisor state on latest head
|
|
Follow-up on the latest advisor items:
E2E status context:
|
Selective E2E Results — ✅ All requested jobs passedRun: 26720047254
|
|
Required Scenario Advisor coverage completed: |
## Summary - Adds the v0.0.56 release notes section with links to the deeper docs pages for installer, status, inference, messaging, policy, and lifecycle changes. - Updates source docs for the remaining release-prep gaps around `uv` in the PyPI preset, compact WhatsApp pairing guidance, and `nemoclaw inference set` command boundaries. - Refreshes generated `nemoclaw-user-*` skills and removes skipped experimental command terms from generated skill surfaces. ## Source summary - #4613 -> `docs/manage-sandboxes/lifecycle.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Documents that public installs and `nemoclaw update` follow the maintained `lkg` tag by default. - #4419 -> `docs/about/release-notes.mdx`: Notes that non-interactive Linux installs can reactivate Docker group membership and continue in one installer run when `sg docker` is available. - #4550 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Captures live sandbox agent-version probing for status, connect, and upgrade checks. - #4609 -> `docs/inference/use-local-inference.mdx`, `docs/about/release-notes.mdx`: Captures the GPU Docker-driver host-network local-inference reachability gate. - #4607 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Documents compact WhatsApp QR pairing guidance and gateway/session diagnostics. - #4582 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Reflects Slack credential validation before enabling the channel. - #4554 -> `docs/manage-sandboxes/messaging-channels.mdx`, `docs/reference/troubleshooting.mdx`, `docs/about/release-notes.mdx`: Keeps Telegram allowlist alias guidance in the generated user skills and release notes. - #4563 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Includes the new `nemoclaw <name> skill remove <skill>` command in command docs and release notes. - #4566 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Documents the `nemoclaw inference set` redirect boundary when `--provider` or `--model` is missing. - #4323 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Captures per-sandbox status JSON support. - #4506 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Captures debug command sandbox-name validation and safer tarball writing. - #4569 -> `docs/network-policy/integration-policy-examples.mdx`, `docs/about/release-notes.mdx`: Documents that the `pypi` preset allows `/usr/local/bin/uv`. - #4579 -> `docs/network-policy/integration-policy-examples.mdx`, `docs/about/release-notes.mdx`: Captures observable Jira preset validation guidance. - #4229 -> `docs/manage-sandboxes/lifecycle.mdx`, `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Documents user-data preservation defaults for uninstall. - #4399 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Captures CPU-only sandbox intent preservation across rebuilds. - #4058 -> `docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Captures safer snapshot restore behavior around existing destinations. - #4155 and #4460 -> skipped by `docs/.docs-skip`: Removed skipped experimental command terms from source docs and generated skill evals instead of documenting those features. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `npm run docs` (passes; Fern reports the pre-existing light-mode accent contrast warning) - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" .agents/skills` (no matches) - `npm run build:cli` (run to refresh local CLI artifacts for the pre-push TypeScript hook) - Commit hooks passed, including `NEMOCLAW_* env-var documentation gate`, `Verify docs-to-skills output`, `markdownlint-cli2`, `gitleaks`, and `Test (skills YAML)`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded Model Router setup with YAML examples, flow diagrams, and credential handling; strengthened agent-config immutability and integrity guidance; messaging channels updated (Telegram aliases, WhatsApp pairing/diagnostics); CLI docs revised (GPU detection, inference set behavior, uninstall/rebuild preservation); overview rebranded to NemoClaw and added v0.0.56 release notes. * **New Features** * Added `nemoclaw <name> channels status` (messaging diagnostics, JSON); added `nemoclaw <name> skill remove`; Hermes no longer marked experimental; DGX Spark quickstart sandbox-name note. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - Add the missing `v0.0.57` release-notes section with links to the detailed docs pages for command, inference, onboarding, messaging, status, installer, and policy changes. - Remove public references to docs-skip terms from source docs and regenerate the NemoClaw user skills from the current Fern MDX docs. - Carry forward generated references for the per-agent documentation split, including Hermes-specific reference files. ## Source summary - #4615 and #4653 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover host-side `sessions` and `agents` commands plus `NEMOCLAW_EXTRA_AGENTS_JSON` secondary-agent baking. - #4163, #4204, #4611, #4619, and #4676 -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`: Release notes now cover managed vLLM progress/readiness, DGX Spark model default changes, local Ollama streaming usage, and inference route divergence warnings. - #4267, #4601, #4609, #4642, #4645, and #4661 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover UFW auto-remediation, local-inference reachability gates, gateway reuse/binding, cancel rollback, and policy selection persistence. - #4577, #4582, #4607, and #4660 -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/messaging-channels.mdx`: Release notes now cover Slack validation, atomic `channels add`, WhatsApp QR diagnostics, and Slack placeholder normalization. - #4388, #4600, #4646, and #4647 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Release notes now cover status failure layers, paused-container hints, Docker-driver doctor behavior, and non-destructive stale-registry recovery. - #4569, #4579, and #4678 -> `docs/about/release-notes.mdx`, `docs/manage-sandboxes/lifecycle.mdx`, `docs/network-policy/integration-policy-examples.mdx`: Release notes now cover installer tag pinning, PyPI `uv` policy access, and observable Jira validation. - #4632 -> `.agents/skills/`: Regenerated user skills from the current per-agent docs source, including newly generated Hermes reference files. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" docs --glob "*.mdx"` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" .agents/skills --glob "*.md"` - `npm run docs` - `npm run build:cli` - Commit hooks: markdownlint, docs-to-skills verification, gitleaks, skills YAML, commitlint <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Restructured documentation to clearly distinguish OpenClaw and Hermes agent variants throughout user guides. * Enhanced security, credential storage, and deployment guidance with clearer setup flows. * Added Hermes plugin installation and ecosystem documentation. * Improved workspace, messaging, and policy management references with variant-specific command examples. * Refined troubleshooting and CLI reference sections for clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
channels add slackpersists credentials or registers providersTests
Fixes #1912
Summary by CodeRabbit
New Features
Bug Fixes
Docs
Tests