[jek]

Related to https://github.com/shop/issues-develop/issues/23885

WHY are these changes introduced?

app security review is the only combined view of the deterministic and agent results: it applies check precedence, suppressions and result ages. Coding agents currently read it from terminal boxes and have to rejoin paths and commands that wrap across lines.

WHAT is this pull request doing?

Adds --json to app security review. The result has:

• valid and blocking: {level, blockedCheckCount}. valid is false exactly when --blocking fails; the result is still printed, and the command exits 1.
• resultsFiles.deterministic and resultsFiles.agent, each with its path, generatedAt, engine and scope, or null when the file is missing. scopeDiffers compares the two scopes.
• detection and coverage from the latest scan.
• checks, after --check-id. Each check has its metadata, each source's own result (bySource) and every finding, whatever --verbose says. Deterministic findings carry fix; agent findings carry confidence, reasoning and suppression.

How values appear:

• Paths are absolute and native.
• Stored enum values are kebab-case, for example not-applicable and structured-config.
• Times are whole-second UTC instants, or null when they can't be parsed.
• Optional text that's empty in the results files is null.
• Lines and columns are one-based, as the results files record them. json-output.md asks for zero-based positions, so this is still open: before the stack lands, either the guidance changes or the codec converts them, in one place.

New error codes:

• results-file-invalid, with details.files: [{path, errors}];
• unknown-check-id, with details.checkIds listing the unknown IDs.

Commits:

1. securityReview returns the reviewed results, and a presenter in security-review/result.ts renders them and sets the exit code. No behaviour change.
2. Add --json. The scope, scan directory and blocking definitions go in the shared module, for check.
3. Reject results files with values the CLI never writes, for example line: 0, a docs_url that isn't a URL, or a fractional version. review now reports such a hand-edited file as invalid, in both formats. Before, --json failed as an unexpected error, and the text view showed file:0. This commit has its own patch changeset. The JSON output is covered by Add typed JSON output to app security clean #8963's changeset.

How to manually test your changes?

Build once and run the local CLI directly, so nothing else reaches stdout:

pnpm nx build cli
alias shopify-dev="node $PWD/packages/cli/bin/dev.js"

shopify-dev app security check --path <app> --no-input --skip-instructions
shopify-dev app security review --path <app> --json
shopify-dev app security review --path <app> --json --blocking low; echo "exit $?"   # valid: false, exit 1 when there are findings
shopify-dev app security review --path <app> --json --check-id NOPE                  # unknown-check-id

To see results-file-invalid, set any "line" in .shopify/app-security/<results key>/deterministic-findings.json to 0 and run review again, with and without --json.

Checklist

• I've considered possible cross-platform impacts (Mac, Linux, Windows)
• I've considered possible documentation changes
• I've considered analytics changes to measure impact
• The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

[isaacroldan]

I'm just trusting both types.ts and result.ts are correct, will tophat also

[gonzaloriestra]

Could we assign unchanged enum values directly instead of keeping identity maps such as SEVERITIES, PRECEDENCES, DISPOSITIONS, CONFIDENCES and LANGUAGE_SUPPORT?

The typed projections already reject values that don't fit the public schema. We could keep maps for actual conversions, such as not_applicable to not-applicable.

[jek]

Done: SEVERITIES, PRECEDENCES, DISPOSITIONS, CONFIDENCES and LANGUAGE_SUPPORT are gone, and those values are assigned directly. The maps that convert spellings stay.