Skip to content

refactor(sandbox): slack token override nits from PR #2151#2246

Merged
ericksoa merged 5 commits into
NVIDIA:mainfrom
Dongni-Yang:refactor/slack-token-override-nits-2151
Apr 22, 2026
Merged

refactor(sandbox): slack token override nits from PR #2151#2246
ericksoa merged 5 commits into
NVIDIA:mainfrom
Dongni-Yang:refactor/slack-token-override-nits-2151

Conversation

@Dongni-Yang

@Dongni-Yang Dongni-Yang commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Two optional nits deferred from PR #2151 review:

  • Regex substitution — replace json.load + json.dump(cfg, f, indent=2) in apply_slack_token_override with re.sub targeting only the two openshell:resolve:env:SLACK_* placeholder values in-place, preserving original openclaw.json formatting
  • Fold non-root fail-fast into function — when non-root and SLACK_BOT_TOKEN is set, apply_slack_token_override now returns 1 directly (instead of returning 0 and relying on a separate post-call guard at the call site); set -euo pipefail propagates the return 1 to a script exit with the same code and message as before

Test plan

  • npx vitest run test/nemoclaw-start.test.ts — 85/85 pass
  • Two tests updated: "only applies override in root mode" (now also asserts return 1 on non-root+token path) and "fails fast when SLACK_BOT_TOKEN is set in non-root mode" (now checks function body, asserts no separate call-site guard)
  • shfmt -i 2 -ci -bn -l scripts/nemoclaw-start.sh — no output (clean)

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced Slack token configuration validation to immediately fail with specific error messages when attempting to use bot tokens in non-root mode.
  • Tests

    • Updated tests to verify the new fail-fast security behavior for Slack token overrides.

Signed-off-by: Dongni Yang dongniy@nvidia.com

Dongni-Yang and others added 2 commits April 22, 2026 15:25
… token override

Patch only the two openshell:resolve:env:SLACK_* placeholder values in-place
using re.sub, preserving the original openclaw.json formatting instead of
round-tripping through json.load/json.dump which reformats the entire file.

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…en_override

When non-root and SLACK_BOT_TOKEN is set, apply_slack_token_override now prints
the error and returns 1 directly instead of silently returning 0. The separate
post-call check in the non-root block is removed. Under set -euo pipefail the
return 1 causes the script to exit 1 with the same error message as before.

Update tests to assert the fail-fast lives in the function body and that the
call site no longer carries a standalone SLACK_BOT_TOKEN guard.

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Apr 22, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

apply_slack_token_override() now fails fast (exit 1) with a clear SECURITY message when SLACK_BOT_TOKEN is set in non-root containers. Slack placeholder resolution switched from JSON parse/serialize to regex-based in-place text substitution; tests updated to assert the stricter behavior.

Changes

Cohort / File(s) Summary
Slack Token Override Refactoring
scripts/nemoclaw-start.sh
apply_slack_token_override() enforces fail-fast non-root behavior (prints SECURITY message, returns exit code 1) when SLACK_BOT_TOKEN is present; token replacement changed from JSON parse/modify/serialize to raw-text regex substitutions for botToken and optional appToken; removed redundant top-level non-root guard.
Test Updates
test/nemoclaw-start.test.ts
Tests updated to expect the exact non-root SECURITY message and return code 1; removed prior call-site guard/exit check and adjusted assertions to call apply_slack_token_override() directly for the non-root branch.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I sniffed the tokens in the night,
Rootless shadows met my light,
Regex nibbled where JSON slept,
A stern "SECURITY" woke and lept,
Hop—safe startup, snacks in sight 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: implementing deferred refactoring nits for the Slack token override mechanism from a prior PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
test/nemoclaw-start.test.ts (1)

572-584: Add one executable regression for the regex rewrite path.

These updated checks still only scan shell source. The risky part of this refactor is the embedded re.sub(...) rewrite, and a bad substitution or escaping bug would still pass here. A small fixture test that rewrites a sample openclaw.json and then reparses it would lock this down much better.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/nemoclaw-start.test.ts` around lines 572 - 584, Add an executable
regression that exercises the regex rewrite path: create a small test that runs
the same re.sub(...) rewrite logic on a sample openclaw.json fixture, writes the
rewritten file, then re-parses it to assert the rewrite produced valid
shell/source equivalent to the original semantics; locate the rewrite code
invoked by apply_slack_token_override and the re.sub(...) call, invoke that code
path in the test, and add assertions that the rewritten output can be parsed and
preserves the expected SLACK_BOT_TOKEN-related checks (e.g., the non-root return
behavior and absence of duplicate checks).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 460-482: The script currently injects raw SLACK_BOT_TOKEN and
SLACK_APP_TOKEN into the JSON by string substitution (see variables bot_token,
app_token and the re.sub calls), which can break the JSON if tokens contain " or
\; replace the regex splicing with proper JSON parsing and serialization: load
the file with json.load, set the "botToken" and "appToken" fields to
bot_token/app_token (only set appToken if app_token is non-empty), then write
back using json.dump or json.dumps to ensure values are correctly escaped;
remove the two re.sub blocks and the direct string write to content in favor of
updating the parsed object and writing it with json.dump.

---

Nitpick comments:
In `@test/nemoclaw-start.test.ts`:
- Around line 572-584: Add an executable regression that exercises the regex
rewrite path: create a small test that runs the same re.sub(...) rewrite logic
on a sample openclaw.json fixture, writes the rewritten file, then re-parses it
to assert the rewrite produced valid shell/source equivalent to the original
semantics; locate the rewrite code invoked by apply_slack_token_override and the
re.sub(...) call, invoke that code path in the test, and add assertions that the
rewritten output can be parsed and preserves the expected
SLACK_BOT_TOKEN-related checks (e.g., the non-root return behavior and absence
of duplicate checks).
🪄 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: b52c9543-75de-4510-836d-9e6fa33c680f

📥 Commits

Reviewing files that changed from the base of the PR and between 3199354 and a2fc653.

📒 Files selected for processing (2)
  • scripts/nemoclaw-start.sh
  • test/nemoclaw-start.test.ts

Comment thread scripts/nemoclaw-start.sh Outdated
…law.json

json.dumps(token)[1:-1] produces the inner string representation with all
special characters properly escaped (backslashes doubled, quotes escaped),
making the regex substitution safe for any token value even if it contains
JSON metacharacters.

Reported by CodeRabbit on PR NVIDIA#2246.

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Dongni-Yang Dongni-Yang changed the title refactor(sandbox): Slack token override nits from PR #2151 refactor(sandbox): slack token override nits from PR #2151 Apr 22, 2026
@ericksoa ericksoa merged commit 8222db8 into NVIDIA:main Apr 22, 2026
14 of 15 checks passed
@wscurran wscurran added the refactor PR restructures code without intended behavior change label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor PR restructures code without intended behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants