fix(hermes): store dashboard home as a profile - #7892
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHermes now stores the dashboard profile under ChangesHermes dashboard profile
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HermesEntrypoint
participant DashboardSeeder
participant LegacyProfile
participant CurrentProfile
HermesEntrypoint->>DashboardSeeder: invoke startup seeding
DashboardSeeder->>LegacyProfile: inspect legacy dashboard-home
DashboardSeeder->>CurrentProfile: migrate when destination is absent or empty
DashboardSeeder-->>HermesEntrypoint: continue seeding or exit 1
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@agents/hermes/seed-dashboard-config.py`:
- Around line 182-185: The migration path around os.rename in
seed-dashboard-config.py must safely create and validate profiles_dir before
renaming legacy_home when both the canonical profiles directory and
dashboard-home destination are absent. Update the migration test in
test/seed-hermes-dashboard-config.test.ts to cover that state, verifying the
public entrypoint reaches the replacement path and the superseded path does not
continue executing.
In `@docs/manage-sandboxes/backup-restore.mdx`:
- Line 43: Update docs/manage-sandboxes/backup-restore.mdx at lines 43-43 to
state that migration occurs only when the profiles destination is absent or
empty and the legacy path is not a symlink, including that unsafe merges are
refused. Update docs/manage-sandboxes/transfer-state-manually.mdx at lines 54-55
with commands for manually recovering preserved files from
.hermes/dashboard-home/, while retaining the prohibition on copying .env and
config.yaml.
In `@src/lib/sandbox/config.ts`:
- Line 725: Update the dashboard profile detection and inference flow around
dashboardHome to also inspect the legacy profile path and invoke the existing
seeder when only that profile exists. Return “absent” only when neither
canonical nor legacy profile exists, and ensure migration removes or prevents
execution through the old path. Add tests through public entrypoints proving
migration reaches the canonical path and the legacy path is deleted or cannot
execute.
🪄 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: de28addd-b031-41c7-93f5-fcbf3712fa16
📒 Files selected for processing (19)
agents/hermes/Dockerfileagents/hermes/manifest.yamlagents/hermes/seed-dashboard-config.pyagents/hermes/start.shdocs/inference/switch-providers.mdxdocs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/transfer-state-manually.mdxdocs/manage-sandboxes/workspace-files.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxscripts/update-hermes-agent.shsrc/lib/actions/inference-set.tssrc/lib/sandbox/config.tssrc/lib/sandbox/hermes-dashboard-reseed.test.tstest/e2e/live/hermes-inference-switch.test.tstest/e2e/live/hermes-root-entrypoint-smoke.test.tstest/hermes-start-config-integrity.test.tstest/seed-hermes-dashboard-config.test.tstest/update-hermes-agent-script.test.ts
|
Added the missing helper docstrings and extended the Hermes root-entrypoint smoke to migrate a real legacy dashboard profile while preserving its state, ownership, and modes. Focused migration and E2E contract tests pass on 6c3fe92. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/live/hermes-root-entrypoint-smoke.test.ts (1)
346-367: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover recursive profile state in the migration fixture.
The legacy setup and assertion cover only
MEMORY.md. A regression that loses nested profile files or the dashboard database could still pass. Add a representative nested/database artifact and assert its content, ownership, and mode after migration.This follows the PR objective to preserve profile state, including databases.
Also applies to: 417-420
🤖 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/live/hermes-root-entrypoint-smoke.test.ts` around lines 346 - 367, The assertLegacyDashboardMigration fixture currently verifies only MEMORY.md; expand the legacy setup to include a representative nested profile file and dashboard database, then assert both migrated artifacts’ contents, ownership, and permissions alongside the existing checks. Ensure the assertions target the migrated profiles/dashboard-home location and preserve the existing migration validations.
🤖 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 `@test/e2e/live/hermes-root-entrypoint-smoke.test.ts`:
- Around line 346-367: The assertLegacyDashboardMigration fixture currently
verifies only MEMORY.md; expand the legacy setup to include a representative
nested profile file and dashboard database, then assert both migrated artifacts’
contents, ownership, and permissions alongside the existing checks. Ensure the
assertions target the migrated profiles/dashboard-home location and preserve the
existing migration validations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e6b40f5d-9710-4da6-834e-0a999f45cddd
📒 Files selected for processing (2)
agents/hermes/seed-dashboard-config.pytest/e2e/live/hermes-root-entrypoint-smoke.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/hermes/seed-dashboard-config.py
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 `@agents/hermes/seed-dashboard-config.py`:
- Around line 214-218: Replace the check-then-os.rename flow around
_ensure_profile_parent with no-follow directory-descriptor operations: open and
validate the canonical profiles_dir, retain that descriptor, and perform the
rename relative to it. Refuse symlink redirection and ensure subsequent seeding
uses the validated descriptor or equivalent descriptor-relative paths, so the
mutated parent cannot change between validation and migration.
🪄 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: 280037e4-93f4-4f37-ae07-ff21df6d28eb
📒 Files selected for processing (6)
agents/hermes/seed-dashboard-config.pydocs/manage-sandboxes/backup-restore.mdxdocs/manage-sandboxes/transfer-state-manually.mdxsrc/lib/sandbox/config.tssrc/lib/sandbox/hermes-dashboard-reseed.test.tstest/seed-hermes-dashboard-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/manage-sandboxes/backup-restore.mdx
- docs/manage-sandboxes/transfer-state-manually.mdx
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 `@agents/hermes/seed-dashboard-config.py`:
- Around line 205-233: Wrap the os.rmdir call in _prepare_dashboard_destination
with an OSError handler. On failure, emit the established security/dashboard
refusal message including the exception and return (False, None), preserving
fail-closed behavior for races such as ENOTEMPTY or ENOTDIR.
🪄 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: 1554fbaa-c2c8-4aab-b8a7-e5a7aa23d9bb
📒 Files selected for processing (2)
agents/hermes/seed-dashboard-config.pytest/seed-hermes-dashboard-config.test.ts
|
Docstrings are updated and the branch is ready for another pass. @coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
agents/hermes/seed-dashboard-config.py (1)
807-811: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCompare normalized directories before reusing the validated descriptor.
os.path.dirname(argv[4]) == os.path.dirname(dst)is a textual match, so an equivalent-but-differently-spelled env destination (e.g..../dashboard-home/./gateway.env) silently drops to the path-based write inside_atomic_write_no_follow. That fallback still appliesO_NOFOLLOWto the final component, so it is a safe degradation rather than a hole — but normalizing keeps the intended anchoring in force.♻️ Suggested hardening
env_parent_fd = ( dashboard_fd - if len(argv) == 5 and os.path.dirname(argv[4]) == os.path.dirname(dst) + if len(argv) == 5 + and os.path.normpath(os.path.dirname(argv[4])) == os.path.normpath(os.path.dirname(dst)) else None )🤖 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 `@agents/hermes/seed-dashboard-config.py` around lines 807 - 811, Update the env_parent_fd selection in the dashboard configuration setup to compare normalized parent directories for argv[4] and dst rather than their raw textual forms. Preserve the existing len(argv) == 5 guard and reuse dashboard_fd whenever both destinations resolve to the same directory, including equivalent paths containing components such as "./".Source: Path instructions
test/seed-hermes-dashboard-config.test.ts (1)
250-295: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStub
os.rmdirfor keyword and positional calls.
fail_dashboard_removal(name, *, dir_fd=None)only acceptsdir_fdas keyword-only, so any positionalos.rmdir(name, dir_fd)reached during the run would raiseTypeErrorinstead of the intendedENOTEMPTY, turning a behavioral assertion into an incidental pass on a traceback-free path. Current production code only calls it with keywords, so this is future-proofing rather than a live defect.♻️ Suggested tweak
-def fail_dashboard_removal(name, *, dir_fd=None): +def fail_dashboard_removal(name, *args, **kwargs): if name == "dashboard-home": raise OSError(errno.ENOTEMPTY, "directory changed") - return original_rmdir(name, dir_fd=dir_fd) + return original_rmdir(name, *args, **kwargs)🤖 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/seed-hermes-dashboard-config.test.ts` around lines 250 - 295, Update the fail_dashboard_removal stub in the test harness to accept both positional and keyword forms of the dir_fd argument while preserving the intentional ENOTEMPTY failure for dashboard-home. Continue forwarding other calls to original_rmdir so the test remains focused on _prepare_dashboard_destination behavior.
🤖 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 `@agents/hermes/seed-dashboard-config.py`:
- Around line 807-811: Update the env_parent_fd selection in the dashboard
configuration setup to compare normalized parent directories for argv[4] and dst
rather than their raw textual forms. Preserve the existing len(argv) == 5 guard
and reuse dashboard_fd whenever both destinations resolve to the same directory,
including equivalent paths containing components such as "./".
In `@test/seed-hermes-dashboard-config.test.ts`:
- Around line 250-295: Update the fail_dashboard_removal stub in the test
harness to accept both positional and keyword forms of the dir_fd argument while
preserving the intentional ENOTEMPTY failure for dashboard-home. Continue
forwarding other calls to original_rmdir so the test remains focused on
_prepare_dashboard_destination behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 657d1429-92c0-4df1-a39d-1c3bf8fdef15
📒 Files selected for processing (2)
agents/hermes/seed-dashboard-config.pytest/seed-hermes-dashboard-config.test.ts
|
The two follow-up hardening notes are addressed and validated. @coderabbitai review |
|
✅ Action performedReview finished.
|
6720a64 to
a1e9b73
Compare
|
Rebased onto current main. Focused tests and |
a1e9b73 to
4a6b651
Compare
|
Rebased onto current main and revalidated the full profile migration. The Python seeder suite, focused CLI tests, and PR validation pass. |
wscurran
added
area: ui
|
✨ Thanks for the PR. This fixes the Hermes dashboard home migration by storing it as a canonical profile with real-process filesystem tests. Maintainers will review the migration logic, legacy state handling, and test coverage. Related open issues: |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head 4a6b651. The dashboard profile migration preserves legacy state, refuses symlink and dual-populated ambiguity, anchors subsequent writes to validated directory descriptors, retains legacy snapshot coverage, and updates runtime, reseed, documentation, and E2E paths consistently. I found no blocking correctness, security, compatibility, or data-loss issue. The failed E2E gate timed out waiting for trusted verdict infrastructure and does not identify a change-attributable failure.
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
4a6b651 to
46c0bf7
Compare
|
Rebased on current main. The dashboard migration suites and full PR validation pass, and all commits are verified. @coderabbitai review |
|
✅ Action performedReview finished.
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Synced with latest main and kept the new legacy Hermes restore coverage. Focused migration tests and PR validation pass; CI is rerunning. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
…oard-profile # Conflicts: # scripts/state-dir-guard.py # test/state-dir-guard.test.ts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Synced with current main while preserving Julie's dashboard hardening series. All commits are verified and CodeRabbit is clean; CI is rerunning. |
|
Synced with today's main while preserving the dashboard hardening series. All commits remain verified; advisors and E2E are still running. |
6 participants
Summary
Hermes Web Dashboard state now uses the canonical
~/.hermes/profiles/dashboard-homeprofile location.Startup preserves existing installations by moving the legacy dashboard home only when the destination is absent or empty.
Related Issue
Fixes #7200
Changes
profiles/dashboard-homein the Hermes image, startup path, runtime inference reseed, update tooling, live contracts, and user documentation.test/seed-hermes-dashboard-config.test.ts.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/manage-sandboxes/backup-restore.mdxanddocs/manage-sandboxes/runtime-controls.mdxfor Hermes dashboard-profile permissions and Shields behavior. Independent review verified terminology, structure, voice, code-sample presentation, behavioral accuracy, docs ownership, Hermes variant scoping, and security-risk ordering. Focused tests, repository checks, CLI build and type checking, semantic E2E validation, docs synchronization, docs build, and security re-review passed.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Additional validation:
npm run build:cli,npm run typecheck:cli,npm run lint,npm run docs, andnpm run validate:prpassed. Live E2E was not run because it mutates real Docker, OpenShell, and provider resources.Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
/sandbox/.hermes/profiles/dashboard-home/directory.