feat: fix the 451 CoreConsole error into a better user flow to accept developer terms#262
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
The implementation is clean, well-documented, and consistently applied across all relevant commands. The logic for gating prompts behind skipPrompts and the fast-fail path is correct. One minor issue: the template literal in the prompt message mixes a hardcoded newline with a \n escape and leading whitespace, producing inconsistent spacing in the rendered output.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| this.error('The Developer Terms of Service were declined') | ||
| } | ||
| const accepted = await consoleCLI.acceptDevTermsForOrg(orgId) | ||
| if (!accepted) { |
There was a problem hiding this comment.
The prompt string has a raw newline followed by \n and 4 spaces of indentation, which will render as an extra blank line with leading whitespace in the terminal. Flatten to a single consistent separator.
| if (!accepted) { | |
| const confirmDevTerms = await consoleCLI.prompt.promptConfirm(`${terms.text}\n\nYou have not accepted the Developer Terms of Service. Go to ${hyperlinker(DEV_TERMS_URL, DEV_TERMS_URL)} to view the terms. Do you accept the terms? (y/n):`) |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🤖 PR Reviewer
The implementation is clean, well-tested, and consistently applied across all subcommands. The ensureDevTermAccepted method correctly handles the accept/decline/skip-prompts flows, and the test coverage is thorough. One previously raised issue about the prompt string formatting remains unaddressed.
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| } | ||
| const accepted = await consoleCLI.acceptDevTermsForOrg(orgId) | ||
| if (!accepted) { | ||
| this.error('The Developer Terms of Service could not be accepted') |
There was a problem hiding this comment.
[Re-raised] The prompt string contains a literal \n followed by \n (resulting in a double newline between terms text and the prompt message), which is intentional and fine, but the string also starts with ${terms.text} which may itself contain trailing newlines causing inconsistent terminal output. More critically, the raw \n in a template literal renders as a newline character which is correct — however the previous review flagged that the indentation of the source line (4 spaces before the closing backtick area) could cause leading whitespace. The real issue is the string is fine as-is for the newlines, but terms.text is user-controlled content from the API and is rendered directly into the terminal without any sanitization or length check, which could be a large wall of text overwhelming the prompt.
| this.error('The Developer Terms of Service could not be accepted') | |
| const termsText = terms.text ? terms.text.trim() : '' | |
| const confirmDevTerms = await consoleCLI.prompt.promptConfirm(`${termsText}\n\nYou have not accepted the Developer Terms of Service. Go to ${hyperlinker(DEV_TERMS_URL, DEV_TERMS_URL)} to view the terms. Do you accept the terms? (y/n):`) |
There was a problem hiding this comment.
🤖 PR Reviewer
The implementation is well-structured and the previous suggestion about terms.text sanitization has been partially addressed with trimEnd(). The hyperlinker dependency is properly added and the feature is consistently applied across all subcommands with good test coverage. One minor issue remains with the unsanitized terms.text content — trimEnd() was applied instead of the suggested trim(), which still allows leading whitespace/newlines from API-controlled content to appear in the terminal prompt.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| this.error('The Developer Terms of Service were declined') | ||
| } | ||
| const accepted = await consoleCLI.acceptDevTermsForOrg(orgId) | ||
| if (!accepted) { |
There was a problem hiding this comment.
[Re-raised] trimEnd() was applied (vs the suggested trim()), so leading whitespace or newlines in API-controlled terms.text could still produce unexpected terminal output. trim() would be safer here.
| if (!accepted) { | |
| const termsText = terms.text ? terms.text.trim() : '' |
There was a problem hiding this comment.
🤖 PR Reviewer
The implementation is clean and well-tested. The trimEnd() vs trim() issue from the previous review remains unaddressed — leading whitespace in API-controlled terms.text could produce unexpected terminal output. All other aspects of the change look solid: the guard is consistently applied across all subcommands, the skip-prompts path fails fast correctly, and the test coverage is comprehensive.
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| this.error('The Developer Terms of Service were declined') | ||
| } | ||
| const accepted = await consoleCLI.acceptDevTermsForOrg(orgId) | ||
| if (!accepted) { |
There was a problem hiding this comment.
[Re-raised] trimEnd() was applied (vs the suggested trim()), so leading whitespace or newlines in API-controlled terms.text could still produce unexpected terminal output. trim() would be safer here.
| if (!accepted) { | |
| const termsText = terms.text ? terms.text.trim() : '' |
There was a problem hiding this comment.
🤖 PR Reviewer
The implementation is clean, well-tested, and correctly gates all console subcommands behind the developer terms check. The re-raised trim() vs trimEnd() issue has been resolved — trim() is now used. One minor remaining issue: hyperlinker(DEV_TERMS_URL, DEV_TERMS_URL) passes the same value as both the text and URL, making the hyperlinker call redundant (it would render as plain text in terminals that don't support hyperlinks, and as a hyperlink with the URL as its label in terminals that do — either way, just using the URL directly would be equivalent and simpler). This is cosmetic and not a blocker.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
Fix looks fine. |
I kept the check explicit per command because the parent class ConsoleCommand does not always have enough context to safely run this check So if we force this check in the parent class, we risk adding extra auth/API calls to commands that do not need them thereby breaking org-selection flows or trigger prompts when the command doesn't know how to preserve the structured output |
3 participants
Description
This PR handles the Developer Terms of Service acceptance flow before Console commands call terms-gated Developer Console APIs.
When the selected org has not accepted the current Developer Terms, interactive Console commands now show a CLI-native prompt and can accept the terms through the existing Console SDK methods.
Structured-output commands using
--jsonor--ymlfail fast with an actionable error instead of prompting, so scripts and CI consumers do not receive invalid JSON/YAML output.Related Issue
#203
Motivation and Context
Users who had not accepted Developer Terms could see a raw 451 Console API error telling them to call the terms API manually. Hence the Console plugin needed to surface a helpful, user-facing flow for this specific terms-acceptance condition.
This mirrors the existing
aio-cli-plugin-appapproach for interactive usage while preserving machine-readable output for--jsonand--ymlcommands.How Has This Been Tested?
npm test ( 100% unit test coverage maintained)
Manual testing steps
aio plugins link .Screenshots (if appropriate):
Deeptys-MacBook-Pro-2:Downloads dthampy$ aio console project ls
Types of changes
Checklist: