Repository navigation
Conversation
Look up the --client-id value in the user's account, as deploy does, before gathering, scanning, writing or reading results, and before the no-app-configuration prompt. The TOML's client_id and the picker's client ID aren't looked up. clean and instructions don't opt in, so results saved under a mistyped client ID can still be removed.
github-actions
Bot
added
the
no-changelog
--client-id= passed an empty string, which the truthiness check skipped, so check --list-files listed files without the lookup. The value was passed, so it's looked up like any other.
The shared missing-app error suggests passing --reset, which the app security commands don't take. appFromIdentifiers takes offerReset, true by default, and the app security lookup passes false; the account and login steps stay.
check --json printed its scan and --list-files results without a schema,
and instructions had no --json at all. Both are finite commands, so they
now declare a jsonOutputSchema and encode through it.
- check: the schema accepts either the scan result or the --list-files
result; the two shapes share no key.
- instructions: adds --json, returning
{instructions: {content, copied_to_clipboard, path}}. content is always
included, also when the instructions are copied or written.
- check: the scan result gains an instructions field with the same
AppSecurityInstructions definition, or null when none were chosen.
- check: --json no longer disables prompts; only a non-interactive
terminal or --list-files does. In JSON mode the instructions prompt
runs before the result is printed, and the chosen instructions go in
the result instead of stdout.
Renames security-json.ts to security-check-json.ts and toSecurityJson to
toSecurityCheckJson to match the other app security commands.
It repeated deterministic_findings.engine exactly: the scan result holds nothing from the agent side, so the copy carried no information. This matches review --json, where each source carries only its own engine.
Knip flagged the result type aliases, which no caller needs, and the AppSecurityEngineMetadata re-export, unused since check --json dropped its top-level engine. The scan directory type pin is now asserted in a test, as the review JSON pins are.
b29e828 to
c5855ac
Compare
…m off With --json, check can ask which app configuration to scan, offer to scan without one and then ask for the app, not only offer the instructions. The help now says so, and points automation at --no-input: a choice the command would have asked for then becomes an error, and a --client-id that needs a login fails instead of opening the browser. --list-files no longer claims it never prompts: its --client-id lookup can require a login.
c5855ac to
e2bb489
Compare
…JSON mode Delivering the instructions is presentation: it owns stdout, the clipboard and the --write file. deliverAppSecurityInstructions moves to app-security-instructions-output.ts, next to the other app security presenters, and appSecurityInstructions stays the data-only builder. In JSON mode the copy and write confirmation banners are no longer shown: the result's copied_to_clipboard and path already say where the instructions went, and docs/cli/json-output.md keeps human-only banners in the text presenter. A command-boundary test runs check and then instructions --json --write with the real delivery.
With --json, the warning about a scan directory that Git ignores and the "To skip these prompts next time" command were rendered as banners. They're now outputWarn and outputInfo messages, which CLI Kit emits as diagnostic events on stderr in JSON mode, so they keep their information without human-only banners. Text mode is unchanged.
gonzaloriestra
left a comment
gonzaloriestra
left a comment
There was a problem hiding this comment.
Thanks for adding this! And sorry for not warning before that this is now required for new public commands 🙏
I left some comments. The main issue is following the JSON conventions to be consistent with other outputs and avoid a breaking change in the future.
| ...appSecurityBlockingFlag, | ||
| yes: Flags.boolean({ | ||
| description: 'Print coding-agent instructions without prompting.', | ||
| description: "Print coding-agent instructions without prompting. With --json, they're in the result instead.", |
There was a problem hiding this comment.
I don't think we need to explain that (same for other commands)
|
|
||
| const scanResultSchema = zod.object({ | ||
| selection: zod.object({ | ||
| app_directory: zod.string(), |
There was a problem hiding this comment.
Can we align the CLI-owned wrapper with docs/cli/json-output.md: camelCase fields, directory for the project directory, and kebab-case multiword enum values? The native security document remains unchanged inside deterministicFindings; that exception doesn't extend to the wrapper.
Suggested mapping:
| Current | Recommended |
|---|---|
selection.app_directory |
selection.directory |
selection.app_config_file |
selection.configPath |
selection.client_id |
selection.clientId |
selection.client_id_source |
selection.clientIdSource |
selection.scan_directories |
selection.scanDirectories |
deterministic_findings |
deterministicFindings |
agent_checks_path |
agentChecksPath |
instructions.copied_to_clipboard |
instructions.copiedToClipboard |
origin app_directory / include_dir |
app-directory / include-dir |
configPath is a naming recommendation. The guide requires a descriptive ...Path field but doesn't prescribe that exact name. Please update the codecs, schemas, tests, and examples together.
| import type {AppSecurityExecution} from './app-security-api.js' | ||
| import type {AppSecurityScanDirectory, AppSecuritySelection} from './app-security-selection.js' | ||
|
|
||
| const scanDirectorySchema = zod.object({ |
There was a problem hiding this comment.
Can we add .strict() to every CLI-owned object, including selection, scan-directory entries, instructions, and the scan, file-list, and instructions result wrappers? The guide requires explicit projections before validation. Scan directories currently pass through unchanged, so please map their public fields explicitly and test rejection of unknown keys.
The versioned native security-document schemas should remain unchanged.
| agentCheckCount: number, | ||
| ): Promise<AppSecurityInstructionsDestination> { | ||
| if (options.json || options.skipInstructions) return 'nothing' | ||
| if (options.skipInstructions) return 'nothing' |
There was a problem hiding this comment.
Keeping interactivity independent from --json is correct, but these prompts still use Ink's default stdout. Rendering them before the result doesn't leave stdout as one JSON document. Can we route prompt UI to stderr in JSON mode, including configuration and app-selection prompts? Please test the real renderer with separate stdout/stderr capture; mocked prompt callbacks don't verify stream routing.
| warnAboutIgnoredScanDirectories(ignoredScanDirectories, options.json, dependencies) | ||
| if (options.json) { | ||
| dependencies.output(JSON.stringify({files: paths}, null, 2)) | ||
| dependencies.output(securityCheckJsonOutputSchema.encode({files: paths})) |
There was a problem hiding this comment.
--list-files --json currently returns paths relative to the app directory. The guide requires absolute native filesystem paths; these are gathered files, not relative route names. Can the JSON codec resolve them against the app directory, while text output keeps its relative paths? Please document that format in the schema.
| origin: zod.enum(['app_directory', 'include_dir']), | ||
| }) | ||
|
|
||
| const scanResultSchema = zod.object({ |
There was a problem hiding this comment.
The guide requires a completed validation result to expose valid and consistent issues. This result currently exposes only the native findings document and associated metadata. Can we add the CLI validation projection, define how valid relates to the blocking policy, and test its result and exit behavior? Keep the versioned security document unchanged.
| // Resolved first so a mistyped directory fails before any prompt. | ||
| const includeDirectories = await resolveIncludeDirectories(options.includeDirs) | ||
| const canPrompt = !options.json && !options.listFiles && dependencies.canPrompt() | ||
| const canPrompt = !options.listFiles && dependencies.canPrompt() |
There was a problem hiding this comment.
Enabling this flow with --json also exposes an existing cancellation mismatch: declining "Scan without app configuration?" in app-security-selection.ts currently throws an AbortError. The guide treats declined confirmation as a cancelled/skipped outcome with exit zero. Can this return a typed cancellation, encoded as one result object with status: "cancelled"? Missing required input under --no-input should remain an error.
This behavior predates this PR, but the guide explicitly requires schema-adoption work to align existing behavior too.
| const instructions = await deliverChosenInstructions() | ||
| dependencies.output( | ||
| encodeSecurityJson(toSecurityJson(execution, artifacts.agentChecksPath, selection, scanDirectories)), | ||
| securityCheckJsonOutputSchema.encode( |
There was a problem hiding this comment.
The execution service still encodes and prints JSON, renders the text report, and selects format-specific diagnostic helpers. Can it return typed result data and leave final encoding/presentation to the command or presenter?
Also, please move the shared delivery result type out of the output presenter so security-instructions-json.ts doesn't depend on presentation-owned types.
jplhomer
left a comment
jplhomer
left a comment
There was a problem hiding this comment.
LGTM once Gonzalo's feedback is addressed!
The result schema in help already says where the instructions and the --list-files paths go, so the --yes description, the check and instructions descriptions no longer repeat it. The check help also no longer says that --json keeps prompting, which every command does. It still lists the prompts check can show, since that's what the --no-input advice depends on, and the sentence about running instructions later is back next to the instructions prompt it follows.
docs/cli/json-output.md asks for camelCase in CLI-owned fields, `directory` for the project directory and kebab-case multiword enum values, and keeps original keys only inside native payloads. The selection, agent checks path and instructions now follow it, and the selected configuration file is `configPath`. deterministicFindings is the deterministic findings document unchanged, so its coverage.scan_directories still spells origins app_directory and include_dir. Its schema now says it keeps its own field conventions and schema_version, as the guide asks for each native format. The internal scan directory origins keep the document's spelling, and the JSON mapping converts them, which replaces the type pin that only held while scan directories passed through unchanged.
The instructions result type lived in the output presenter, so the JSON contract in security-instructions-json.ts depended on presentation. The presenter also returned the result, although everything in it was known before delivery. Now the commands build the instructions and deliver them as requested: deliverAppSecurityInstructions only copies them or writes the file, and renderAppSecurityInstructions only shows the confirmation or prints them in text mode. The result is built from the same request once the delivery succeeds, so a failed copy or write prints no result that says it happened, and the presenter no longer needs to know about JSON mode.
The published JSON Schema already says additionalProperties: false for every object in these results, but Zod's default stripped an unknown key instead of rejecting it. A field added to the selection, a scan directory or the instructions would then vanish from --json without an error. Every object these commands define is now strict, as docs/cli/json-output.md asks. The deterministic findings document keeps its own schema.
The gathered files were relative to the app directory in JSON as in text, but docs/cli/json-output.md asks for absolute native filesystem paths, as the rest of the check result already uses. The JSON result now resolves each file against the app directory, including files in an --include-dir above it, and its schema says the paths are absolute. The text output keeps its relative paths.
check --list-files --json now prints absolute paths, and this layout test still expected the relative ones.
…mand The check service encoded and printed the JSON result, rendered the text report, offered the coding-agent instructions and chose between banners and diagnostic events, so its results couldn't be used without its terminal output. docs/cli/json-output.md asks the domain service to return typed data and leave presentation to the command. The command now resolves the selection, shows how to skip the prompts when it prompted, and runs the scan, which returns the scan or the gathered files. renderSecurityCheckResult in security-output.ts presents that result: the report or the JSON document, the instructions prompt and delivery, the Git ignore notices and the blocking exit code. The command decides once whether the selection and the instructions can prompt. The presenter tests run the real encoder and output writers and capture stdout and stderr separately, so they check that JSON mode prints one document on stdout and only side events on stderr.
Answering no to "Scan it without app configuration?" aborted with the
same error as finding no configuration without a prompt, so check
exited 1 and, with --json, printed an error document.
docs/cli/json-output.md treats a declined confirmation as a cancelled
outcome that exits zero, with one result object.
Declining now throws cli-kit's CancelExecution, which
resolveSecurityCheckSelection returns as a cancelled resolution. The
command presents it without scanning: nothing in text mode, and
{"status": "cancelled"} with --json. Every check result now has a
status, success for a scan and for --list-files, so a prompt added to
--list-files later wouldn't change its contract. Without prompts, a
missing app configuration is still an error.
|
Deferring this work post 4.9.0 |
3 participants
WHY are these changes introduced?
Fixes https://github.com/shop/issues-develop/issues/23885
app security check --jsonprinted its results without ajsonOutputSchema, andapp security instructionshad no--jsonat all.docs/cli/json-output.mdrequires both from finite commands; the lint rule didn't catch them only because it skips hidden commands.check --jsonalso turned off every prompt, which the same doc says JSON output must not do.WHAT is this pull request doing?
check: declaresAppSecurityCheckResult, which accepts either the scan result or the--list-filesresult ({files}). The two shapes share no key, so consumers tell them apart by the flag they passed.instructions: adds--json, returning{instructions: {content, copied_to_clipboard, path}}.contentis always included, also with--copyor--write.checkscan result: gains aninstructionsfield using the sameAppSecurityInstructionsdefinition, ornullwhen none were chosen. Drops the top-levelengine, which repeateddeterministic_findings.engineexactly, matchingreview --json.checkprompts:--jsonno longer disables them; only a non-interactive terminal or--list-filesdoes. In JSON mode the instructions prompt (or--yes) runs before the result is printed, and the chosen instructions go in the result rather than stdout, so stdout stays one JSON document.docs/cli/json-output.md, delivering the instructions is a presenter (app-security-instructions-output.ts), and the copy and write confirmation banners aren't shown in JSON mode.check's notices (a scan directory that Git ignores, the command that skips the prompts) are diagnostic events on stderr in JSON mode instead of banners.checkhelp: says which prompts--jsoncan show (the configuration picker, the offer to scan without one and the app picker after it, and the instructions), and points automation at--no-input, which turns a choice the command would have asked for into an error.--list-filesno longer claims it never prompts, since Validate --client-id in App Security check, record and review #8764's--client-idlookup can require a login (review comment).Stacked on #8764.
No changeset: the commands are hidden and unreleased.
Before (
check --json --yes):{engine, selection, deterministic_findings, agent_checks_path}, instructions not delivered.After:
{ "selection": {…}, "deterministic_findings": {…}, "agent_checks_path": "…/agent-checks.json", "instructions": {"content": "…", "copied_to_clipboard": false, "path": null} }How to manually test your changes?
Create an app to scan:
From this checkout, with
CLI="pnpm --dir <path to this checkout> shopify app security":$CLI check --path /tmp/sec-json --list-files --jsonprints{"files": [...]}.$CLI check --path /tmp/sec-json --json --yes | jq 'keys, .instructions.copied_to_clipboard'showsagent_checks_path,deterministic_findings,instructions,selectionandfalse..instructions.contentsays to use the existing scan results.$CLI check --path /tmp/sec-json --jsonin an interactive terminal, without piping stdout, shows the instructions prompt before any output. Choose "Copy": the result has"copied_to_clipboard": trueand the clipboard holds the same text. Piped, redirected or with--no-input, there's no prompt andinstructionsisnull.$CLI instructions --path /tmp/sec-json --json --write /tmp/sec-json/handoff.md | jq .instructions.pathprints the file's path, the file holdscontent, and no banner is shown.$CLI check --json-schemaand$CLI instructions --json-schemaprint the result schemas.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add