fix(cli): preserve Hermes light terminal hints - #6391
Closed
chengjiew wants to merge 4 commits into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesHermes TUI light-mode env injection
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant connect.ts
participant buildSandboxConnectEnv
participant openshell
User->>connect.ts: nemoclaw sandbox connect
connect.ts->>buildSandboxConnectEnv: buildSandboxConnectEnv(agent, process.env)
buildSandboxConnectEnv-->>connect.ts: env with optional HERMES_TUI_LIGHT
connect.ts->>openshell: spawnSync(sandbox connect, env)
openshell-->>User: Hermes TUI session
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings 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. |
![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
🧹 Nitpick comments (1)
src/lib/actions/sandbox/connect.ts (1)
153-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider moving light-mode decision logic to a domain module.
hasExplicitBoolean,hostTerminalLooksLight, andbuildSandboxConnectEnvare pure decision functions with no host/process calls, embedded directly in this actions file. Per the layering rules, actions should orchestrate while pure decisions live in domain modules.As per path instructions,
src/lib/{actions,domain,adapters,state}/**should be reviewed "againstsrc/lib/README.md: actions orchestrate, domain modules make pure decisions, adapters own host/process/network boundaries, and state modules own persisted files and state I/O."🤖 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 153 - 190, Move the pure environment/theme decision logic out of the actions layer: hasExplicitBoolean, hostTerminalLooksLight, and buildSandboxConnectEnv should not live in connect.ts because they are domain-style decision helpers. Extract them into an appropriate domain module under src/lib/domain, keep the actions file as an orchestrator, and update any imports/call sites so buildSandboxConnectEnv is referenced from the new domain location while preserving the existing behavior.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-flow.test.ts`:
- Line 74: Update the test title in connect-flow.test.ts so it keeps the
behavior-oriented wording for the Hermes connect session case and appends the
local issue reference suffix as required, changing the it(...) description to
end with “(`#6380`)”.
---
Nitpick comments:
In `@src/lib/actions/sandbox/connect.ts`:
- Around line 153-190: Move the pure environment/theme decision logic out of the
actions layer: hasExplicitBoolean, hostTerminalLooksLight, and
buildSandboxConnectEnv should not live in connect.ts because they are
domain-style decision helpers. Extract them into an appropriate domain module
under src/lib/domain, keep the actions file as an orchestrator, and update any
imports/call sites so buildSandboxConnectEnv is referenced from the new domain
location while preserving the existing behavior.
🪄 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: eb95dc4f-a385-42c1-b313-957c93cea07e
📒 Files selected for processing (2)
src/lib/actions/sandbox/connect-flow.test.tssrc/lib/actions/sandbox/connect.ts
| expect(exitSpy).toHaveBeenCalledWith(0); | ||
| }); | ||
|
|
||
| it("marks Hermes connect sessions as light-mode when launched from light macOS Terminal.app", async () => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add local issue reference to test title.
The title is behavior-oriented but is missing the required (#6380) suffix linking to the issue this regression test covers.
As per coding guidelines, **/*.test.ts should "Write behavior-oriented test titles, and put local issue references in a final (#1234) suffix."
✏️ Proposed fix
- it("marks Hermes connect sessions as light-mode when launched from light macOS Terminal.app", async () => {
+ it("marks Hermes connect sessions as light-mode when launched from light macOS Terminal.app (`#6380`)", async () => {📝 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.
Suggested change
| it("marks Hermes connect sessions as light-mode when launched from light macOS Terminal.app", async () => { | |
| it("marks Hermes connect sessions as light-mode when launched from light macOS Terminal.app (`#6380`)", async () => { |
🤖 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-flow.test.ts` at line 74, Update the test
title in connect-flow.test.ts so it keeps the behavior-oriented wording for the
Hermes connect session case and appends the local issue reference suffix as
required, changing the it(...) description to end with “(`#6380`)”.
Source: Coding guidelines
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 1 in-scope improvement
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: None Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 24-37: The afterEach cleanup in connect-hermes-light-theme.test.ts
has a redundant conditional around restoring process.stdout.isTTY; both branches
do the same reset behavior, so simplify it to a single Object.defineProperty
call using originalStdoutIsTty directly and remove the unnecessary if/else to
satisfy the growth guardrail.
In `@src/lib/domain/sandbox/connect-env.ts`:
- Around line 24-27: The sandbox domain helper is still crossing the process
boundary by defaulting `env` to `process.env`, which violates domain purity.
Update `buildSandboxConnectEnv` in `connect-env.ts` so `env` is a required
argument with no `process` reference, then change the `connect.ts` call site to
pass `process.env` explicitly. Keep the process access in the action layer and
let the domain helper only transform the provided environment.
🪄 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: 3c4b7d2d-32a7-438a-b0ea-cef23a0010a4
📒 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
Comment on lines
+24
to
+27
| export function buildSandboxConnectEnv( | ||
| agent: { name?: string } | null | undefined, | ||
| env: NodeJS.ProcessEnv = process.env, | ||
| ): NodeJS.ProcessEnv { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Domain helper directly references process.env, breaking layer purity.
src/lib/README.md requires domain modules to stay pure with no direct process/fs/Docker/network calls, and this PR explicitly calls for the connect action to delegate to "a pure domain helper... rather than manipulating process environment... inside the action." The default parameter env: NodeJS.ProcessEnv = process.env embeds a direct process reference inside the domain module itself, and the actual call site (connect.ts) never passes process.env explicitly — so the domain layer, not the action, ends up owning the process boundary access.
Make env a required parameter and have the action pass process.env explicitly, keeping the process boundary owned by the action layer as intended.
♻️ Proposed fix
export function buildSandboxConnectEnv(
agent: { name?: string } | null | undefined,
- env: NodeJS.ProcessEnv = process.env,
+ env: NodeJS.ProcessEnv,
): NodeJS.ProcessEnv {And update the call site in src/lib/actions/sandbox/connect.ts:
- env: buildSandboxConnectEnv(agent),
+ env: buildSandboxConnectEnv(agent, process.env),📝 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.
Suggested change
| export function buildSandboxConnectEnv( | |
| agent: { name?: string } | null | undefined, | |
| env: NodeJS.ProcessEnv = process.env, | |
| ): NodeJS.ProcessEnv { | |
| export function buildSandboxConnectEnv( | |
| agent: { name?: string } | null | undefined, | |
| env: NodeJS.ProcessEnv, | |
| ): NodeJS.ProcessEnv { |
🤖 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 24 - 27, The sandbox
domain helper is still crossing the process boundary by defaulting `env` to
`process.env`, which violates domain purity. Update `buildSandboxConnectEnv` in
`connect-env.ts` so `env` is a required argument with no `process` reference,
then change the `connect.ts` call site to pass `process.env` explicitly. Keep
the process access in the action layer and let the domain helper only transform
the provided environment.
Source: Path instructions
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
1 participant
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
Preserve light-terminal hints when connecting to Hermes sandboxes so Hermes can select its readable light TUI theme on macOS Terminal.app.
Fixes #6380.
Related Issue
Fixes #6380
Changes
HERMES_TUI_LIGHT=1for Hermessandbox connectsessions when the host terminal reports a light background and no explicit Hermes TUI theme/light override is set.COLORFGBGlight-background evidence instead of inferring light mode fromTERM_PROGRAMalone.HERMES_TUI_LIGHTandHERMES_TUI_THEME.COLORFGBG=0;15, dark backgrounds, explicit overrides, malformed inputs, and non-Hermes agents.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailableNPM_CONFIG_CACHE=/tmp/nemoclaw-6380-npm-cache npx vitest run --project cli src/lib/domain/sandbox/connect-env.test.ts src/lib/actions/sandbox/connect-hermes-light-theme.test.ts src/lib/actions/sandbox/connect-flow.test.tspassed, 20 tests.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:NPM_CONFIG_CACHE=/tmp/nemoclaw-6380-npm-cache npm testwas run and failed with 53 failures outside the touched connect files, including installer locale/timeouts, macOS/privatepath expectations, bash/BASHPID environment issues, Deep Agents Pythonfcntlsupport, policy-denial hint tests, and shields timing failures.npm run docsbuilds without warnings (doc changes only)Additional verification:
HERMES_TUI_LIGHTremained empty.src/lib/domain/sandbox/connect-env.test.tsfailed before the domain helper existed.NPM_CONFIG_CACHE=/tmp/nemoclaw-6380-npm-cache npm run typecheck:clipassed.NPM_CONFIG_CACHE=/tmp/nemoclaw-6380-npm-cache npx prek run --all-filespassed.git diff --checkpassed.Signed-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit