fix(policy): treat preset picker stdin EOF as cancellation - #7510
Conversation
`policy-add` and `policy-remove` with no preset name and a closed stdin reached the interactive picker. The readline question callback never fired, the picker promise never settled, and the CLI exited 0 having applied nothing, so automation could not distinguish an applied preset from a no-op. Both pickers now reject with code EOF when readline closes before an answer, and both callers convert that rejection into exit 1. This adopts the EOF half of the prompt() contract in credentials/store.ts (NVIDIA#5990), which these pickers never used. An answered prompt and an empty-line cancel keep their current behavior. The EOF exit reports that stdin closed rather than reusing the NEMOCLAW_NON_INTERACTIVE message, because that variable is unset in this path and naming it misdirects the reader of a boot-unit log. Picker tests move to test/policy-preset-picker.test.ts because test/policies.test.ts sits at its size budget. The move drops that file under the default ceiling and retires its legacy exemption. The readline fake in the moved tests now emits close when close() runs, matching readline, so a successful selection exercises the reentrancy guard. Signed-off-by: harjoth <harjoth.khara@gmail.com>
📝 WalkthroughWalkthroughPolicy preset pickers now reject stdin EOF deterministically. The policy-add and policy-remove commands convert EOF into exit code 1 with diagnostic and usage output, while preserving propagation of non-EOF errors. Unit, command, integration, and test-budget coverage are updated. ChangesPolicy prompt EOF handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant policyChannel
participant selectFromList
participant askPreset
participant readline
CLI->>policyChannel: execute policy-add or policy-remove
policyChannel->>selectFromList: request preset selection
selectFromList->>askPreset: open interactive question
askPreset->>readline: create prompt
readline-->>askPreset: stdin closes before answer
askPreset-->>policyChannel: reject with code EOF
policyChannel-->>CLI: exit 1 with EOF message and usage
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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. |
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 `@test/package-contract/cli/policy-prompt-eof.test.ts`:
- Around line 71-88: Increase the timeout for the parameterized test covering
runPolicyCommandAtStdinEof so it exceeds the child process’s 30-second spawnSync
cap, using an explicit timeout such as 45 seconds when closing the it.each case.
Preserve the existing assertions and test behavior.
In `@test/policy-preset-picker.test.ts`:
- Around line 108-131: Update runSelectionPromptAtEof to stub process.stdin’s
ref, pause, and unref methods using the same setup and restoration pattern as
runSelectionPrompt. Ensure all related spies are restored during cleanup so
askPreset cannot alter the worker stdin handle state or leak mock effects
between tests.
🪄 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: 6a68c7fb-df6e-4bb4-9e0e-9a7be611723a
📒 Files selected for processing (7)
ci/test-file-size-budget.jsonsrc/lib/actions/sandbox/policy-channel-policy.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/policy/index.tstest/package-contract/cli/policy-prompt-eof.test.tstest/policies.test.tstest/policy-preset-picker.test.ts
💤 Files with no reviewable changes (1)
- test/policies.test.ts
Readline emits close for an interrupt as well as for stdin EOF, so the close handler added for NVIDIA#7418 reported Ctrl-C at the preset picker as "No input available on stdin" and exited 1. On a PTY the operator interrupted the command and the CLI named the wrong cause. askPreset now listens for SIGINT, rejects with code SIGINT, and re-raises the signal, so the process dies by SIGINT as credentials/store.ts already does. Verified on a real PTY: the command now terminates by signal 2 and prints no stdin message. Also stubs and restores stdin ref/pause/unref in the EOF harness, which otherwise left the Vitest worker's stdin unreferenced, and raises the package-contract case timeout above the child process cap so a hung picker fails on its assertions rather than as a suite timeout. Signed-off-by: harjoth <harjoth.khara@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/policy-preset-picker.test.ts`:
- Around line 186-208: Update the interrupt-test result construction around the
mocked process.kill in the policies[functionName] flow to return the signal
argument from kill.mock.calls, not only the call count. Adjust both
interrupt-case assertions to verify the returned signal is "SIGINT", while
preserving the existing error-code and re-raise checks.
🪄 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: 66f77527-483d-4bb1-93d2-4aad86412a33
📒 Files selected for processing (3)
src/lib/policy/index.tstest/package-contract/cli/policy-prompt-eof.test.tstest/policy-preset-picker.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/package-contract/cli/policy-prompt-eof.test.ts
- src/lib/policy/index.ts
The interrupt tests asserted only that process.kill ran once, so re-raising SIGTERM instead of SIGINT still passed. Capturing the signal argument and asserting it closes that gap: the same mutation now fails both cases. Signed-off-by: harjoth <harjoth.khara@gmail.com>
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed at 7d77283. EOF fails closed, SIGINT is preserved, and policy selection/apply behavior is unchanged. Focused unit, integration, and compiled-CLI tests pass; no unresolved review findings. Required E2E remains independently enforced.
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical `docs/changelog/2026-07-25.mdx` release entry with the exact `## v0.0.96` heading. The entry reconciles all 90 first-parent commits since v0.0.95 with all 92 merged PRs in the live `v0.0.96` label ledger and groups the user-visible changes by operator journey. ## Changes - Add the parser-safe dated MDX changelog entry for v0.0.96 with root-absolute links to the focused user guides. - Source summary: - [#7194](#7194) -> `docs/changelog/2026-07-25.mdx`: Document persistent baseline network policy exclusions and their inspection, rebuild, and snapshot behavior. - [#7188](#7188), [#7427](#7427), and [#7546](#7546) -> `docs/changelog/2026-07-25.mdx`: Document DNS-backed HTTPS inference routing, keyless loopback endpoints, and provider-marker isolation. - [#7238](#7238) -> `docs/changelog/2026-07-25.mdx`: Document blueprint sandbox and provider identifier validation before state writes or OpenShell calls, with bounded terminal-safe rejection previews. - [#7319](#7319), [#7274](#7274), [#7528](#7528), [#7353](#7353), and [#7560](#7560) -> `docs/changelog/2026-07-25.mdx`: Document the managed default gateway service, onboarding readiness, and container-runtime identity safeguards. - [#7349](#7349), [#7498](#7498), [#7406](#7406), [#7196](#7196), [#7559](#7559), [#7421](#7421), [#7510](#7510), [#7295](#7295), and [#7565](#7565) -> `docs/changelog/2026-07-25.mdx`: Document gateway-scoped status, lifecycle diagnostics, managed MCP recovery, delete-edge safeguards, and fail-closed CLI prompt and command output. - [#7591](#7591) -> `docs/changelog/2026-07-25.mdx`: Document opt-in authenticated MCP tool-name discovery, its bounded and names-only contract, probe interaction, and rebuild requirement. - [#7305](#7305), [#7480](#7480), [#7471](#7471), [#7365](#7365), and [#7541](#7541) -> `docs/changelog/2026-07-25.mdx`: Document installer version checks, version-tag reporting, license guidance, WSL Ollama selection, and DGX Station vLLM detection. - [#7482](#7482), [#7466](#7466), [#7208](#7208), [#7434](#7434), and [#7586](#7586) -> `docs/changelog/2026-07-25.mdx`: Document Ollama resource details, reasoning precedence, Hermes onboarding behavior, and preserved managed Hermes BuildKit failures. - [#6830](#6830), [#7492](#7492), [#7563](#7563), and [#7582](#7582) -> `docs/changelog/2026-07-25.mdx`: Document the authoritative OpenClaw production lock, fixed managed-image dependencies, immutable Hermes base adoption, and Hermes image-size reduction. - [#7505](#7505), [#7530](#7530), [#7547](#7547), [#7508](#7508), [#7548](#7548), [#7549](#7549), [#7537](#7537), [#7534](#7534), [#7515](#7515), [#7511](#7511), [#7551](#7551), [#7562](#7562), [#7575](#7575), [#7496](#7496), [#7594](#7594), [#7595](#7595), and [#7599](#7599) -> `docs/changelog/2026-07-25.mdx`: Summarize release validation, transient and bounded dispatch reconciliation, exact pre-tag qualification, identity revalidation, npm-audit retry, sharding, image reuse, timeout, telemetry, and workflow-hardening changes. - Reconciled without separate changelog prose: - [#7539](#7539), [#7526](#7526), [#7507](#7507), [#7506](#7506), [#7519](#7519), [#7516](#7516), [#7396](#7396), [#7254](#7254), [#7583](#7583), [#7596](#7596), and [#7598](#7598): Test-harness or fixture-only changes. - [#7403](#7403), [#7161](#7161), [#6877](#6877), [#7531](#7531), [#7525](#7525), [#7522](#7522), [#7536](#7536), [#7552](#7552), [#7566](#7566), [#7553](#7553), [#7561](#7561), [#7577](#7577), [#7569](#7569), [#7585](#7585), [#7584](#7584), [#7592](#7592), [#7580](#7580), [#7571](#7571), [#7517](#7517), [#7589](#7589), [#7402](#7402), [#7558](#7558), [#7544](#7544), and [#7601](#7601): Dependency, internal recovery, validation, contributor-workflow, E2E optimization, telemetry, or CI trust changes with no separate user-facing release claim. - [#7556](#7556), [#7573](#7573), [#7576](#7576), and [#7578](#7578): Experimental repository-maintainer conflict automation with no canonical user documentation surface. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates dated changelog structure, version headings, and published links. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed `docs/changelog/2026-07-25.mdx` at exact head `0f5dedb47` against 90 first-parent release commits and 92 merged PRs labeled `v0.0.96`. Verified parser-safe MDX SPDX, the exact version heading, literal CLI names, writing style, skip terms, all 20 root-absolute published links, and the accepted #7591 opt-in authenticated discovery bounds. #7544, #7599, and #7601 remain internal or CI-only release-ledger entries. Changelog tests passed 6/6, the docs build passed with 0 errors and two pre-existing Fern warnings, and `npm run check:diff` plus the final diff check passed. - Agent: Codex Desktop documentation-writer subagent <!-- docs-review-head-sha: 0f5dedb --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [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 test/changelog-docs.test.ts`: 6/6 passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this prose-only changelog entry. - [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) — the build passed with 0 errors and 2 existing Fern warnings; the published-route check passed. - [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) — native changelog files use the required parser-safe MDX SPDX comment and no frontmatter. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Persistent network policy exclusions with consistent restore/exclusion reporting across rebuilds/snapshots. * Opt-in MCP tool discovery via `mcp status --tools` with bounded, redacted authenticated traffic. * Improved HTTPS inference switching for custom endpoints and refreshed onboarding/model menu details. * Refined OpenShell gateway defaults for port `8080`, including more reliable readiness checks. * **Bug Fixes** * Prevent incorrect provider/model restoration after compatible-provider update failures. * Preserve managed MCP state after exec loss and tighten gateway/doctor status scoping. * **Tests** * Stronger, fail-closed release validation with hardened evidence/artifact handoff and bounded timeouts/retries. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
policy-addandpolicy-removewith no preset name and a closed stdin reached the interactive picker, whose prompt hit EOF. The readlinequestioncallback never fired, the promise never settled, and the CLI exited 0 having applied nothing, so automation could not distinguish an applied preset from a no-op.Both commands now report that stdin closed and exit 1. Ctrl-C at the picker terminates by SIGINT, because readline emits
closefor an interrupt as well as for EOF and the two must not report the same cause. Answered prompts and empty-line cancels are unchanged.Related Issue
Refs #7418
Fixes only the prompt defect in that report. The cold-boot probe/rollback root cause, the
sudo lsofcheck, and preset loss on re-onboard are untouched, so the issue stays open.Changes
src/lib/policy/index.ts: addsaskPreset(), used by both pickers. Rejects withcode: "EOF"when readline closes before an answer, and withcode: "SIGINT"on Ctrl-C, re-raising the signal so the process dies by SIGINT.finishedis set beforerl.close()— which itself emitsclose— so only a premature close rejects. This adopts theprompt()contract incredentials/store.ts(fix(onboard): treat prompt stdin EOF as cancellation (#5976) #5990, for [Linux][Onboard] nemoclaw onboard with stdin EOF exits 0 silently and skips "Installation cancelled" message #5976).onboard/messaging-selector.tsalready follows it; these pickers were the outlier.src/lib/actions/sandbox/policy-channel.ts: both call sites route throughpickPresetOrExit(), which convertsEOFintoexitPromptStdinClosed()and exits 1. Other errors propagate. Kept separate fromexitPresetNameRequired()because that meansNEMOCLAW_NON_INTERACTIVE=1was set, while this means stdin closed with it unset.test/policy-preset-picker.test.ts: picker tests moved out oftest/policies.test.ts, plus EOF and SIGINT coverage. The readline fake now emitscloseonclose(), matching real readline, so successful-selection tests exercise the reentrancy guard. Both harnesses stub and restore stdinref/pause/unrefso the Vitest worker's stdin handle is not left unreferenced.test/package-contract/cli/policy-prompt-eof.test.ts: drives the compiled CLI on real stdin at EOF, with subprocess timeout, exact status assertions, and tmpdir cleanup. The case timeout sits above the child cap so a hung picker fails on its assertions rather than as a suite timeout.src/lib/actions/sandbox/policy-channel-policy.test.ts: caller-boundary tests for EOF conversion and non-EOF propagation.ci/test-file-size-budget.json: the move was forced —test/policies.test.tswas pinned at exactly 1530 lines. The split drops it to 1338, under the 1500 default, so its legacy exemption is retired rather than raised.Type of Change
Quality Gates
docs/reference/commands.mdxalready requires the positional form in scripted workflows and documentsNEMOCLAW_NON_INTERACTIVE=1. That contract is unchanged; this fixes an undocumented failure mode.PR review advisor (Nemotron 3 Ultra)timed out during final synthesis after emitting a complete zero-finding ledger and final JSON; the primary Terra lane passed. Accepted as non-blocking advisor infrastructure: https://github.com/NVIDIA/NemoClaw/actions/runs/30139892547Documentation Writer Review
no-docs-neededVerification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not run; change is confined to two CLI source files and their testsnpm run docsbuilds without warnings (doc changes only)Evidence
A driver requires the built
dist/nemoclaw.js, setsprocess.argvto<sandbox> policy-addwith no preset, and stubs only registry and preset lookups. Real readline decides when the prompt closes.Before, on
origin/main— the settle marker never prints:After, on this branch:
Only a premature close rejects:
< /dev/null1printf '1\n'0printf '\n'0Ctrl-C at the picker, driven through a real PTY:
origin/main0No input available on stdin— wrong cause1Negative control —
policy-prompt-eof.test.tsagainst restoredorigin/mainsources:2 failed. On this branch:2 passed.Mutation testing:
rl.on("close")handlerif (finished) return;guardrl.on("SIGINT")handlerThe second is why the readline fake changed. With the original fake, deleting the guard left 17/17 tests passing, so the guard had no coverage.
Final reviewed SHA
7d77283ae: 25 caller-boundary tests, 19 picker tests, and 2 compiled-CLI EOF tests passed; CLI type-checking and normal pre-push hooks passed.typecheck:cli,test-size:check,test:projects:check, andgit diff --checkpass.Pre-existing failures
The full
package-contractproject fails 4 tests here and the identical 4 on pristineorigin/main(4 failed | 142 passedthere,4 failed | 144 passedhere).test:fastand thecliproject are order-dependent onorigin/mainitself — repeating the same command changes the failing set (main: 8 then 9 files; this branch: 2 then 5). Failures areTest timed out in 5000msacross unrelated files.policy-channel-refresh.test.tsfails on pristineorigin/main.policy-channel-conflict.test.tsalso appeared here, so I checked it: zero picker references, failing case ischannels start, passes 5/5 in isolation. Reporting it rather than filtering it, but not attributing it to this change.Limitations
store.tscontract rather than having regressed off it.src/lib/sandbox/config.tsconfirmYesNo()has the same never-settles shape and neither handler. It is a different command surface and outside NemoClaw cold-boot gateway recovery starts a gateway, then rolls it back #7418, so this PR leaves it alone rather than widening the change. It looks worth its own issue.AI-assisted: authored with Claude Code, reviewed and verified by the contributor.
Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit
policy-addandpolicy-removeexit with code1and show “No input available on stdin” on EOF (non-EOF errors still surface as before).