Skip to content

Prevent app security rescans from dropping compiled findings - #8594

Open
jek wants to merge 2 commits into
mainfrom
app-doctor/findings-merge
Open

jek wants to merge 2 commits into
mainfrom
app-doctor/findings-merge

Conversation

@jek

@jek jek commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

Fixes #23971

If agentic findings exist, don't silently remove or invalidate them when running app security check. Adds app security check --clean to start a fresh trace.

WHAT is this pull request doing?

  • Adds app security check --clean to start a fresh trace & clean up findings files
  • Updates agent instructions to clarify workflow & not invalidate their own work with follow-up checks
image image

How to manually test your changes?

$ app security check
$ app security instructions
$ app security check --findings [findings.json]
$ app security check
$ app security check --clean

A valid findings.json is needed to fully tophat this

Checklist

  • [ x ] I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • [ x ] 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

Assisted-By: devx/a43d5d42-941f-47ca-ba7d-47133d5a8cc6

@github-actions github-actions Bot added the Area: @shopify/app @shopify/app package issues label Sep 18, 2026
@jek
jek force-pushed the app-doctor/findings-merge branch from 1463dad to 25d5c2d Compare September 18, 2026 19:58
@jek
jek changed the base branch from main to joshlarson/rename-app-doctor-to-app-security September 21, 2026 13:28
@jek
jek force-pushed the app-doctor/findings-merge branch from 25d5c2d to 4034747 Compare September 21, 2026 13:30
@jek jek changed the title Prevent App Doctor rescans from dropping compiled findings Prevent app security rescans from dropping compiled findings Sep 21, 2026
@jek
jek added this pull request to stack #8610 September 21, 2026 13:32
@jek
jek marked this pull request as ready for review September 21, 2026 13:51
@jek
jek requested review from a team as code owners September 21, 2026 13:51
Base automatically changed from joshlarson/rename-app-doctor-to-app-security to main September 21, 2026 15:58
Refuse to start a new scan when agent findings or compiled review results
already exist, preventing the deterministic check from replacing that work.

Add --clean for explicitly discarding the current review and starting over,
and update the generated agent instructions to describe the guarded flow.

Co-authored-by: AI <noreply@pi.dev>
Assisted-By: devx/a43d5d42-941f-47ca-ba7d-47133d5a8cc6
@jek
jek force-pushed the app-doctor/findings-merge branch from 4034747 to 069ab34 Compare September 21, 2026 15:58

@dmerand dmerand left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me + LGTM overall. I have a few agentic findings worth looking at/resolving before we ship.

Comment thread packages/app/src/cli/services/security-check.ts
Comment thread packages/app/src/cli/services/security-check.ts
Comment thread packages/app/src/cli/services/app-security-artifacts.ts
Comment thread packages/app/src/cli/commands/app/security/check.ts Outdated
- Require explicit --clean; ignore inherited environment values
- Reject clean compilation before writing artifacts
- Add command-boundary tests for compiled-trace and findings protection

Co-authored-by: AI <noreply@pi.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: @shopify/app @shopify/app package issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants