fix(sandbox/recover): revert Hermes probe boundary enforcement - #5547
Closed
sandl99 wants to merge 4 commits into
Closed
fix(sandbox/recover): revert Hermes probe boundary enforcement#5547sandl99 wants to merge 4 commits into
sandl99 wants to merge 4 commits into
Conversation
… on prob…" This reverts commit c4bd014.
Contributor
📝 WalkthroughWalkthroughRemoves the Hermes secret-boundary enforcement feature end-to-end: deletes exported marker constants and the ChangesHermes Secret-Boundary Enforcement Removal
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-5547.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/actions/sandbox/process-recovery.ts`:
- Line 490: The fail-closed Hermes boundary check was removed from the
live-gateway recovery path, allowing dashboard and forward recovery for
already-running gateways to proceed without re-validating the Hermes environment
file at `/sandbox/.hermes/.env`. This can leave a poisoned Hermes environment in
place without proper validation failure. Restore the deleted running-gateway
validator check or add an equivalent fail-closed boundary gate before any
dashboard or forward recovery operations in the affected code block (around line
506 and throughout the recovery logic in lines 502-568) to ensure the Hermes
environment is properly validated and `SECRET_BOUNDARY_REFUSED` is emitted when
appropriate.
🪄 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: 066c32a2-d2ec-4ac6-a269-3daf3c6e9e4b
📒 Files selected for processing (8)
docs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/connect-flow.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/agent/hermes-recovery-boundary.tssrc/lib/agent/runtime-hermes-secret-boundary-behavioural.test.tstest/process-recovery.test.ts
💤 Files with no reviewable changes (5)
- docs/reference/commands-nemohermes.mdx
- docs/reference/commands.mdx
- src/lib/actions/sandbox/connect-flow.test.ts
- src/lib/agent/runtime-hermes-secret-boundary-behavioural.test.ts
- src/lib/agent/hermes-recovery-boundary.ts
sandl99
changed the title
Signed-off-by: San Dang <sdang@nvidia.com>
sandl99
changed the title
Signed-off-by: San Dang <sdang@nvidia.com> # Conflicts: # test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
Collaborator
Author
|
Currently #5530 blocked me from testing new feature with nemohermes connect or recover. Please feel free add a fix or approve this revert PR to fix the issue. @cv @laitingsheng |
This was referenced Jun 22, 2026
Collaborator
|
Thanks @sandl99 — I checked Can you paste the exact redacted stderr from: nemohermes <sandbox> connect --probe-only
nemohermes <sandbox> recoverPlease redact values, but keep the key names and the boundary/refusal/inconclusive lines. If the failure is a raw-secret refusal, the right fix is probably to update the new feature’s Hermes config path to write openshell:resolve:env: placeholders instead of raw token values. If it is an inconclusive validator/root-exec failure, I’ll put up a narrow fix that preserves the #5530 security boundary instead of reverting it wholesale. |
Collaborator
Author
|
@cv @cjagwani Sorry I forgot to put the stderr in the description. Put it now. I am good if we have a forward fix for that given it's security issue. |
Collaborator
Author
|
@cv @cjagwani (Re-comment as my previous replies suddenly disappear) |
Collaborator
|
Thanks @sandl99 — I opened a forward-fix PR that keeps the #5530 Hermes running-gateway boundary enforcement but makes the root Could you please try that branch against your git fetch origin pull/5590/head:pr-5590-hermes-boundary-framing
git switch pr-5590-hermes-boundary-framing
npm run build:cli
nemohermes sb connect
nemohermes sb recoverExpected result: the normal healthy sandbox should no longer fail with |
14 tasks
Collaborator
Author
|
@cv I still encountered same issue after running on your bits. It can be repro in my both Ubuntu and MacOS |
Collaborator
|
Thanks @sandl99 — agreed, that means my first parser-only fix was incomplete. I couldn't reproduce with a live sandbox locally because my OpenShell gateway is down here (
Could you please retry #5590 at latest ( git fetch origin fix/hermes-recover-root-exec-framing
git switch fix/hermes-recover-root-exec-framing
git pull --ff-only
npm run build:cli
nemohermes hm connect
nemohermes hm recoverIf it still fails, the next useful datapoint would be whether this command prints the marker when run directly: openshell sandbox exec --name hm -- sh -c "printf '%s\\n' '__NEMOCLAW_SANDBOX_EXEC_STARTED__'; echo SECRET_BOUNDARY_OK" |
Collaborator
Author
|
@cv, it works perfectly at 7cfa0db and 2829b1d (some CI bugs). You can wrap it up at 7cfa0db. Thank you. |
Collaborator
|
Thanks for the fast retest @sandl99. Since your validation passed at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#5530 break
Reverts fix(sandbox/recover): enforce Hermes env-file secret boundary on probe path #5530
Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation