Conversation
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>
|
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 (5)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe 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. ChangesCredential recovery prompt
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to 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)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
rsliter
left a comment
There was a problem hiding this comment.
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.
|
Thanks for looking at this. The TTY question is worth asking, but I do not The condition you describe is already implemented, and neither PR changes 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
So the behaviour is global to every
The module comment gives the reason. Reading On scope. #12079 says it "does not change credential validation or This PR is the text change the issue asks for and nothing else. I am assigned On where the |
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
retryopens.Before:
After:
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.tsstates whatretry,back, andexitdo, 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.tsadds 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.tsandtest/onboarding/onboard-selection-anthropic-retry.test.tsupdate the prompt string and regex that they pin.ci/test-file-size-budget.jsonlowers theonboard-selection.test.tsbudget 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, andexitkeep their existing behavior at both prompts.Verification
npx vitest run --project cli src/lib/onboard/validation-recovery-prompt.test.ts: 10 passednpx vitest run --project integration test/onboarding/onboard-selection.test.ts: 62 passednpx vitest run --project integration test/onboarding/onboard-selection-anthropic-retry.test.ts: 2 passednpm run test:changed: passedNODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli: passednpm run format:check: passednpx oxlint --ignore-pattern 'src/lib/adapters/**' --ignore-pattern 'nemoclaw/src/**' .: passednpm run checks:repository: 19 checks passed, 0 failednpm run validate:prdid not complete.prekacquired~/.cache/prek/.lockand 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 --checkis 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:138has 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