refactor(sandbox): slack token override nits from PR #2151#2246
Conversation
… 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>
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
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 sampleopenclaw.jsonand 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
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/nemoclaw-start.test.ts
…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>
Summary
Two optional nits deferred from PR #2151 review:
json.load+json.dump(cfg, f, indent=2)inapply_slack_token_overridewithre.subtargeting only the twoopenshell:resolve:env:SLACK_*placeholder values in-place, preserving originalopenclaw.jsonformattingSLACK_BOT_TOKENis set,apply_slack_token_overridenow returns 1 directly (instead of returning 0 and relying on a separate post-call guard at the call site);set -euo pipefailpropagates thereturn 1to a script exit with the same code and message as beforeTest plan
npx vitest run test/nemoclaw-start.test.ts— 85/85 passreturn 1on 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
Tests
Signed-off-by: Dongni Yang dongniy@nvidia.com