Repository navigation
Add --print-instructions to app security check - #8878
Conversation
Co-authored-by: Qlaw <[email protected]>
github-actions
Bot
added
shopify.dev preview
Area: @shopify/cli
Co-authored-by: Qlaw <[email protected]>
|
/snapit |
|
🫰✨ Thanks @nickwesselman! Your snapshot has been published to npm. Built from Test the snapshot by installing your package globally: pnpm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]Caution After installing, validate the version by running |
| test.each([{flags: ['--print-instructions']}, {flags: ['--yes']}, {flags: ['--print-instructions', '--yes']}])( | ||
| 'rejects $flags with --skip-instructions', | ||
| async ({flags}) => { | ||
| await expect(SecurityCheck.run([...flags, '--skip-instructions'], import.meta.url)).rejects.toThrow() |
There was a problem hiding this comment.
Optional: this assertion passes for any error, not only the flag conflict. In these tests, every parse error becomes process.exit unexpectedly called with "1". I checked: --not-a-real-flag --skip-instructions throws the same error. So if a flag in this test is renamed later, the test still passes. Lines 219 and 278 have the same problem.
Please check the rendered message, as the test at line 243 does:
const outputMock = mockAndCaptureOutput()
// ...
await expect(SecurityCheck.run([...flags, '--skip-instructions'], import.meta.url)).rejects.toThrow(
'process.exit unexpectedly called with "1"',
)
expect(outputMock.error()).toContain('cannot also be provided when using')The error box wraps long lines, so match a short part of the message.
There was a problem hiding this comment.
Updated the conflict tests to check the rendered cannot also be provided message, rather than only asserting that something throws. This includes the CLI, true/false environment, and list-files cases. Verified with an unknown-flag mutation: it now fails the conflict-message assertion.
| ['SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS', '--list-files'], | ||
| ['SHOPIFY_FLAG_YES', '--skip-instructions'], | ||
| ['SHOPIFY_FLAG_YES', '--list-files'], | ||
| ])('rejects %s with %s', async (environment, incompatible) => { |
There was a problem hiding this comment.
Optional: these tests set a printing variable and pass a mode flag. Some combinations are not tested:
- A mode from the environment with a printing flag. For example,
SHOPIFY_FLAG_LIST_FILES=1with--print-instructions, orSHOPIFY_FLAG_APP_SECURITY_SKIP_INSTRUCTIONS=1with--yes. - Both printing variables set.
- A printing flag with the other printing variable set to
0. For example,--print-instructionswithSHOPIFY_FLAG_YES=0.
I expect these to work, because oclif checks the exclusions of every flag that has a value. But each exclusion is declared on one flag only. For example, yes lists list-files, but list-files does not list yes. Tests for these cases would catch a future edit that removes one side.
There was a problem hiding this comment.
Added 12 cases covering both printing variables across all 0/1 combinations, a printing CLI flag with the other printing variable set to 0 or 1, and environment-selected list-files/skip-instructions modes with either printing CLI flag. All 143 focused tests, lint, and builds passed. Warning behavior is unchanged.
| ['SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS', '1', true, false], | ||
| ['SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS', '0', false, false], | ||
| ['SHOPIFY_FLAG_YES', '1', true, true], | ||
| ['SHOPIFY_FLAG_YES', '0', false, true], |
There was a problem hiding this comment.
Optional question: with SHOPIFY_FLAG_YES=0, the command does not print the instructions, but it still shows this warning:
Warning: The "yes" flag has been deprecated. Use "--print-instructions" instead.
I saw this with a fresh build of this head. oclif warns for every deprecated flag that has a value, and 0 is a value. This row expects the warning, so it can be intentional. But a user who sets SHOPIFY_FLAG_YES=0 does not want printing. For them, the correct change is to remove the variable, not to use --print-instructions. Consider warning only when the value turns printing on. If the current behavior is intended, ignore this.
Co-authored-by: Qlaw <[email protected]>
WHY are these changes introduced?
Review feedback noted that
--yessounds like a general confirmation bypass, but it only selects printed coding-agent instructions. It does not answer app-selection prompts or choose the interactive default of copying.WHAT is this pull request doing?
--print-instructionsandSHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONSas the explicit instruction-output option.--yesandSHOPIFY_FLAG_YESworking as a hidden, deprecated compatibility path. Normal invocations do not warn.--skip-instructionsand--list-files;--no-inputstill only controls interactivity. The internal option is now namedprintInstructions.How to manually test your changes?
pnpm shopify app security check --help. It advertises--print-instructions, not--yes, and has no empty usage alternative.pnpm shopify app security check --path <app> --no-input --print-instructions. Instructions print after the scan without a delivery prompt.--yesinstead. It still prints instructions and warns to use--print-instructions.SHOPIFY_FLAG_YES=1remains supported too.--skip-instructionsor--list-filesis rejected.Validation
pnpm refresh-manifestsandpnpm build-dev-docs; verified the rendered public help and generated reference use the canonical flag.git diff --checkpassed.An existing missing-config path assertion fails on both untouched main and this change in a shallow checkout. All focused tests passed from a normal-depth checkout; no unrelated test fix is included.
Checklist
PR authored by Qlaw