refactor(hermes): generate configurations from one policy - #8026
refactor(hermes): generate configurations from one policy#8026jyaunches wants to merge 24 commits into
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
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 configuration now uses one versioned managed-policy model. The policy is serialized, securely loaded by Python consumers, applied to runtime profiles and dashboard configuration, and used for inference route updates. Docker installation, startup wiring, probes, and tests validate the shared policy. ChangesHermes managed policy
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/hermes-start-config-integrity.test.ts (1)
113-118: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert the managed-policy argument in the seeder invocation.
prepare_hermes_dashboard_homepasses_HERMES_MANAGED_POLICYin the required first position, but the test does not assert this path. Assert the logged invocation includes the managed-policy path before/config.yaml.🤖 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/hermes-start-config-integrity.test.ts` around lines 113 - 118, Update the assertions for prepare_hermes_dashboard_home to verify that the seeder invocation includes the _HERMES_MANAGED_POLICY path in the required first argument position, immediately before /config.yaml, using the existing managed-policy path value.
🧹 Nitpick comments (6)
src/lib/actions/inference-route-api.ts (1)
4-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
src/lib/hermes-managed-route.tsas the only in-repository import path for these symbols.Import
hermesApiModedirectly insrc/lib/actions/inference-set.tsand its test. Update the placeholder imports in the three tests, then remove both forwarding exports. Retain compatibility paths only for documented external contracts with bounded retirement criteria.🤖 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/actions/inference-route-api.ts` around lines 4 - 10, Use src/lib/hermes-managed-route.ts as the sole in-repository import source for hermesApiMode and remove the forwarding exports from src/lib/actions/inference-route-api.ts:4-10 and src/lib/hermes-proxy-api-key.ts:4. Update inference-set, its test, and the three affected tests to import the symbols directly from hermes-managed-route.ts, retaining compatibility paths only where they represent documented external contracts with bounded retirement criteria.Sources: Coding guidelines, Path instructions
agents/hermes/managed_policy.py (1)
86-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the loop variable to avoid shadowing the
pathparameter.
load_managed_policytakespath: Path. Line 91 rebindspathto astrfrommanaged_paths. No current code readspathafter the loop, so behavior is correct today. The shadowing invites a type error in later edits, for example adding the file path to an error message.♻️ Proposed rename
config = document["config"] - for path in managed_paths: - policy_value(config, path) + for managed_path in managed_paths: + policy_value(config, managed_path)🤖 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/managed_policy.py` around lines 86 - 96, Rename the loop variable in load_managed_policy’s managed_paths iteration so it no longer shadows the method’s path: Path parameter, and update the policy_value call to use the new variable.agents/hermes/config/managed-policy.ts (1)
263-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
ttsguard.
REMOTE_PLATFORM_TOOLSETSnever contains"tts", so!remotePlatformToolsets.includes("tts")is always true. The check adds a condition that cannot fail.♻️ Proposed simplification
const remotePlatformToolsets = [...REMOTE_PLATFORM_TOOLSETS]; if ( settings.managedToolGateways.brokerEnabled && - settings.managedToolGateways.presets.includes("nous-audio") && - !remotePlatformToolsets.includes("tts") + settings.managedToolGateways.presets.includes("nous-audio") ) { remotePlatformToolsets.push("tts"); }🤖 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/config/managed-policy.ts` around lines 263 - 273, Remove the redundant remotePlatformToolsets.includes("tts") condition from buildHermesRemotePlatformToolsets, while preserving the brokerEnabled and nous-audio preset checks and the existing tts insertion behavior.agents/hermes/patch-profile-policy-defaults.py (1)
345-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport a manifest load failure as a build error.
load_managed_policyraisesManagedPolicyError. This call site does not catch it, so an invalid or unreadable manifest produces a traceback during the image build. The build still fails closed, but the message does not identify the manifest as the cause.patch_filealready converts patch failures intoSystemExitwith a clear prefix. Apply the same treatment here.♻️ Proposed change
- values = profile_default_values(load_managed_policy(args.policy)) + try: + values = profile_default_values(load_managed_policy(args.policy)) + except ManagedPolicyError as exc: + raise SystemExit(f"ERROR: {args.policy}: {exc}") from excAdd
ManagedPolicyErrorto themanaged_policyimport list.🤖 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/patch-profile-policy-defaults.py` around lines 345 - 353, Update the policy-loading flow around load_managed_policy to catch ManagedPolicyError and convert it into a SystemExit with a clear manifest-related error prefix, matching patch_file’s failure handling. Add ManagedPolicyError to the existing managed_policy imports and keep successful profile_default_values and patch_file processing unchanged.agents/hermes/config/hermes-config.ts (1)
5-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant forwarding module.
generate.tsalready imports both symbols frommanaged-policy.ts. Replace the two test imports and update the path-based test, documentation, and wrapper references. Then deleteagents/hermes/config/hermes-config.ts.🤖 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/config/hermes-config.ts` around lines 5 - 15, Remove the redundant hermes-config forwarding module and its exported symbols, then update all references to import buildHermesManagedPolicy and finalizeHermesPlatformToolsets directly from managed-policy.ts. Replace the two test imports and revise path-based tests, documentation, and wrapper references before deleting hermes-config.ts.Source: Coding guidelines
agents/hermes/seed-dashboard-config.py (1)
197-222: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for the dashboard provider rewrite. Use a gateway fixture with
model.provider: "custom"and assert that seeding sets it to_nemoclaw_upstream.provider_keywhile preserving the endpoint, proxy placeholder, and API mode across both provider paths.🤖 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 197 - 222, Add a regression test covering the dashboard provider rewrite through _normalized_routing, using a gateway fixture whose model.provider starts as "custom". Assert that the seeded model.provider becomes _nemoclaw_upstream.provider_key while endpoint, proxy placeholder, and API mode remain unchanged for both provider paths.
🤖 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/config/write-config.ts`:
- Around line 32-34: Align generate-config.ts, the Python consumers, and
start.sh on a single managed-policy artifact instead of maintaining separate
runtime and installed copies. Update the policy generation path around
policyPath to write the trusted artifact consumed at runtime, ensure env_lines
contain resolved credentials and gateway values rather than placeholders, and
update all consumers to read that same artifact so the manifest and config.yaml
remain consistent.
In `@agents/hermes/start.sh`:
- Around line 229-232: Remove the fallback assignment in the managed-policy
initialization near _HERMES_MANAGED_POLICY, leaving
/usr/local/share/nemoclaw/hermes-managed-policy.json as the sole policy path.
Preserve the existing missing-file behavior so the policy consumer fails closed
when the immutable artifact is absent, and do not read policy from HERMES_DIR.
In `@test/hermes-profile-policy-defaults.test.ts`:
- Around line 282-285: Replace the source-text checks on image-build-probes.py
in the hermes profile defaults test with a behavioral test that runs the probe
against a fixture managed policy and deliberately mismatched configuration.
Assert the process exit status and relevant stdout/stderr to verify the probe
derives and enforces profile_default_values(policy), following the existing
patchSource subprocess pattern; retain only observable public-boundary
assertions.
In `@test/seed-hermes-dashboard-config.test.ts`:
- Around line 65-90: Replace output assertions that currently derive expected
values through projectManagedPolicy or REVIEWED_POLICY with hand-authored
expected dashboard configuration values, specifically the assertions around
lines 178–182 and 439. Keep REVIEWED_POLICY unchanged for input fixtures, and
remove or stop using projectManagedPolicy for these expected outputs so the
assertions are independent of the seeder projection logic.
---
Outside diff comments:
In `@test/hermes-start-config-integrity.test.ts`:
- Around line 113-118: Update the assertions for prepare_hermes_dashboard_home
to verify that the seeder invocation includes the _HERMES_MANAGED_POLICY path in
the required first argument position, immediately before /config.yaml, using the
existing managed-policy path value.
---
Nitpick comments:
In `@agents/hermes/config/hermes-config.ts`:
- Around line 5-15: Remove the redundant hermes-config forwarding module and its
exported symbols, then update all references to import buildHermesManagedPolicy
and finalizeHermesPlatformToolsets directly from managed-policy.ts. Replace the
two test imports and revise path-based tests, documentation, and wrapper
references before deleting hermes-config.ts.
In `@agents/hermes/config/managed-policy.ts`:
- Around line 263-273: Remove the redundant
remotePlatformToolsets.includes("tts") condition from
buildHermesRemotePlatformToolsets, while preserving the brokerEnabled and
nous-audio preset checks and the existing tts insertion behavior.
In `@agents/hermes/managed_policy.py`:
- Around line 86-96: Rename the loop variable in load_managed_policy’s
managed_paths iteration so it no longer shadows the method’s path: Path
parameter, and update the policy_value call to use the new variable.
In `@agents/hermes/patch-profile-policy-defaults.py`:
- Around line 345-353: Update the policy-loading flow around load_managed_policy
to catch ManagedPolicyError and convert it into a SystemExit with a clear
manifest-related error prefix, matching patch_file’s failure handling. Add
ManagedPolicyError to the existing managed_policy imports and keep successful
profile_default_values and patch_file processing unchanged.
In `@agents/hermes/seed-dashboard-config.py`:
- Around line 197-222: Add a regression test covering the dashboard provider
rewrite through _normalized_routing, using a gateway fixture whose
model.provider starts as "custom". Assert that the seeded model.provider becomes
_nemoclaw_upstream.provider_key while endpoint, proxy placeholder, and API mode
remain unchanged for both provider paths.
In `@src/lib/actions/inference-route-api.ts`:
- Around line 4-10: Use src/lib/hermes-managed-route.ts as the sole
in-repository import source for hermesApiMode and remove the forwarding exports
from src/lib/actions/inference-route-api.ts:4-10 and
src/lib/hermes-proxy-api-key.ts:4. Update inference-set, its test, and the three
affected tests to import the symbols directly from hermes-managed-route.ts,
retaining compatibility paths only where they represent documented external
contracts with bounded retirement criteria.
🪄 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: 8a6ec943-5900-49ff-a96e-d67f92467b1a
📒 Files selected for processing (28)
agents/hermes/Dockerfileagents/hermes/config/generate.tsagents/hermes/config/hermes-config.tsagents/hermes/config/managed-policy.tsagents/hermes/config/write-config.tsagents/hermes/hermes-wrapper.pyagents/hermes/image-build-probes.pyagents/hermes/managed_policy.pyagents/hermes/patch-profile-policy-defaults.pyagents/hermes/seed-dashboard-config.pyagents/hermes/start.shsrc/lib/actions/inference-route-api.tssrc/lib/actions/inference-set-hermes-run.test.tssrc/lib/actions/inference-set-patch-hermes.test.tssrc/lib/actions/inference-set.tssrc/lib/hermes-managed-route.tssrc/lib/hermes-proxy-api-key.tssrc/lib/sandbox/config.tssrc/lib/sandbox/hermes-dashboard-reseed.test.tstest/generate-hermes-config.test.tstest/hermes-doctor-config-hash.test.tstest/hermes-final-image-layout.test.tstest/hermes-managed-policy.test.tstest/hermes-profile-policy-defaults.test.tstest/hermes-start-config-integrity.test.tstest/sandbox-provisioning.test.tstest/sandbox-rlimit-hooks.test.tstest/seed-hermes-dashboard-config.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. Since last review: 0 prior items resolved · 0 still apply · 0 new items found 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
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. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Security code review — FAILReviewed commit SHA This PR is not approval-ready. Issues #8008 and #8004 establish product scope, but two security and correctness findings remain. Blocking findings
Nine-category result
Files reviewed
Required GitHub checks pass for commit |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Keep dashboard authentication out of mirrored configuration. Carry the selected model context window through Hermes inference switches. Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Release-queue follow-up: current head b462e8a is DIRTY against main. The existing security review is bound to older head fac55ec, so it cannot establish the current head's disposition. Please resolve the conflict with a signed/verified commit, then refresh the full required CI/advisor/E2E matrix and an exact-head security review before approval. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Reviewed commit b462e8ad4 and the recorded fixes for issue #8008. The typed managed policy, versioned manifest, no-follow dashboard token read, environment-only credential delivery, generic errors, and context-window propagation address the prior credential and routing findings. No unresolved major review thread remains on this commit.
Approval is blocked by a behavior-changing conflict with current main, failed reviewed-npm audit and required CI/E2E checks, and extensive overlap in Hermes configuration, image, inference, credential, and documentation paths. Current main also removed or relocated the Hermes-specific skill reference changed by this branch. I did not apply an automatic merge because choosing between those contracts can change credential handling and generated configuration. A maintainer reconciliation must preserve the manifest schema, dashboard credential boundary, context-window behavior, current dependency audit, and current documentation ownership, then rerun the security and documentation reviews for the resulting commit.
wscurran
added
area: architecture
7 participants
Summary
Hermes configuration generation now derives the primary home, isolated dashboard home, and config-less named profiles from one typed managed policy. A versioned, root-owned, read-only manifest carries that policy into Python startup consumers. The security-review follow-up keeps dashboard authentication out of mirrored configuration and preserves the selected model context window during Hermes inference switches.
Related Issue
Closes #8008
Changes
context_lengthwhen the target window cannot be established.Type of Change
Quality Gates
12435980e. Local security re-review passed with no blockers: raw credential routes are denied; the gateway token is descriptor-read with no-follow validation, removed from internal launch metadata, and provided only to the dashboard process environment; errors are generic; and the selected model context window is propagated or deliberately omitted when unknown. The remaining CodeQL empty-except thread was addressed by documenting the non-retryable close and O_CLOEXEC boundary.Documentation Writer Review
docs-updateddocs/inference/configure-model-limits.mdx,docs/inference/switch-models.mdx,docs/manage-sandboxes/transfer-state-manually.mdx,docs/security/credential-storage.mdx,docs/inference/model-capability-audit.mdx, and.agents/skills/nemoclaw-contributor-update-hermes/references/hermes-contract-map.md. Independent exact-head review atb462e8ad4found no blockers and confirmed that the final test-only commit changes no production or user-facing contract, so it requires no additional documentation.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.sh.Verification
Signed-off-by:line and every published commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run test:changedpassed 191/191 before the current-main refresh; post-merge focused Hermes security/context, risk-plan, home-channel, and E2E-support validation passed 8 files and 133 tests with one file and 32 skips; the cross-runtime compatibility hash test hit its five-second timeout only under the concurrent focused run and passed 2/2 in isolation; focused wrapper tests passed 4 with 46 platform skips; the final E2E resilience change passed its focused support suite 17/17 andnpm run test:changed;npm run checks:repository,npm run test-size:check,npm run docs, Python syntax, checksum validation, ShellCheck, Hadolint, the test-conditionals scan, and normal hooks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; the change is scoped to Hermes policy generation and its named runtime consumers, covered by focused suites and normal repository hooks.npm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 existing warningsThe merged risk-plan v12 selects the issue-required Hermes live coverage, including Hermes smoke, inference switching, dashboard remote bind, Bedrock compatibility, shields, channel lifecycle, adapter lifecycle, and security posture targets. No unmapped E2E target remains.
Signed-off-by: Julie Yaunches jyaunches@nvidia.com