fix(policy): scope Hermes messaging policies#3983
Closed
sandl99 wants to merge 1 commit into
Closed
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
sandl99
added
bug
Contributor
📝 WalkthroughWalkthroughThis PR implements agent-aware policy filtering to fix Hermes sandbox privilege escalation: sandboxes configured for Discord were retaining inactive Telegram/Slack/WeChat egress policies, and generic preset application was using Node-oriented binary allowlists instead of Hermes-specific Python rules. The PR adds infrastructure for loading agent-filtered presets, filters Hermes base policy during sandbox creation, and wires filtered preset loading through all preset application and detection call sites. ChangesHermes messaging policy filtering and agent-aware preset loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review AdvisorRecommendation: blocked This is an automated advisory review. A human maintainer must make the final merge decision. Limitations: Did not execute tests, package-manager commands, or PR scripts; review is based on trusted context, diff, and read-only repository inspection.; Git diff provided by the prompt was truncated in places; targeted files were inspected with read-only tools where needed.; No live GitHub workflow re-query was performed beyond the supplied trusted status context.; No E2E Advisor comment content was available; E2E recommendation check was still in progress. Full advisor summaryPR Review AdvisorBase: Blocked by failing CI/merge state and missing completed E2E validation for sandbox policy behavior; the code direction appears least-privilege-positive but needs gates and budget/E2E closure. Gate status
🔴 Blockers
🟡 Warnings
🔵 Suggestions
Acceptance coverage
Security review
Test / E2E status
✅ What looks good
Review completeness
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
5257-5265:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI line-budget gate is blocking this PR
Line 5264 introduced a net +1 line in
src/lib/onboard.ts, andonboard-entrypoint-budgetis failing. Please keep this file net-neutral so CI can pass.Possible net-neutral tweak
- additionalPresets: hermesToolGateways, - agentName: agent?.name || null, + additionalPresets: hermesToolGateways, agentName: agent?.name || null,🤖 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 `@src/lib/onboard.ts` around lines 5257 - 5265, The new parameter line "agentName: agent?.name || null," added to the options object passed to prepareInitialSandboxCreatePolicy caused a +1 net line; make the change net-neutral by moving that key onto the previous line (or combining it with another property) so the object stays the same but total lines do not increase. Edit the call site where initialSandboxPolicy is created (the prepareInitialSandboxCreatePolicy invocation) and merge "agentName: agent?.name || null" into the same line as "additionalPresets: hermesToolGateways," (or another neighbouring property) so the object contents are unchanged but the file line count remains neutral.
🧹 Nitpick comments (1)
src/lib/onboard/initial-policy.ts (1)
262-264: 💤 Low valueConsider using
HERMES_MESSAGING_POLICY_KEYSfor consistent channel-to-key mapping.The lookup checks if the channel name exists directly as a policy key, but
HERMES_MESSAGING_POLICY_KEYSshowswechat_bridge. This means theexistingChannelPresetseven when its policy is present.Since
CREATE_TIME_POLICY_PRESETS_BY_CHANNEL, this likely has no functional impact currently—just incompleteappliedPresetsmetadata if wechat support is later added to create-time presets.🤖 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 `@src/lib/onboard/initial-policy.ts` around lines 262 - 264, existingChannelPresets currently checks activeMessagingChannels against basePolicyNames directly, which misses channels like "wechat" that map to a different policy key; update the filter to map each channel via HERMES_MESSAGING_POLICY_KEYS (falling back to the channel name if no mapping) before checking basePolicyNames so channels with alias keys (e.g., wechat -> wechat_bridge) are detected; ensure this change is applied where existingChannelPresets is computed and that appliedPresets/CREATE_TIME_POLICY_PRESETS_BY_CHANNEL logic continues to use the mapped key for lookups.
🤖 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.
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 5257-5265: The new parameter line "agentName: agent?.name ||
null," added to the options object passed to prepareInitialSandboxCreatePolicy
caused a +1 net line; make the change net-neutral by moving that key onto the
previous line (or combining it with another property) so the object stays the
same but total lines do not increase. Edit the call site where
initialSandboxPolicy is created (the prepareInitialSandboxCreatePolicy
invocation) and merge "agentName: agent?.name || null" into the same line as
"additionalPresets: hermesToolGateways," (or another neighbouring property) so
the object contents are unchanged but the file line count remains neutral.
---
Nitpick comments:
In `@src/lib/onboard/initial-policy.ts`:
- Around line 262-264: existingChannelPresets currently checks
activeMessagingChannels against basePolicyNames directly, which misses channels
like "wechat" that map to a different policy key; update the filter to map each
channel via HERMES_MESSAGING_POLICY_KEYS (falling back to the channel name if no
mapping) before checking basePolicyNames so channels with alias keys (e.g.,
wechat -> wechat_bridge) are detected; ensure this change is applied where
existingChannelPresets is computed and that
appliedPresets/CREATE_TIME_POLICY_PRESETS_BY_CHANNEL logic continues to use the
mapped key for lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 036d70c6-2e6c-4049-99ab-eb935110dd35
📒 Files selected for processing (6)
agents/hermes/policy-additions.yamlsrc/lib/onboard.tssrc/lib/onboard/initial-policy.test.tssrc/lib/onboard/initial-policy.tssrc/lib/policy/index.tstest/policies.test.ts
12 tasks
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
2 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.
Summary
This PR scopes Hermes messaging policy so selected channels are applied without pre-enabling every Hermes messaging provider. It also makes dynamic preset application use Hermes-specific policy content, preventing Discord from falling back to generic Node-oriented allowlists on Hermes sandboxes.
Related Issue
Fixes #3981
Changes
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional checks run:
npm run build:clinpm run typecheck:clinpx vitest run src/lib/onboard/initial-policy.test.ts test/policies.test.tsgit commitgit pushSigned-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
New Features
Tests