fix(install): require an interactive terminal for --station-deepseek (#7014) - #7018
Closed
yanyunl1991 wants to merge 5 commits into
Closed
fix(install): require an interactive terminal for --station-deepseek (#7014)#7018yanyunl1991 wants to merge 5 commits into
yanyunl1991 wants to merge 5 commits into
Conversation
…--station-deepseek (#7008, #7009) #7008: NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 (and --yes-i-accept-third-party-software) inferred non-interactive mode, which then made --station-deepseek reject its own required interactive DGX Station express prompt, so the documented curl-pipe install aborted immediately. The two signals are orthogonal -- one accepts a licence, the other opts into an interactive express flow. Skip the inference when --station-deepseek is set so the express prompt is reached; the inference is preserved for every other install (its #4414/#2671 partial-install protection is unchanged). #7009: the conflict error printed the same message regardless of what put the run in non-interactive mode. main() exports NON_INTERACTIVE into NEMOCLAW_NON_INTERACTIVE, so the origin cannot be recovered at error time -- record it during flag parsing and name it in the message ("the --non-interactive flag" or "NEMOCLAW_NON_INTERACTIVE=1"). Verified on a DGX-Station-mocked aarch64 host: the reported command now proceeds past the express preflight without inferring non-interactive; explicit --non-interactive and NEMOCLAW_NON_INTERACTIVE=1 still conflict and now name their trigger; notice acceptance without --station-deepseek still implies non-interactive. Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
shfmt (-i 2 -ci -bn) puts the binary operator at the start of the continuation line. No behavior change. Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
- Drop the now-dead NON_INTERACTIVE_SOURCE assignment in the notice-acceptance block: with the #7008 guard it only runs for non-station installs, and its only reader (validate_station_deepseek_override) is station-only, so the value is never read. - Append the "(triggered by: ...)" clause only when the origin is known, so direct callers that set NON_INTERACTIVE without going through flag parsing get a clean message instead of a guessed attribution. No behavior change for real installs: in main(), NON_INTERACTIVE always has a recorded source when it is set. Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
…7014) --station-deepseek selects the interactive DGX Station express prompt. With no TTY, maybe_offer_express_install logged "Skipping express prompt (no TTY)" and continued, silently ignoring the flag and installing a different configuration. This surfaces once #7008 stops notice acceptance from forcing non-interactive, which lets accept-notice + --station-deepseek reach the express step. Add a TTY-availability check to validate_station_deepseek_override so the flag fails fast at preflight (before Docker / build deps) with a clear message, mirroring the existing --non-interactive rejection. Checked last so a genuine provider/model conflict is still reported first. A curl|bash pipe with /dev/tty available (the normal interactive case) still proceeds. Verified on a DGX-Station-mocked aarch64 host: no-TTY --station-deepseek now errors before any host mutation; --non-interactive and conflicting-model errors still take precedence; a real pty proceeds. Stacked on #7010 (#7008/#7009), which this builds on. Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe installer now detects whether an interactive terminal is available for ChangesDGX Station DeepSeek terminal validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 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 |
Contributor
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. 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: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/install-express-prompt.test.ts (1)
408-447: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClean up the temporary directory on test failure.
fs.rmSyncruns only after all assertions succeed, so a failed assertion orspawnSyncexception leaks the temporary directory. Move cleanup into afinallyblock or the test suite’s cleanup hook.🤖 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/install-express-prompt.test.ts` around lines 408 - 447, Update the temporary-directory test around the spawnSync and assertions so cleanup always runs, including assertion failures and spawnSync exceptions. Move fs.rmSync for tmp into a finally block or the suite’s cleanup hook while preserving the existing test behavior and assertions.
🤖 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 `@test/install-express-prompt.test.ts`:
- Around line 408-447: Update the temporary-directory test around the spawnSync
and assertions so cleanup always runs, including assertion failures and
spawnSync exceptions. Move fs.rmSync for tmp into a finally block or the suite’s
cleanup hook while preserving the existing test behavior and assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d8eea07e-aa9e-4030-bbe9-43a561d47eb8
📒 Files selected for processing (2)
scripts/install.shtest/install-express-prompt.test.ts
21 tasks
Collaborator
|
Superseded by #7045, a clean current-main replacement that preserves Yanyun Liao's original authorship and DCO credit, retains the validated fail-fast fix, makes the no-TTY regression portable, and includes the required user-facing documentation. Closing this conflicted duplicate so review and CI continue on one compliant branch. |
apurvvkumaria
added a commit
that referenced
this pull request
Jul 17, 2026
<!-- markdownlint-disable MD041 --> ## Summary Require `--station-deepseek` to have a readable interactive terminal so the installer fails before host mutation instead of silently selecting a different configuration. This is a clean current-main replacement for #7018 that preserves Yanyun Liao's original implementation and contributor credit. ## Related Issue Fixes #7014 ## Changes - Validate stdin or `/dev/tty` during the explicit Station DeepSeek preflight, before Docker or build-dependency setup. - Add a cross-platform no-controlling-terminal regression that proves the flag fails without host mutation while normal PTY and conflict paths remain covered. - Document the terminal requirement, the `curl | bash` behavior, and the equivalent headless managed-vLLM configuration in installer help, quickstart, vLLM setup, and generated platform support. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent maintainer review found no security finding. The local terminal probe is fail-closed, uses the existing installer TTY boundary, and the regression proves failure occurs before Docker or build-dependency mutation. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project installer-integration test/install-express-prompt.test.ts` (26 passed, 1 skipped); installer help rendering and platform-doc sync also passed. - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — `npm run check:diff` passed against current `origin/main`. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — build passed with 0 errors; Fern reported two pre-existing hidden/default warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — not applicable; no new page was added. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enforced that DGX Station `--station-deepseek` express install requires an interactive terminal; it now fails fast with a clear error when terminal access isn’t available. * Installer behavior no longer proceeds when reading the interactive prompt fails. * **Documentation** * Updated DGX Station setup guidance with terminal requirements, download/model expectations, and clarified deferred express onboarding status. * Added/expanded headless and non-interactive installation examples using environment variables. * **Tests** * Added coverage for terminal-required failure modes and prompt read resiliency. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Yanyun Liao <yanyunl@nvidia.com>
21 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
3 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
--station-deepseekselects the interactive DGX Station express prompt. On a host with no TTY,maybe_offer_express_installlogsSkipping express prompt (no TTY)and continues — so the flag is silently ignored and a different configuration is installed, with no error.This surfaces once #7008 stops notice acceptance from forcing non-interactive mode, which lets
NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=1 … --station-deepseekreach the express step instead of aborting earlier. (Plain--station-deepseekwith no TTY and no notice acceptance errors at the license prompt first; the silent-skip is the accept-notice + no-TTY corner.)Fix
Add a TTY-availability check to
validate_station_deepseek_override, so--station-deepseekfails fast at preflight (before Docker / build deps) with a clear, actionable message — mirroring the existing--non-interactiverejection. The check is last in the validator, so a genuine provider/model conflict is still reported first. Acurl | bashpipe where/dev/ttyis available (the normal interactive case) still proceeds.Verification (DGX-Station-mocked aarch64)
Via
validate_station_deepseek_overrideand fullmain:setsid, stdin/dev/null) +--station-deepseek→ errors before any host mutation (was: silent skip, express ignored).--non-interactive→ its own error first; conflictingNEMOCLAW_VLLM_MODEL→ model error first.--station-deepseek→ validate passes (interactive express still works).New test uses
setsidfor a deterministic no-TTY session; the existing--station-deepseek-with-TTY tests (run under a pty) still pass.Stacking
Builds on #7010 (#7008 / #7009); this PR targets that branch and shows only its own diff. GitHub will retarget it to
mainonce #7010 merges. It also adjusts the #7008 test to stop before validate (so it no longer needs a TTY to assert the arg-parse behavior).Fixes #7014.
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit
--station-deepseekwhen no interactive terminal is available.