fix(cli): prepare Hermes light terminal skin - #6578
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:
📝 WalkthroughWalkthroughThis PR adds Hermes light-terminal detection and managed skin configuration for sandbox connects, applies it before launching ChangesHermes light-theme skin preparation
Gateway recovery test timing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant connectSandbox
participant prepareHermesLightTerminalSkin
participant Sandbox
participant openshell
connectSandbox->>prepareHermesLightTerminalSkin: prepare Hermes skin before connect
prepareHermesLightTerminalSkin->>Sandbox: read Hermes config
prepareHermesLightTerminalSkin->>Sandbox: write skin YAML and updated config
connectSandbox->>openshell: spawn sandbox connect with constructed environment
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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
🧹 Nitpick comments (4)
src/lib/domain/sandbox/connect-env.test.ts (1)
14-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exact equality instead of
objectContaining.Since
buildSandboxConnectEnvcurrently just spreadsenv, assertingtoEqual(env)directly would give a stronger guarantee thanobjectContaining, which only checks the given keys are present and wouldn't catch unexpected additions/omissions.🤖 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/domain/sandbox/connect-env.test.ts` around lines 14 - 26, The test in buildSandboxConnectEnv is using a weak partial match, so update the assertion to compare the full returned environment exactly rather than using objectContaining. Keep the existing setup around buildSandboxConnectEnv, but change the expectation to verify the result matches the input env object precisely so unexpected keys or missing keys are caught.src/lib/domain/sandbox/connect-env.ts (1)
34-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate YAML parse/validate logic between the two helpers.
hermesConfigHasDisplaySkin(Lines 34-44) andbuildHermesLightSkinConfig(Lines 46-64) both repeat the same parse-and-validate sequence:YAML.parseDocument,doc.errors.length > 0check, and the root-type guard. Consider extracting a shared helper (e.g.parseHermesConfigDocument(configText): YAML.Document | null) that both functions call, to keep the validation rule in one place.♻️ Suggested extraction
+function parseHermesConfigDocument(configText: string): YAML.Document | null { + const doc = YAML.parseDocument(configText.trim() ? configText : "{}"); + if (doc.errors.length > 0) return null; + const root = doc.toJSON(); + if (root !== null && (typeof root !== "object" || Array.isArray(root))) return null; + return doc; +} + export function hermesConfigHasDisplaySkin(configText: string): boolean | null { try { - const doc = YAML.parseDocument(configText.trim() ? configText : "{}"); - if (doc.errors.length > 0) return null; - const root = doc.toJSON(); - if (root !== null && (typeof root !== "object" || Array.isArray(root))) return null; + const doc = parseHermesConfigDocument(configText); + if (!doc) return null; return doc.hasIn(["display", "skin"]); } catch { return null; } } export function buildHermesLightSkinConfig(configText: string): string | null { try { - const doc = YAML.parseDocument(configText.trim() ? configText : "{}"); - if (doc.errors.length > 0) return null; - - const root = doc.toJSON(); - if (root !== null && (typeof root !== "object" || Array.isArray(root))) return null; + const doc = parseHermesConfigDocument(configText); + if (!doc) return null; if (doc.hasIn(["display", "skin"])) return 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/domain/sandbox/connect-env.ts` around lines 34 - 64, Both hermesConfigHasDisplaySkin and buildHermesLightSkinConfig repeat the same YAML.parseDocument, doc.errors.length check, and root-type validation, so extract that shared parse/validate flow into a helper such as parseHermesConfigDocument(configText) in connect-env.ts and have both functions call it; then keep hermesConfigHasDisplaySkin focused on the display/skin lookup and buildHermesLightSkinConfig focused on the mutation path using the parsed document.src/lib/actions/sandbox/connect.ts (1)
118-133: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicate gating logic ahead of
shouldPrepareHermesLightSkin.Lines 123-130 hand-roll the same
agent?.name === "hermes",hostTerminalLooksLight(env),HERMES_TUI_LIGHT/HERMES_TUI_THEMEchecks thatshouldPrepareHermesLightSkin(fromconnect-env.ts) already encodes, then line 133 callsshouldPrepareHermesLightSkinagain with the same inputs plus the config check. This is understandable as an optimization to avoid the sandbox round-trip inreadSandboxHermesConfigwhen the cheap checks already fail, but the duplicated boolean logic can silently drift from the domain source of truth (e.g. ifhasEnvValue's trimming/semantics change inconnect-env.ts, this inline copy won't follow). Consider exposing a cheap pre-check (e.g. ahasEnvValue-style helper or ashouldPrepareHermesLightSkinvariant that accepts a lazy config getter) from the domain module instead of re-implementing the guard here.As per path instructions,
src/lib/{actions,domain,adapters,state}/**should avoid "duplicate sources of truth" between layers.🤖 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/sandbox/connect.ts` around lines 118 - 133, The guard logic in prepareHermesLightTerminalSkin duplicates the same Hermes/light-theme checks already owned by shouldPrepareHermesLightSkin, creating a second source of truth. Refactor this flow to reuse the domain logic from connect-env.ts instead of re-implementing the agent name, hostTerminalLooksLight, and env-value checks inline. Keep the early exit optimization, but move it behind a shared helper or a lazy-config variant of shouldPrepareHermesLightSkin so the sandbox config read only happens after the canonical pre-check passes.Source: Path instructions
src/lib/actions/sandbox/connect-hermes-light-theme.test.ts (1)
49-69: 📐 Maintainability & Code Quality | 🔵 TrivialAssertions are entirely mock-call/implementation-detail based.
Both tests verify behavior exclusively by inspecting
captureOpenshellSpy/runOpenshellSpycall arguments (exact CLI argv shape, substring checks on the generated shell script) rather than any observable outcome throughconnectSandbox's public surface. This locks the test to the exact shape of the generatedsh -cscript and argv positions (e.g.args.slice(0, 6).join(" "),args[7]), so refactoring the script construction (even without changing behavior) will break these tests without a real regression.Given the harness mocks the openshell adapter entirely, there's no alternative filesystem/output artifact to assert on directly, so this is a reasonable practical compromise — but it's worth being aware this is exactly the "mock-call assertion" pattern flagged for
**/*.test.{ts,js,mts,mjs,cts,cjs}.As per path instructions, tests should "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
Also applies to: 119-125
🤖 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/sandbox/connect-hermes-light-theme.test.ts` around lines 49 - 69, The test is asserting against mock-call implementation details instead of an observable outcome from connectSandbox. Update the connectHermes light-theme tests to verify the public behavior of the connectSandbox flow through the harness’s externally visible result/state, and avoid depending on exact argv positions, shell-script substrings, or captureOpenshellSpy/runOpenshellSpy call shapes. Use the connectSandbox and harness APIs as the stable entry points, and keep only assertions that reflect the intended outcome rather than the generated command text.Source: Path instructions
🤖 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/domain/sandbox/connect-env.ts`:
- Around line 80-86: The buildSandboxConnectEnv function is intentionally not
using the agent parameter, but it currently silences that with a void statement
instead of following the unused-variable convention. Rename the parameter in
buildSandboxConnectEnv to use an underscore prefix (for example, `_agent`) and
remove the void agent line so the intent is clear and consistent with the coding
guidelines.
---
Nitpick comments:
In `@src/lib/actions/sandbox/connect-hermes-light-theme.test.ts`:
- Around line 49-69: The test is asserting against mock-call implementation
details instead of an observable outcome from connectSandbox. Update the
connectHermes light-theme tests to verify the public behavior of the
connectSandbox flow through the harness’s externally visible result/state, and
avoid depending on exact argv positions, shell-script substrings, or
captureOpenshellSpy/runOpenshellSpy call shapes. Use the connectSandbox and
harness APIs as the stable entry points, and keep only assertions that reflect
the intended outcome rather than the generated command text.
In `@src/lib/actions/sandbox/connect.ts`:
- Around line 118-133: The guard logic in prepareHermesLightTerminalSkin
duplicates the same Hermes/light-theme checks already owned by
shouldPrepareHermesLightSkin, creating a second source of truth. Refactor this
flow to reuse the domain logic from connect-env.ts instead of re-implementing
the agent name, hostTerminalLooksLight, and env-value checks inline. Keep the
early exit optimization, but move it behind a shared helper or a lazy-config
variant of shouldPrepareHermesLightSkin so the sandbox config read only happens
after the canonical pre-check passes.
In `@src/lib/domain/sandbox/connect-env.test.ts`:
- Around line 14-26: The test in buildSandboxConnectEnv is using a weak partial
match, so update the assertion to compare the full returned environment exactly
rather than using objectContaining. Keep the existing setup around
buildSandboxConnectEnv, but change the expectation to verify the result matches
the input env object precisely so unexpected keys or missing keys are caught.
In `@src/lib/domain/sandbox/connect-env.ts`:
- Around line 34-64: Both hermesConfigHasDisplaySkin and
buildHermesLightSkinConfig repeat the same YAML.parseDocument, doc.errors.length
check, and root-type validation, so extract that shared parse/validate flow into
a helper such as parseHermesConfigDocument(configText) in connect-env.ts and
have both functions call it; then keep hermesConfigHasDisplaySkin focused on the
display/skin lookup and buildHermesLightSkinConfig focused on the mutation path
using the parsed document.
🪄 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: 8a19716f-c550-4a8f-85d9-20bb751ff888
📒 Files selected for processing (4)
src/lib/actions/sandbox/connect-hermes-light-theme.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/domain/sandbox/connect-env.test.tssrc/lib/domain/sandbox/connect-env.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
I resolved the merge conflict by merging current main (4df9fc2c7) into this branch and preserved both the inference-route hardening from main and this PR's Hermes hook. I also applied CodeRabbit's valid unused-parameter cleanup. The exact head is 0581b7ab0; the focused 11 tests, CLI type-check, npm run check:diff, and normal pre-push hooks pass.
I am requesting changes for three remaining hard gates:
-
The PR description is missing the contributor DCO declaration. All commits currently appear as GitHub
Verified, but the body has noSigned-off-by:line..github/PULL_REQUEST_TEMPLATE.md:31and:42-43require the contributor's declaration in the PR description. Please add your own valid sign-off (for example,Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>if that identity is correct); a maintainer cannot declare it on your behalf. -
The managed light skin is sticky across later dark-terminal connections.
src/lib/domain/sandbox/connect-env.ts:59persistsdisplay.skin: nemoclaw-light. On a later dark connection,src/lib/actions/sandbox/connect.ts:136-143exits before reading or reconciling that managed setting, andsrc/lib/domain/sandbox/connect-env.ts:53,76treats every existing skin—including NemoClaw's own—as an override. This conflicts with the stated “activate it only whenCOLORFGBGreports a light background” behavior and can recreate unreadable colors after the user switches to a dark terminal. Please define the managed-skin lifecycle without ever removing a user-owned skin, and add a light-to-dark transition regression test. -
The config write bypasses the supported Hermes mutation boundary and suppresses failure.
src/lib/actions/sandbox/connect.ts:153-165overwrites/sandbox/.hermes/config.yamlthrough ordinaryopenshell sandbox exec, withignoreError: trueand ignored stdio. The existing contract says direct in-sandbox edits are not adopted (docs/reference/commands.mdx:1081-1084), host config writes must share the root-only mutation lock and atomically refresh strict/compatibility hashes (:1086-1088), and config changes are refused while shields are up (:1090). The runtime enforces the same trust model inagents/hermes/start.sh:1850-1857. As written, this can silently fail under the normal locked posture or create config bytes that later fail strict-hash validation. Please route the change through the supported host mutation/lock/hash path (or choose a session-scoped design), surface failures, and cover shields-up plus hash-refresh behavior.
The documentation pass confirmed this changes user-visible connect behavior, but docs should follow after these lifecycle and mutation semantics are settled. I did not dispatch live E2E because the source/design gates above must be resolved first.
|
Exact-head CI follow-up for Please keep the changed test linear when addressing the requested source/design changes: split conditional behavior into separate cases, use declarative/mapped mock outputs, or move non-asserting conditional setup into a non-test helper. This is an additional required CI fix; no live E2E was dispatched while the source gates remain open. |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…_skin Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> # Conflicts: # test/support/connect-flow-test-harness.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/connect-hermes-light-theme.test.ts (1)
37-39: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake all Hermes override gates explicit in these tests.
The first case does not stub
HERMES_TUI_THEME; the other two cases stub neitherHERMES_TUI_THEMEnorHERMES_TUI_LIGHT. A runner-level override could therefore skip the new path while the tests still pass for the wrong reason. Set both variables to the intended empty/unset state in each case, with separate tests for non-empty overrides.Also applies to: 86-89, 113-115
🤖 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/sandbox/connect-hermes-light-theme.test.ts` around lines 37 - 39, The Hermes theme tests do not explicitly control all override gates, allowing runner environment values to affect results. In each test case around the existing environment stubs, explicitly set both HERMES_TUI_THEME and HERMES_TUI_LIGHT to empty/unset as intended, and add separate coverage for non-empty overrides; update all referenced cases consistently.Source: Path instructions
🧹 Nitpick comments (1)
src/lib/domain/sandbox/connect-env.test.ts (1)
19-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the environment is actually cloned.
This test only checks copied values, so it would pass if
buildSandboxConnectEnvreturned the original object. Capture the input, assertresultis not the same reference, and verify the input remains unchanged.As per path instructions, tests should verify observable behavior rather than only incidental implementation details.
🤖 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/domain/sandbox/connect-env.test.ts` around lines 19 - 30, The test should verify that buildSandboxConnectEnv returns a cloned environment without mutating the input. Store the input environment in a variable, capture the result, assert the result is not the same reference as the input, and confirm the original input remains unchanged while retaining the existing value assertions.Source: Path instructions
🤖 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/connect-hermes-light-theme.test.ts`:
- Around line 137-138: Update the test assertion after the write operation to
inspect the actual payload passed to writeSandboxConfig via
writeSandboxConfigSpy.mock.calls[0][2], asserting it equals { model: "test" } or
otherwise contains no display.skin; do not rely on hermesConfig, since it is the
same object returned by the read mock and may only demonstrate in-place
mutation.
In `@src/lib/domain/sandbox/connect-env.ts`:
- Around line 39-62: Refactor applyHermesLightSkinConfig and
removeHermesLightSkinConfig to avoid mutating the caller’s ConfigObject: return
an immutable result containing the transformed config and a changed boolean,
preserving existing no-op and invalid-input behavior. Update action-layer
callers to use the returned config and persist changes there, while keeping
domain logic limited to pure transformations and decisions.
- Around line 120-132: Update shouldApplyHermesLightSkin to distinguish a
missing display.skin key from an explicitly present non-string value; use the
relevant config structure/key-presence check alongside hermesConfigDisplaySkin
so only missing skin or managed light skin qualifies. Keep
applyHermesLightSkinConfig behavior consistent, and add a regression test
covering non-string display.skin values such as null to ensure light-skin
preparation is not incorrectly selected.
---
Outside diff comments:
In `@src/lib/actions/sandbox/connect-hermes-light-theme.test.ts`:
- Around line 37-39: The Hermes theme tests do not explicitly control all
override gates, allowing runner environment values to affect results. In each
test case around the existing environment stubs, explicitly set both
HERMES_TUI_THEME and HERMES_TUI_LIGHT to empty/unset as intended, and add
separate coverage for non-empty overrides; update all referenced cases
consistently.
---
Nitpick comments:
In `@src/lib/domain/sandbox/connect-env.test.ts`:
- Around line 19-30: The test should verify that buildSandboxConnectEnv returns
a cloned environment without mutating the input. Store the input environment in
a variable, capture the result, assert the result is not the same reference as
the input, and confirm the original input remains unchanged while retaining the
existing value assertions.
🪄 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: 51466300-474d-49e0-910e-319909700b61
📒 Files selected for processing (6)
src/lib/actions/sandbox/connect-hermes-light-theme.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/domain/sandbox/connect-env.test.tssrc/lib/domain/sandbox/connect-env.tssrc/lib/onboard/gateway-recovery.test.tstest/support/connect-flow-test-harness.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/connect.ts
| expect(harness.writeSandboxConfigSpy).toHaveBeenCalledOnce(); | ||
| expect(hermesConfig).toEqual({ model: "test" }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the configuration sent to the write boundary.
hermesConfig is the same object returned by the harness read mock, while writeSandboxConfig is a no-op. Checking the fixture afterward only proves in-place mutation; it can miss a regression where the write payload still contains display.skin. Assert writeSandboxConfigSpy.mock.calls[0][2] has no display.skin instead.
Suggested assertion
- expect(hermesConfig).toEqual({ model: "test" });
+ const writtenConfig = harness.writeSandboxConfigSpy.mock.calls[0][2];
+ expect(writtenConfig).toEqual(expect.objectContaining({ model: "test" }));
+ expect(writtenConfig).not.toHaveProperty("display.skin");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(harness.writeSandboxConfigSpy).toHaveBeenCalledOnce(); | |
| expect(hermesConfig).toEqual({ model: "test" }); | |
| const writtenConfig = harness.writeSandboxConfigSpy.mock.calls[0][2]; | |
| expect(writtenConfig).toEqual(expect.objectContaining({ model: "test" })); | |
| expect(writtenConfig).not.toHaveProperty("display.skin"); |
🤖 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/sandbox/connect-hermes-light-theme.test.ts` around lines 137
- 138, Update the test assertion after the write operation to inspect the actual
payload passed to writeSandboxConfig via writeSandboxConfigSpy.mock.calls[0][2],
asserting it equals { model: "test" } or otherwise contains no display.skin; do
not rely on hermesConfig, since it is the same object returned by the read mock
and may only demonstrate in-place mutation.
Source: Path instructions
| export function applyHermesLightSkinConfig(config: ConfigObject): boolean { | ||
| const display = config.display; | ||
| if (isConfigRecord(display)) { | ||
| if (display.skin !== undefined && display.skin !== NEMOCLAW_HERMES_LIGHT_SKIN_NAME) { | ||
| return false; | ||
| } | ||
| if (display.skin === NEMOCLAW_HERMES_LIGHT_SKIN_NAME) return false; | ||
| display.skin = NEMOCLAW_HERMES_LIGHT_SKIN_NAME; | ||
| return true; | ||
| } | ||
| if (display !== undefined) return false; | ||
| config.display = { skin: NEMOCLAW_HERMES_LIGHT_SKIN_NAME }; | ||
| return true; | ||
| } | ||
|
|
||
| export function removeHermesLightSkinConfig(config: ConfigObject): boolean { | ||
| const display = config.display; | ||
| if (!isConfigRecord(display) || display.skin !== NEMOCLAW_HERMES_LIGHT_SKIN_NAME) { | ||
| return false; | ||
| } | ||
| delete display.skin; | ||
| if (Object.keys(display).length === 0) delete config.display; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep config transforms pure in the domain layer.
applyHermesLightSkinConfig and removeHermesLightSkinConfig mutate caller-owned objects. Move this transformation to an immutable result ({ config, changed }) and let the action layer persist it, preventing shared-state surprises and aligning ownership boundaries.
As per path instructions, domain modules should make pure decisions while actions orchestrate and state modules own persisted state.
🤖 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/domain/sandbox/connect-env.ts` around lines 39 - 62, Refactor
applyHermesLightSkinConfig and removeHermesLightSkinConfig to avoid mutating the
caller’s ConfigObject: return an immutable result containing the transformed
config and a changed boolean, preserving existing no-op and invalid-input
behavior. Update action-layer callers to use the returned config and persist
changes there, while keeping domain logic limited to pure transformations and
decisions.
Source: Path instructions
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/connect-hermes-light-skin.ts (1)
57-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the apply/remove control flow to reduce branching.
The mixed if/else-if chain with embedded early returns (Lines 75-84) is hard to trace at a glance and pushes cyclomatic complexity higher than needed for this function.
♻️ Suggested simplification
- if (shouldApplyHermesLightSkin(agent, env, config)) { - const changed = applyHermesLightSkinConfig(config); - if (!changed && !hermesConfigUsesManagedLightSkin(config)) return; - if (!writeHermesLightSkinFile(sandboxName)) return; - if (!changed) return; - } else if (!shouldRemoveHermesLightSkin(agent, env, config)) { - return; - } else if (!removeHermesLightSkinConfig(config)) { - return; - } + let needsConfigWrite = false; + if (shouldApplyHermesLightSkin(agent, env, config)) { + const changed = applyHermesLightSkinConfig(config); + const alreadyManaged = hermesConfigUsesManagedLightSkin(config); + if (changed || alreadyManaged) { + if (!writeHermesLightSkinFile(sandboxName, target.configDir)) return; + needsConfigWrite = changed; + } + } else if (shouldRemoveHermesLightSkin(agent, env, config)) { + needsConfigWrite = removeHermesLightSkinConfig(config); + } + if (!needsConfigWrite) return;As per coding guidelines, "Keep function complexity low."
🤖 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/sandbox/connect-hermes-light-skin.ts` around lines 57 - 91, Refactor the apply/remove decision logic in prepareHermesLightTerminalSkin into a simpler, flatter flow with clearly separated apply and removal paths. Preserve the existing guards, file-write behavior, and early-return semantics while reducing nested branching; consider extracting the apply/remove operations into focused helper functions.Source: Coding guidelines
🤖 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 `@src/lib/actions/sandbox/connect-hermes-light-skin.ts`:
- Around line 57-91: Refactor the apply/remove decision logic in
prepareHermesLightTerminalSkin into a simpler, flatter flow with clearly
separated apply and removal paths. Preserve the existing guards, file-write
behavior, and early-return semantics while reducing nested branching; consider
extracting the apply/remove operations into focused helper functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ccc9bdf-06ea-4297-9794-6a6519138e40
📒 Files selected for processing (5)
src/lib/actions/sandbox/connect-hermes-light-skin.tssrc/lib/actions/sandbox/connect-hermes-light-theme.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/domain/sandbox/connect-env.test.tssrc/lib/domain/sandbox/connect-env.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/domain/sandbox/connect-env.test.ts
- src/lib/domain/sandbox/connect-env.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-6578.docs.buildwithfern.com/nemoclaw |
E2E Target Results — ✅ All requested jobs passedRun: 29111677227
|
E2E Target Results — ✅ All requested jobs passedRun: 29112526596
|
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - #6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - #6584 / #6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - #6210 / #6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - #6624 / #6623 / #6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - #6629 / #6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - #6634 / #6626 / #6596 / #5569 / #6610 / #6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - #6578 / #5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
wscurran
added
area: cli
## Summary - write a NemoClaw-managed `nemoclaw-light` Hermes skin inside the sandbox Hermes home for light terminal sessions - activate it only when `COLORFGBG` reports a light background and the user has not set `HERMES_TUI_THEME`, `HERMES_TUI_LIGHT`, or `display.skin` - keep the theme chrome unchanged while using the darker Hermes body/list text color from the upstream Hermes fix proposal Fixes NVIDIA#6380. Related upstream Hermes PR: NousResearch/hermes-agent#60972. ## Validation - `npm test -- --run src/lib/domain/sandbox/connect-env.test.ts src/lib/actions/sandbox/connect-hermes-light-theme.test.ts` - `git diff --check` - Linux smoke on `aits-log-worker-6`: real Hermes TUI output used `#7A5A0F` (`38;2;122;90;15`) for body/list text after writing `/sandbox/.hermes/skins/nemoclaw-light.yaml` and `display.skin: nemoclaw-light` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Hermes sandbox connections now automatically prepare/apply a light-compatible terminal skin when the host terminal appears light. * The connect flow now uses a computed connection environment and avoids unnecessary sandbox reconfiguration when a managed `display.skin` is already present. * **Bug Fixes** * Improved reconnect and failure handling: connection still proceeds while emitting safe, redacted warnings. * Skin/config operations are correctly scoped to the selected Hermes sandbox and skipped when user theme overrides are set. * **Tests** * Expanded Hermes connect-environment/skin behavior coverage and updated the connect-flow test harness (plus timing determinism for gateway recovery). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Chengjie Wang <chengjiew@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
## Summary Release-prep documentation for **v0.0.80**. Adds the `## v0.0.80` section to `docs/about/release-notes.mdx` summarizing user-facing changes since v0.0.79, each bullet linking to the relevant deeper page. Produced via `nemoclaw-contributor-update-docs` (pre-tag path): scanned `v0.0.79..HEAD`, applied the docs skip list (no violations), and confirmed the 8 commits that already shipped in-PR docs are complete. No new pages needed. ## Source summary - NVIDIA#6507 -> `docs/about/release-notes.mdx`: Hermes v0.18 + Slack Block Kit (rich rendering, digest-pinned base image). - NVIDIA#6584 / NVIDIA#6616 -> `docs/about/release-notes.mdx`: host-local OpenRouter runtime attribution adapter (port `11437`, `NEMOCLAW_OPENROUTER_RUNTIME_ADAPTER_PORT`) and native Deep Agents `openrouter` provider. - NVIDIA#6210 / NVIDIA#6292 -> `docs/about/release-notes.mdx`: host corporate proxy CA import into sandbox trust (`NEMOCLAW_CORPORATE_CA_BUNDLE`, `NEMOCLAW_CORPORATE_CA_IMPORT`). - NVIDIA#6624 / NVIDIA#6623 / NVIDIA#6656 -> `docs/about/release-notes.mdx`: release-matched base-image selection, surfaced cluster-image build diagnostics, preserved Nemotron profile registration. - NVIDIA#6629 / NVIDIA#6637 -> `docs/about/release-notes.mdx`: bare `connect` default-sandbox behavior and route-probe hardening. - NVIDIA#6634 / NVIDIA#6626 / NVIDIA#6596 / NVIDIA#5569 / NVIDIA#6610 / NVIDIA#6655 -> `docs/about/release-notes.mdx`: onboarding/recovery preservation, stale-gateway-PID fix, installer backup message, vLLM label on managed platforms. - NVIDIA#6578 / NVIDIA#5670 -> `docs/about/release-notes.mdx`: automatic Hermes light terminal skin and non-interactive `npx` MCP server startup. ## Verification `npm run docs`: 0 errors, all internal links resolve (2 pre-existing hidden-page warnings). `_build/` variants for OpenClaw, Hermes, and Deep Agents all regenerate with the v0.0.80 section. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added release notes for v0.0.80. * Documented Hermes upgrades, including Slack Block Kit rendering. * Added details on OpenRouter traffic routing and attribution headers. * Documented improved proxy certificate handling and sandbox reliability. * Highlighted enhanced connection defaults, route-probing safeguards, onboarding recovery, and terminal/MCP startup behavior. * Added references to relevant user-guide documentation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7 participants
Summary
nemoclaw-lightHermes skin inside the sandbox Hermes home for light terminal sessionsCOLORFGBGreports a light background and the user has not setHERMES_TUI_THEME,HERMES_TUI_LIGHT, ordisplay.skinFixes #6380.
Related upstream Hermes PR: NousResearch/hermes-agent#60972.
Validation
npm test -- --run src/lib/domain/sandbox/connect-env.test.ts src/lib/actions/sandbox/connect-hermes-light-theme.test.tsgit diff --checkaits-log-worker-6: real Hermes TUI output used#7A5A0F(38;2;122;90;15) for body/list text after writing/sandbox/.hermes/skins/nemoclaw-light.yamlanddisplay.skin: nemoclaw-lightSummary by CodeRabbit
display.skinis already present.Signed-off-by: Chengjie Wang chengjiew@nvidia.com