V2 terminal env#3184
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughIntroduces a v2 terminal env boundary: desktop resolves a strict shell-derived snapshot and launches host-service with that snapshot plus explicit runtime keys; host-service preserves a stripped terminal base env, provides deterministic shell bootstrap/launch semantics, and composes final PTY envs with a minimal terminal surface and v2 Superset metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Desktop as Desktop
participant HostService as HostService
participant DB as DB
participant PTY as PTY
Desktop->>Desktop: resolveTerminalShellSnapshot() (strict shell-derived env)
Desktop->>HostService: spawn host-service (env = shell snapshot + explicit keys)
HostService->>HostService: initTerminalBaseEnv() (store stripped baseEnv)
HostService->>DB: query.projects(workspace.projectId) -> rootPath
HostService->>HostService: resolveLaunchShell(baseEnv) / getShellLaunchArgs(...)
HostService->>HostService: buildV2TerminalEnv(baseEnv, shell, rootPath, ids)
HostService->>PTY: spawn(shell, shellArgs, env=V2Env)
PTY-->>HostService: PTY ready / shell runs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — documentation-only change with no runtime impact This PR adds only a planning document. There is no executable code, database schema, or configuration change. The only finding is a minor section numbering gap in the markdown. The plan itself is thorough, well-structured, and ready to guide implementation. plans/v2-terminal-env-handoff.md — minor section 6 numbering gap in 'Refined v2 contract' Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Desktop process.env] -->|shell env snapshot| B[Shell-derived Base Env]
B -->|strip desktop / Electron vars| C[Host-Service Launch Env]
C -->|sanitized base env| D[V2 Terminal Env Builder]
D --> E[Public Terminal Env]
D --> F[Superset Metadata]
D --> G[Shell Bootstrap Env]
E --> H[PTY]
F --> H
G --> H
H --> I[User Shell Session]
E:::note
classDef note fill:#f9f,stroke:#333
note1["TERM · TERM_PROGRAM · COLORTERM · LANG · PWD"] --> E
note2["SUPERSET_TERMINAL_ID · SUPERSET_WORKSPACE_ID\nSUPERSET_WORKSPACE_PATH · SUPERSET_AGENT_HOOK_PORT"] --> F
note3["zsh: ZDOTDIR\nbash: --rcfile\nfish: --init-command"] --> G
Reviews (1): Last reviewed commit: "Update doc" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plans/v2-terminal-env-handoff.md`:
- Line 153: SUPERSET_ENV in the v2 contract metadata list is undefined and
either needs documentation or removal; update the v2-terminal-env-handoff.md to
either (a) document SUPERSET_ENV by specifying its source/origin (who sets it,
when, valid values "development" or "production"), where it is read/used in the
system, and add acceptance criteria that verify its propagation/format, or (b)
remove SUPERSET_ENV from the metadata list so it isn’t part of the v2 contract;
reference the metadata list entry labeled SUPERSET_ENV when making the change
and update any related acceptance criteria sections to reflect the addition or
removal.
- Around line 173-261: Rename the markdown heading "### 7. Dynamic state" to
"### 6. Dynamic state" in plans/v2-terminal-env-handoff.md so the section
numbering follows "### 5. Shell behavior and integration"; update any nearby
numeric references if present to keep numbering consistent (search for "7.
Dynamic state" or "### 7." and replace with "6").
- Around line 121-128: The spec's environment block (variables like TERM,
TERM_PROGRAM, LANG, COLORTERM, PWD) must define fallback behavior for LANG
resolution: update the document to state that implementations should attempt to
derive a UTF-8 locale from the base environment and, if none is available or it
is non-UTF-8, set LANG to a safe default such as en_US.UTF-8 (or C.UTF-8 where
en_US is unavailable); explicitly note this fallback rule and the expected
precedence (use host-provided UTF-8 locale → fallback) so consumers of the spec
know to expect a consistent LANG value.
- Around line 133-134: Clarify that TERM_PROGRAM_VERSION must be the desktop app
(Superset) version rather than the host-service package or npm_package_version:
update the text and any code that sets TERM_PROGRAM_VERSION (replace uses of
npm_package_version or host-service package version) to pull the desktop app's
version identifier so the PTY presents the user-facing app version; reference
TERM_PROGRAM_VERSION, npm_package_version, and host-service in your change so
readers know which symbol to replace and which source to prefer.
- Around line 184-187: Validate inherited shell paths before using them: when
selecting the shell from the SHELL (macOS/Linux) or COMSPEC (Windows)
environment variables, check that the referenced path exists and is executable
(or on Windows, exists and is a file) and only use it if the validation
succeeds; otherwise fall back to the configured defaults (/bin/sh for Unix,
cmd.exe for Windows). Update the fallback logic described around the
SHELL/COMSPEC usage so that any invalid or non-executable environment value is
ignored and documented as such to avoid PTY spawn failures.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f5e457a-0407-4daa-917e-fa300618826c
📒 Files selected for processing (1)
plans/v2-terminal-env-handoff.md
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/host-service-manager.ts`:
- Around line 127-162: getShellEnvironment currently swallows errors and can't
convey whether it fell back, so modify getShellEnvironment to return { env:
Record<string,string>, source: "shell" | "fallback" } (or equivalent flag like
isFallback) and update resolveHostServiceBaseEnv to await that result and return
it directly (use the returned env and source) instead of the unreachable
try/catch; remove the dead catch block in resolveHostServiceBaseEnv, keep the
fallback filtering logic only if getShellEnvironment returns source ===
"fallback" (or rely on getShellEnvironment to already provide a filtered
fallback), and update any other callers of getShellEnvironment to handle the new
return shape.
In `@plans/v2-terminal-env-handoff.md`:
- Around line 283-298: Spec assumes a projects.repoPath column exists but the
projects table schema lacks it, breaking the derivation of SUPERSET_ROOT_PATH;
fix by either (A) adding a repoPath column to the projects schema with a DB
migration and updating any schema models, ORM mappings, tests and docs that
reference projects.repoPath and the SUPERSET_ROOT_PATH derivation, or (B) if you
prefer not to change the DB, update the spec and code that computes
SUPERSET_ROOT_PATH to use an alternate source (e.g., existing
SUPERSET_WORKSPACE_PATH or a new explicit field like projects.root or
workspace->project join logic) and update acceptance criteria, tests, and
documentation accordingly so projects.repoPath is no longer assumed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a1f4c6c-f8c6-4b9f-bad5-d0e9bc75512f
📒 Files selected for processing (5)
apps/desktop/src/main/lib/host-service-manager.tspackages/host-service/src/terminal/env.test.tspackages/host-service/src/terminal/env.tspackages/host-service/src/terminal/terminal.tsplans/v2-terminal-env-handoff.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/terminal/env.ts">
<violation number="1" location="packages/host-service/src/terminal/env.ts:22">
P2: Locale normalization is ordered incorrectly: `LC_ALL` has higher precedence than `LANG`, so preferring `LANG` can leave terminals on a non-UTF-8 locale.</violation>
</file>
<file name="apps/desktop/src/main/lib/host-service-manager.ts">
<violation number="1" location="apps/desktop/src/main/lib/host-service-manager.ts:139">
P2: The `catch` block appears to be unreachable. `getShellEnvironment()` has its own internal try-catch that returns a fallback env rather than throwing, so this function always takes the `try` path and returns `source: "shell"` — even when shell resolution actually failed internally. This means:
1. The fallback filtering logic (~20 lines) is dead code
2. The `source` discriminant is misleading since it can never be `"fallback"`
Either remove the dead catch block, or modify `getShellEnvironment()` to expose whether a fallback was used (e.g., return `{ env, isFallback }`) so the caller can distinguish the two cases.</violation>
</file>
<file name="packages/host-service/src/terminal/env.test.ts">
<violation number="1" location="packages/host-service/src/terminal/env.test.ts:18">
P2: This assertion is not platform-safe: it fails on Windows where `resolveLaunchShell` returns `COMSPEC`/`cmd.exe` instead of `SHELL`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Builds the v2 terminal env end to end. Adds theme‑aware color hints and a macOS TLS cert fallback, and locks PTYs to a preserved, shell‑derived base env for reliable CLI behavior.
New Features
@superset/host-service/terminal-env.process.env), injectTERM_PROGRAM=Superset/version, UTF‑8LANG,PWD, v2 metadata (SUPERSET_TERMINAL_ID,SUPERSET_WORKSPACE_ID/_PATH,SUPERSET_ROOT_PATH,SUPERSET_ENV,SUPERSET_AGENT_HOOK_PORT/_VERSION), and preserve user tooling likeSSH_AUTH_SOCK; non‑Windows falls back to/bin/sh.ZDOTDIRwrapper, bash rcfile or-l, fish--init-command; unsupported shells launch natively.COLORFGBG(dark by default, light when requested). On macOS, PTYs setSSL_CERT_FILE=/etc/ssl/cert.pemwhen available to avoid Keychain TLS issues.Tests
ELECTRON_*,npm_*,VITE_*,HOST_*), UTF‑8 locale, shell args/bootstrap, fallback behavior, v2 metadata, theme hinting, macOS cert fallback, and fail‑closed when the snapshot is missing.SUPERSET_HOME_DIR, hook port/version,DESKTOP_VITE_PORT, auth/cloud) without leaking dev/Electron/runtime secrets.Written for commit f3da34c. Summary will update on new commits.
Summary by CodeRabbit