Skip to content

Label integration→main PR when snapshot auto-recovery updates snapshots#606

Merged
vscheuber merged 4 commits into
mainfrom
copilot/add-snapshot-review-label
Apr 21, 2026
Merged

Label integration→main PR when snapshot auto-recovery updates snapshots#606
vscheuber merged 4 commits into
mainfrom
copilot/add-snapshot-review-label

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 20, 2026

Adds targeted labeling for integration PRs that include snapshot auto-recovery changes. The workflow now applies integration-needs-snapshot-review only when snapshot files were actually updated, without changing PR selection or any other batching behavior.

  • Workflow update (.github/workflows/integration-batch.yml)

    • Captures the generated integration -> main PR number from the create/update step.
    • Adds a follow-up labeling step that:
      • reads steps.merge.outputs.snapshot_updates
      • checks snapshot_updates.files length
      • adds integration-needs-snapshot-review only when length > 0
    • Does not remove labels in any path.
  • Scope guardrails preserved

    • Label is applied only to the generated integration PR.
    • Original batched PRs are untouched.
    • Existing PR selection, merge flow, and compact JSON outputs remain unchanged.
  • Docs update (docs/INTEGRATION.md)

    • Documents integration-needs-snapshot-review in the integration workflow label list.
- name: Label integration PR for snapshot review
  run: |
    snapshot_updates='${{ steps.merge.outputs.snapshot_updates }}'
    integration_pr_number='${{ steps.integration_pr.outputs.number }}'
    snapshot_file_count="$(echo "$snapshot_updates" | jq -r '(.files // []) | length' 2>/dev/null || echo 0)"

    if [ "$snapshot_file_count" -gt 0 ]; then
      gh pr edit "$integration_pr_number" --repo "$REPO" --add-label integration-needs-snapshot-review
    fi
Original prompt

Create a PR in rockcarver/frodo-cli to label the generated integration PR when snapshot auto-recovery occurred.

Decision:

  • Only ADD the label when snapshots were updated; do NOT remove it automatically.

Requirements:

  1. Label name: integration-needs-snapshot-review.
  2. Apply ONLY to the generated integration PR (integration -> main), not to original batched PRs.
  3. Update .github/workflows/integration-batch.yml so that after the integration PR is created/updated, if the batch summary indicates snapshot files were updated (non-empty list), add the label to the integration PR.
  4. If no snapshots were updated, do not add the label.
  5. Do not remove the label in any case.
  6. Keep PR selection logic and all other workflow behavior unchanged.
  7. Ensure JSON outputs remain compact where required.

Deliverable: Open a PR implementing this label behavior.

The following is the prior conversation context from the user's chat exploration (may be truncated):

User: OK, let's keep the current PR selection in place. Only green PRs are selected. But let's add the full test run (except for dry-run, then don't run the tests as they run for a long time).

Assistant: Yes—this makes sense and is low-risk: keep selection strict (only green PRs), and add a post-merge full test gate for real (non-dry-run) batching so you know integration is actually ready to PR into main.

I can’t open the PR from here in this turn, but here’s the concrete change I’d make (so you can implement quickly, and I’ll package it as a PR when I have write automation again).

What to add to integration-batch.yml

1) Add a “Full test on integration (non-dry-run only)” step at the end

Place it after:

  • merging selected PRs
  • any snapshot auto-updates
  • any lockfile regeneration
  • pushing integration (if the workflow does that before summaries/labels)

Gate it with the workflow’s dry-run flag, e.g.:

  • if: inputs.dry-run != 'true' (or whatever your workflow uses)

2) Ensure frodo is available (same approach you validated)

Because your tests shell out to frodo, ensure the job does:

npm ci
npm run build:only
npm i -g
echo "$(npm bin -g)" >> "$GITHUB_PATH"
npm test

Notes:

  • In your package.json, npm test maps to npm run test:only, so npm test is fine and keeps it conventional.
  • This will run the full suite once and fail the workflow if integration is not clean—exactly what you want.

3) (Optional but helpful) Step summary

Append a short section to $GITHUB_STEP_SUMMARY, e.g.:

  • “Integration full test suite: PASSED” or “FAILED (see logs)”
    That makes it obvious to maintainers scanning the run.

Why this is the right tradeoff

  • Dry-run remains fast (no full test suite).
  • Real runs become “merge + prove” rather than “merge and hope”.
  • You don’t need to weaken PR selection or add brittle log heuristics.

Two small clarifying questions (so the final PR is perfect)

  1. In your workflow UI, is the input named dry-run, dry_run, or something else? (Just the exact key.)
  2. Do you want the full test run to happen before pushing integration (so a failing batch doesn’t update the branch), or is it acceptable to push and let the run fail (so you can inspect the integration branch state)?

If you answer those, I’ll give you the exact YAML snippet tailored to your workflow’s current structure.

User: @copilot Accepted Confirmation: Confirm agent session

Assistant: I’ll add that full integration-branch test gate while keeping PR selection strict (only green PRs), and I’ll make sure it’s skipped for dry-runs.

I’m not able to push the change from this message, but the exact design is:

Workflow behavior change

After the batch merge work finishes (including any snapshot auto-updates and lockfile regeneration), add a final step:

  • If dry-run: skip (no long-running tests)
  • If not dry-run: run the full test suite on the resulting integration branch state and fail the workflow if it’s not green

What the step should run (non-dry-run only)

Because your tests shell out to frodo, the step should ensure the CLI is available first (same approach that already worked for snapshot updates):

  1. npm ci
  2. npm run build:only
  3. npm i -g
  4. Ensure global npm bin is on PATH for later steps
  5. npm test (which in this repo maps to the full Jest run)

Why this is the right approach

  • You keep the high-signal selection rule (only PRs that are already green).
  • You still protect main by ensuring integration is green as a combined set before an integration PR is opened/merged.
  • Dry-runs stay fast.

One decision to make (so I implement it exactly as you want)

Do you want the workflow to:

  1. Run full tests before pushing/updating the integration branch (so a failing batch never updates the branch), or
  2. Push integration first, then run full tests (so you can inspect the exact integration state even if tests fail)?

Tell me which you prefer, and I’ll align the cha...

This pull request was created from Copilot chat.

Copilot AI changed the title [WIP] Add label for integration PR on snapshot auto-recovery Label integration→main PR when snapshot auto-recovery updates snapshots Apr 20, 2026
Copilot AI requested a review from vscheuber April 20, 2026 12:43
@vscheuber vscheuber requested a review from Copilot April 21, 2026 01:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds targeted labeling to the integration-batch automation so the generated integration -> main PR is labeled for snapshot review only when snapshot auto-recovery actually updated snapshot files.

Changes:

  • Capture the generated integration -> main PR number as a step output in integration-batch.yml.
  • Add a follow-up workflow step that conditionally applies integration-needs-snapshot-review when steps.merge.outputs.snapshot_updates.files is non-empty.
  • Document the new label in docs/INTEGRATION.md.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
.github/workflows/integration-batch.yml Emits the integration PR number and conditionally applies the snapshot-review label based on snapshot_updates.files.
docs/INTEGRATION.md Documents the new integration-needs-snapshot-review label and when it’s applied.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/integration-batch.yml Outdated
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@vscheuber vscheuber marked this pull request as ready for review April 21, 2026 02:03
@vscheuber vscheuber requested a review from Copilot April 21, 2026 02:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vscheuber vscheuber merged commit 955d7e2 into main Apr 21, 2026
4 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants