fix(onboard): reject the Hermes dashboard internal port when it collides with the dashboard port#4931
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a validation check to reject Hermes dashboard onboarding configurations where the internal port equals the resolved Hermes API port. The validation mirrors existing external port checks and is covered by a new test case exercising the collision scenario. ChangesHermes Dashboard Internal Port Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/state/registry.ts`:
- Around line 217-257: The current lock-break path compares only the owner PID
before calling fs.rmSync(LOCK_DIR), which can mistakenly remove a newly-created
lock with the same PID; fix by capturing the original fs.statSync(LOCK_DIR)
result (e.g., lockStat) when you first read lockMtimeMs and then, inside the
"break" branch before rmSync, re-stat LOCK_DIR and require the identity to match
(compare fields like ino and dev, or at minimum mtimeMs and size) in addition to
rechecking LOCK_OWNER/ownerPid; only call fs.rmSync(LOCK_DIR, ...) when both the
owner check (recheck === ownerPid) and the stat identity check indicate the lock
is unchanged.
🪄 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: 65c26c19-63b4-418f-a1bf-000e77bc6bd2
📒 Files selected for processing (4)
src/lib/onboard/hermes-dashboard.test.tssrc/lib/onboard/hermes-dashboard.tssrc/lib/state/registry-lock.test.tssrc/lib/state/registry.ts
…des with the dashboard port resolveHermesDashboardOnboardState guarded the external Hermes dashboard port (NEMOCLAW_HERMES_DASHBOARD_PORT) against both the resolved dashboard/chat-UI port and the internal port, but the internal port (NEMOCLAW_HERMES_DASHBOARD_INTERNAL_PORT) was only checked against the external port. Setting it equal to the dashboard port passed onboarding validation and then collided when the forward was started. Add the missing symmetric check so the internal port is rejected up front, matching the existing external-port guard, with a unit test. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
latenighthackathon
force-pushed
the
fix/hermes-dashboard-internal-port-check
branch
from
0e8cbfa to
fd0e9c2
Compare
wscurran
added
area: onboarding
|
✨ Thanks for submitting this detailed PR about adding a check for the Hermes dashboard internal port collision during onboarding. This proposes a way to fix the regression in the onboarding flow for Hermes integration. Related open issues: |
1 similar comment
|
✨ Thanks for submitting this detailed PR about adding a check for the Hermes dashboard internal port collision during onboarding. This proposes a way to fix the regression in the onboarding flow for Hermes integration. Related open issues: |
3 participants
Summary
A Hermes agent's dashboard uses two ports: an external one people connect to and an internal one it serves on. Before creating the sandbox, NemoClaw already refuses to continue if the external dashboard port clashes with the resolved chat-UI port or with the internal port. It never ran the same check for the internal port, so setting the internal port to the chat-UI port's value slipped through onboarding validation and only failed later, when NemoClaw tried to open the dashboard forward on a port that was already in use. This adds the missing symmetric check so the conflict is reported up front with a clear message, the same way the external port already is.
Related Issue
Closes #4930
Changes
resolveHermesDashboardOnboardState(src/lib/onboard/hermes-dashboard.ts), reject the internal Hermes dashboard port when it equals the resolved dashboard port (config.internalPort === effectivePort), mirroring the existing external-port guard.Type of Change
Verification
Ran:
npx vitest run src/lib/onboard/hermes-dashboard.test.ts(4 passed),npx tsc -p tsconfig.src.json,npm run typecheck:cli, and Biome lint on the changed files, all clean.Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests