Skip to content

fix(onboard): clarify that retry opens the credential prompt - #12088

Open
MannXo wants to merge 1 commit into
NVIDIA:mainfrom
MannXo:fix/12079-credential-reentry-prompt
Open

MannXo wants to merge 1 commit into
NVIDIA:mainfrom
MannXo:fix/12079-credential-reentry-prompt

Conversation

@MannXo

@MannXo MannXo commented Sep 18, 2026

Copy link
Copy Markdown

Outcome

After provider credential validation fails, the options prompt told the user to re-enter the API key and then warned against entering one there. The prompt now states what each option does and names the secure prompt that retry opens.

Before:

NVIDIA Endpoints authorization failed. Re-enter the API key or choose a different provider/model.
⚠️  Do NOT paste your API key here — use the options below:
Options: retry (re-enter key), back (change provider), exit [retry]:

After:

NVIDIA Endpoints authorization failed.
Choose retry to enter the API key securely, back to change the provider or model, or exit to stop onboarding.
Options: retry, back, exit [retry]:

Reason

The warning applied to the options prompt, not to the masked credential prompt that opens after choosing retry. Read together the two lines request a key and forbid entering it, so a user cannot tell which prompt wants what.

Related issues

Fixes #12079

Changes

  • src/lib/onboard/validation-recovery-prompt.ts states what retry, back, and exit do, and drops the contradictory warning line. The options prompt stays masked, so a pasted key is still not echoed.
  • src/lib/onboard/validation-recovery-prompt.test.ts adds two tests. One covers the displayed instructions and asserts no line mentions pasting. One covers a key pasted at the options prompt and asserts it is not echoed.
  • test/onboarding/onboard-selection.test.ts and test/onboarding/onboard-selection-anthropic-retry.test.ts update the prompt string and regex that they pin.
  • ci/test-file-size-budget.json lowers the onboard-selection.test.ts budget from 4126 to 4124, which the size ratchet requires once the file is changed and the formatter collapses two shortened constants.

Credential validation, credential storage, non-interactive provider validation, and OpenShell registration are unchanged. retry, back, and exit keep their existing behavior at both prompts.

Verification

  • npx vitest run --project cli src/lib/onboard/validation-recovery-prompt.test.ts: 10 passed
  • npx vitest run --project integration test/onboarding/onboard-selection.test.ts: 62 passed
  • npx vitest run --project integration test/onboarding/onboard-selection-anthropic-retry.test.ts: 2 passed
  • npm run test:changed: passed
  • NODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli: passed
  • npm run format:check: passed
  • npx oxlint --ignore-pattern 'src/lib/adapters/**' --ignore-pattern 'nemoclaw/src/**' .: passed
  • npm run checks:repository: 19 checks passed, 0 failed
  • Reverted the source wording and reran the unit file, to confirm the two new tests fail without the change. That run gave 2 failed, 8 passed.

npm run validate:pr did not complete. prek acquired ~/.cache/prek/.lock and then held at 0% CPU with no further output for 15 minutes, so I stopped it and ran the file-hygiene hooks directly instead. git diff --check is clean, and every changed file ends in a newline, uses LF, carries its SPDX header, and contains no credential material. I have not run that gate and am not claiming it.

The diff contains no secrets, API keys, or credentials.

Review notes

src/lib/onboard/resume-provider-recovery.ts:138 has a similar "Re-enter the API key" line, in the sandbox-rebuild flow rather than onboarding validation recovery. It is outside this issue's boundary so I left it unchanged. Happy to file it separately if you want the wording aligned.


Signed-off-by: Parman Mohammadalizadeh prmma23@gmail.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved onboarding recovery prompts after credential authorization failures.
    • Retry now securely requests the API key again.
    • Back clearly lets you change the provider or model, while exit stops onboarding.
    • API keys entered at the options prompt are masked and do not overwrite the saved credential.
  • User Experience
    • Simplified retry, back, and exit instructions for faster, clearer navigation.

The options prompt told the user to re-enter the API key and then warned
against entering it there. That warning applied to the options prompt,
not to the masked credential prompt that retry opens, so the two lines
read as a contradiction.

State what retry, back, and exit each do, and name the secure prompt
that retry opens. The options prompt stays masked so a pasted key is
not echoed. Credential validation, storage, and non-interactive
behavior do not change.

Lower the onboard-selection test budget to the new line count, as the
size ratchet requires for a changed test file.

Signed-off-by: Parman Mohammadalizadeh <prmma23@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 18, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f8231ea7-c6a1-461a-b88e-3ee8dd584431

📥 Commits

Reviewing files that changed from the base of the PR and between 85290d3 and 5885922.

📒 Files selected for processing (5)
  • ci/test-file-size-budget.json
  • src/lib/onboard/validation-recovery-prompt.test.ts
  • src/lib/onboard/validation-recovery-prompt.ts
  • test/onboarding/onboard-selection-anthropic-retry.test.ts
  • test/onboarding/onboard-selection.test.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The authorization-failure prompt now explains secure retry, provider/model navigation, and exit. Tests cover accidental API-key paste handling, masked entry, navigation, credential preservation, and updated prompt matching.

Changes

Credential recovery prompt

Layer / File(s) Summary
Secure recovery messaging and validation
src/lib/onboard/validation-recovery-prompt.ts, src/lib/onboard/validation-recovery-prompt.test.ts, test/onboarding/onboard-selection*.test.ts, ci/test-file-size-budget.json
The prompt uses shorter action labels and explains that retry opens secure credential entry. Tests verify masking, navigation, paste handling, and credential preservation. Existing prompt matchers and the legacy file-size budget were updated.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Suggested reviewers: ericksoa, cv

Merge Risk: ⚪ Minimal · up to 58859

The updated recovery prompt directs users to secure credential entry while preserving retry, back, and exit behavior. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: clarifying that retry opens the credential prompt.
Linked Issues check ✅ Passed Issue #12079 requires clear recovery instructions and unchanged credential behavior. The prompt now separates the authorization failure message from the options message. It tells the user to choose `r…
Out of Scope Changes check ✅ Passed All changed files support issue #12079. The implementation changes only recovery prompt wording. The tests verify the wording and credential-recovery behavior. The onboarding prompt matchers and file-…
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@rsliter rsliter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wording repair is correct, but the implementation still calls the secret prompt without requiring both stdin and stderr to be TTY-capable. In non-TTY combinations, silent input can be unavailable, so a pasted API key can be exposed. PR #12093 addresses the same #12079 outcome, adds the fail-closed terminal check, preserves the existing credential, and covers the full terminal-capability matrix. Please close this PR as superseded by #12093 rather than duplicating that security repair.

@MannXo

MannXo commented Sep 19, 2026

Copy link
Copy Markdown
Author

Thanks for looking at this. The TTY question is worth asking, but I do not
think it belongs in #12079, and I would rather not close this PR over it.

The condition you describe is already implemented, and neither PR changes
it.
credentials/store.ts:715 computes

const silent = opts.secret === true && process.stdin.isTTY && process.stderr.isTTY;

which is the same stdin-and-stderr requirement. Its doc comment at :699 states
the fallback is deliberate:

{ secret: true } to mask input on a TTY (falls back to plain readline when
stdin/stderr is non-interactive, e.g. in CI).

So the behaviour is global to every secret: true call site, not something
this PR introduces at this one. Making the fallback fail closed is a change to
prompt() that affects all of them, and it needs the call sites that rely on
the readline path checked first.

isSecureCredentialPromptAvailable() reads process.stdin.isTTY, which the
repo asks callers not to do.
core/stdin.ts:33-39:

True when fd 0 is an interactive terminal. Asks the kernel directly, never
use process.stdin.isTTY for this (see module comment).

The module comment gives the reason. Reading process.stdin instantiates the
stream and libuv flips fd 0 to non-blocking process-wide, after which a later
fs.readSync(0) throws EAGAIN instead of blocking (#5188, regressed by
#5020). It names the onboard TTY probe as one of the paths that already causes
this. isStdinTty() (tty.isatty(0)) is the sanctioned form.

On scope. #12079 says it "does not change credential validation or
storage", and one of its acceptance criteria is that non-interactive provider
validation behaviour does not change. A process.exit(1) on a non-TTY session
changes it. CONTRIBUTING.md asks for the smallest complete change and for a
scope deviation to be disclosed before it is made.

This PR is the text change the issue asks for and nothing else. I am assigned
to #12079 and opened this before #12093, so I would prefer to finish it here.

On where the prompt() question goes, SECURITY.md asks that anything of that
shape not be raised in a public issue or PR, so I am not going to open one. I
am happy to take it to the private channel there, credit your report, and
write the patch if the maintainers want it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify secure API-key re-entry after provider validation fails

2 participants