fix: correct Telegram bridge status detection (fixes #587)#614
fix: correct Telegram bridge status detection (fixes #587)#614kagura-agent wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdded a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/service-env.test.js`:
- Around line 151-162: The test currently counts occurrences of
sandboxEnvPrefix() which can include the helper declaration and thus miss a
missing call in start(); update the test to explicitly assert that the start()
implementation calls sandboxEnvPrefix() (in addition to stop() and showStatus())
— e.g., search the source for the start function body and assert it contains
sandboxEnvPrefix(), and keep the existing count/assertions for stop() and
showStatus() to ensure all three call sites exist; reference sandboxEnvPrefix()
and start(), stop(), showStatus() to locate the relevant checks in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d6c88c6-c7df-402a-963b-87540432b60f
📒 Files selected for processing (2)
bin/nemoclaw.jstest/service-env.test.js
test/service-env.test.js
Outdated
| it("sandboxEnvPrefix() is a shared helper used by start, stop, and status", () => { | ||
| const fs = require("fs"); | ||
| const src = fs.readFileSync( | ||
| path.join(__dirname, "..", "bin", "nemoclaw.js"), | ||
| "utf-8" | ||
| ); | ||
| const uses = (src.match(/sandboxEnvPrefix\(\)/g) || []).length; | ||
| // At least 3 call sites: start(), stop(), showStatus() | ||
| assert.ok( | ||
| uses >= 3, | ||
| `sandboxEnvPrefix() should be called at least 3 times (start/stop/status), found ${uses}` | ||
| ); |
There was a problem hiding this comment.
sandboxEnvPrefix() count check can miss a start() regression.
At Line 157, the regex also matches the helper declaration, so uses >= 3 can still pass with only declaration + stop() + showStatus() and no start() usage.
✅ Suggested test hardening
- it("sandboxEnvPrefix() is a shared helper used by start, stop, and status", () => {
+ it("start() passes SANDBOX_NAME via sandboxEnvPrefix", () => {
const fs = require("fs");
const src = fs.readFileSync(
path.join(__dirname, "..", "bin", "nemoclaw.js"),
"utf-8"
);
- const uses = (src.match(/sandboxEnvPrefix\(\)/g) || []).length;
- // At least 3 call sites: start(), stop(), showStatus()
- assert.ok(
- uses >= 3,
- `sandboxEnvPrefix() should be called at least 3 times (start/stop/status), found ${uses}`
- );
+ const startFn = src.match(/async function start\(\)[^]*?^}/m);
+ assert.ok(startFn, "start() function must exist");
+ assert.ok(
+ startFn[0].includes("sandboxEnvPrefix"),
+ "start() must call sandboxEnvPrefix() to resolve SANDBOX_NAME"
+ );
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("sandboxEnvPrefix() is a shared helper used by start, stop, and status", () => { | |
| const fs = require("fs"); | |
| const src = fs.readFileSync( | |
| path.join(__dirname, "..", "bin", "nemoclaw.js"), | |
| "utf-8" | |
| ); | |
| const uses = (src.match(/sandboxEnvPrefix\(\)/g) || []).length; | |
| // At least 3 call sites: start(), stop(), showStatus() | |
| assert.ok( | |
| uses >= 3, | |
| `sandboxEnvPrefix() should be called at least 3 times (start/stop/status), found ${uses}` | |
| ); | |
| it("start() passes SANDBOX_NAME via sandboxEnvPrefix", () => { | |
| const fs = require("fs"); | |
| const src = fs.readFileSync( | |
| path.join(__dirname, "..", "bin", "nemoclaw.js"), | |
| "utf-8" | |
| ); | |
| const startFn = src.match(/async function start\(\)[^]*?^}/m); | |
| assert.ok(startFn, "start() function must exist"); | |
| assert.ok( | |
| startFn[0].includes("sandboxEnvPrefix"), | |
| "start() must call sandboxEnvPrefix() to resolve SANDBOX_NAME" | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/service-env.test.js` around lines 151 - 162, The test currently counts
occurrences of sandboxEnvPrefix() which can include the helper declaration and
thus miss a missing call in start(); update the test to explicitly assert that
the start() implementation calls sandboxEnvPrefix() (in addition to stop() and
showStatus()) — e.g., search the source for the start function body and assert
it contains sandboxEnvPrefix(), and keep the existing count/assertions for
stop() and showStatus() to ensure all three call sites exist; reference
sandboxEnvPrefix() and start(), stop(), showStatus() to locate the relevant
checks in the test.
WuKongAI-CMU
left a comment
WuKongAI-CMU
left a comment
There was a problem hiding this comment.
Nice fix — extracting sandboxEnvPrefix() as a shared helper is the right call. The root cause (PIDDIR mismatch between start/stop/status) explains the misreported status cleanly.
A couple of observations:
-
The
showStatusregex in the test ([^]*?^}) uses a multiline anchor that depends on the function ending at column 0. If someone indents the closing brace or adds a nested function, this test would silently pass with a partial match. Thestop()regex ([^}]*\}) is more resilient. Maybe use the same greedy-to-brace pattern for both? -
Minor:
sandboxEnvPrefix()callsregistry.listSandboxes()on every invocation. ForshowStatus()this means it's called twice (once explicitly for sandbox info, once via the prefix). Not a real perf issue since it's just reading a JSON file, but worth noting.
Overall this is clean and well-scoped. 👍
|
Thanks for the thoughtful review @WuKongAI-CMU! On your two observations:
Let me push a fix for point 1 — it's a real improvement. |
stop() and showStatus() were not passing SANDBOX_NAME to start-services.sh, causing it to default to 'default' while start() passed the actual sandbox name from the registry. This meant the PID directory used by stop/status (/tmp/nemoclaw-services-default/) was different from the one used by start (/tmp/nemoclaw-services-<name>/), so status always reported 'stopped' and stop never found the running process. Extract sandboxEnvPrefix() helper and use it consistently in start(), stop(), and showStatus() so all three resolve SANDBOX_NAME the same way.
Address review feedback from @WuKongAI-CMU: both stop() and showStatus() regex patterns now use the same approach (`\{[\s\S]*?\n\}`) that handles nested braces and indented closing braces instead of relying on column-0 anchors.
kagura-agent
force-pushed
the
fix/587-telegram-bridge-status-is-mis
branch
from
2371060 to
568b2f5
Compare
3 participants
Problem
nemoclaw startlaunches the Telegram bridge correctly, butnemoclaw statusreports it as stopped andnemoclaw stopsays it's not running (even though the process is alive).Root Cause
In
bin/nemoclaw.js:start()resolves the sandbox name from the registry and passesSANDBOX_NAME=<name>tostart-services.sh, which saves the PID file in/tmp/nemoclaw-services-<name>/.stop()calledstart-services.sh --stopwithout passingSANDBOX_NAME, so the shell script defaulted to"default"and looked in/tmp/nemoclaw-services-default/— a different directory.showStatus()had the same problem — noSANDBOX_NAMEpassed, so it always checked the wrong PID directory.This meant the PID file written by
startwas invisible to bothstatusandstop.Fix
Extract a shared
sandboxEnvPrefix()helper that resolves the default sandbox name from the registry (same logicstart()already had), and use it in all three functions:start(),stop(), andshowStatus().Changes
bin/nemoclaw.js: ExtractsandboxEnvPrefix(), apply it tostop()andshowStatus()test/service-env.test.js: Add regression tests verifyingstop()andshowStatus()use the shared helper, preventing future driftTesting
All 189 existing tests pass (1 pre-existing unrelated failure in
install-preflight.test.js). 3 new regression tests added.Fixes #587
Summary by CodeRabbit
Refactor
Tests