Optimize/split windows package signing (#26403)

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2026-06-08 12:12:50 +00:00 · 2025-11-10 11:05:12 -08:00
parent e7bf5621bf
commit 3596ffa909
27 changed files with 969 additions and 375 deletions
@@ -0,0 +1,78 @@
+# Cherry-Pick Commits Between Branches
+
+Cherry-pick recent commits from a source branch to a target branch without switching branches.
+
+## Instructions for Copilot
+
+1. **Confirm branches with the user**
+   - Ask the user to confirm the source and target branches
+   - If different branches are needed, update the configuration
+
+2. **Identify unique commits**
+   - Run: `git log <target-branch>..<source-branch> --oneline --reverse`
+   - **IMPORTANT**: The commit count may be misleading if branches diverged from different base commits
+   - Compare the LAST few commits from each branch to identify actual missing commits:
+     - `git log <source-branch> --oneline -10`
+     - `git log <target-branch> --oneline -10`
+   - Look for commits with the same message but different SHAs (rebased commits)
+   - Show the user ONLY the truly missing commits (usually just the most recent ones)
+
+3. **Confirm with user before proceeding**
+   - If the commit count seems unusually high (e.g., 400+), STOP and verify semantically
+   - Ask: "I found X commits to cherry-pick. Shall I proceed?"
+   - If there are many commits, warn that this may take time
+
+4. **Execute the cherry-pick**
+   - Ensure the target branch is checked out first
+   - Run: `git cherry-pick <sha1>` for single commits
+   - Or: `git cherry-pick <sha1> <sha2> <sha3>` for multiple commits
+   - Apply commits in chronological order (oldest first)
+
+5. **Handle any issues**
+   - If conflicts occur, pause and ask user for guidance
+   - If empty commits occur, automatically skip with `git cherry-pick --skip`
+
+6. **Verify and report results**
+   - Run: `git log <target-branch> -<number-of-commits> --oneline`
+   - Show the user the newly applied commits
+   - Confirm the branch is now ahead by X commits
+
+## Key Git Commands
+
+```bash
+# Find unique commits (may show full divergence if branches were rebased)
+git log <target>..<source> --oneline --reverse
+
+# Compare recent commits on each branch (more reliable for rebased branches)
+git log <source-branch> --oneline -10
+git log <target-branch> --oneline -10
+
+# Cherry-pick specific commits (when target is checked out)
+git cherry-pick <sha1>
+git cherry-pick <sha1> <sha2> <sha3>
+
+# Skip empty commits
+git cherry-pick --skip
+
+# Verify result
+git log <target-branch> -<count> --oneline
+```
+
+## Common Scenarios
+
+- **Empty commits**: Automatically skip with `git cherry-pick --skip`
+- **Conflicts**: Stop, show files with conflicts, ask user to resolve
+- **Many commits**: Warn user and confirm before proceeding
+- **Already applied**: These will result in empty commits that should be skipped
+- **Diverged branches**: If branches diverged (rebased), `git log` may show the entire history difference
+  - The actual missing commits are usually only the most recent ones
+  - Compare commit messages from recent history on both branches
+  - Cherry-pick only commits that are semantically missing
+
+## Workflow Style
+
+Use an interactive, step-by-step approach:
+- Show output from each command
+- Ask for confirmation before major actions
+- Provide clear status updates
+- Handle errors gracefully with user guidance
@@ -4,6 +4,7 @@ applyTo:
  - "tools/ci.psm1"
  - ".github/**/*.yml"
  - ".github/**/*.yaml"
+  - ".pipelines/**/*.yml"
 ---

 # Build Configuration Guide
@@ -121,7 +122,7 @@ The `Switch-PSNugetConfig` function in `build.psm1` manages NuGet package source
 - **Public**: Uses public feeds (nuget.org and public Azure DevOps feeds)
  - Required for: CI/CD environments, public builds, packaging
  - Does not require authentication
-  
+
 - **Private**: Uses internal PowerShell team feeds
  - Required for: Internal development with preview packages
  - Requires authentication credentials
@@ -0,0 +1,230 @@
+---
+applyTo: "**/*"
+---
+
+# Code Review Branch Strategy Guide
+
+This guide helps GitHub Copilot provide appropriate feedback when reviewing code changes, particularly distinguishing between issues that should be fixed in the current branch versus the default branch.
+
+## Purpose
+
+When reviewing pull requests, especially those targeting release branches, it's important to identify whether an issue should be fixed in:
+- **The current PR/branch** - Release-specific fixes or backports
+- **The default branch first** - General bugs that exist in the main codebase
+
+## Branch Types and Fix Strategy
+
+### Release Branches (e.g., `release/v7.5`, `release/v7.4`)
+
+**Purpose:** Contain release-specific changes and critical backports
+
+**Should contain:**
+- Release-specific configuration changes
+- Critical bug fixes that are backported from the default branch
+- Release packaging/versioning adjustments
+
+**Should NOT contain:**
+- New general bug fixes that haven't been fixed in the default branch
+- Refactoring or improvements that apply to the main codebase
+- Workarounds for issues that exist in the default branch
+
+### Default/Main Branch (e.g., `master`, `main`)
+
+**Purpose:** Primary development branch for all ongoing work
+
+**Should contain:**
+- All general bug fixes
+- New features and improvements
+- Refactoring and code quality improvements
+- Fixes that will later be backported to release branches
+
+## Identifying Issues That Belong in the Default Branch
+
+When reviewing a PR targeting a release branch, look for these indicators that suggest the fix should be in the default branch first:
+
+### 1. The Root Cause Exists in Default Branch
+
+If the underlying issue exists in the default branch's code, it should be fixed there first.
+
+**Example:**
+```yaml
+# PR changes this in release/v7.5:
+- $metadata = Get-Content "$repoRoot/tools/metadata.json" -Raw | ConvertFrom-Json
+ $metadata = Get-Content "$(Build.SourcesDirectory)/PowerShell/tools/metadata.json" -Raw | ConvertFrom-Json
+```
+
+**Analysis:** If `$repoRoot` is undefined because the template doesn't include its dependencies in BOTH the release branch AND the default branch, the fix should address the root cause in the default branch first.
+
+### 2. The Fix is a Workaround Rather Than a Proper Solution
+
+If the change introduces a workaround (hardcoded paths, special cases) rather than fixing the underlying design issue, it likely belongs in the default branch as a proper fix.
+
+**Example:**
+- Using hardcoded paths instead of fixing variable initialization
+- Adding special cases instead of fixing the logic
+- Duplicating code instead of fixing shared dependencies
+
+### 3. The Issue Affects General Functionality
+
+If the issue affects general functionality not specific to a release, it should be fixed in the default branch.
+
+**Example:**
+- Template dependencies that affect all pipelines
+- Shared utility functions
+- Common configuration issues
+
+## Providing Code Review Feedback
+
+### For Issues in the Current Branch
+
+When an issue is specific to the current branch or is a legitimate fix for the branch being targeted, **use the default code review feedback format** without any special branch-strategy commentary.
+
+### For Issues That Belong in the Default Branch
+
+1. **Provide the code review feedback**
+2. **Explain why it should be fixed in the default branch**
+3. **Provide an issue template** in markdown format
+
+**Example:**
+
+```markdown
+The `channelSelection.yml` template relies on `$repoRoot` being set by `SetVersionVariables.yml`, but doesn't declare this dependency. This issue exists in both the release branch and the default branch.
+
+**This should be fixed in the default branch first**, then backported if needed. The proper fix is to ensure template dependencies are correctly declared, rather than using hardcoded paths as a workaround.
+
+---
+
+**Suggested Issue for Default Branch:**
+
+### Issue Title
+`channelSelection.yml` template missing dependency on `SetVersionVariables.yml`
+
+### Description
+The `channelSelection.yml` template uses the `$repoRoot` variable but doesn't ensure it's set beforehand by including `SetVersionVariables.yml`.
+
+**Current State:**
+- `channelSelection.yml` expects `$repoRoot` to be available
+- Not all pipelines that use `channelSelection.yml` include `SetVersionVariables.yml` first
+- This creates an implicit dependency that's not enforced
+
+**Expected State:**
+Either:
+1. `channelSelection.yml` should include `SetVersionVariables.yml` as a dependency, OR
+2. `channelSelection.yml` should be refactored to not depend on `$repoRoot`, OR
+3. Pipelines using `channelSelection.yml` should explicitly include `SetVersionVariables.yml` first
+
+**Files Affected:**
+- `.pipelines/templates/channelSelection.yml`
+- `.pipelines/templates/package-create-msix.yml`
+- `.pipelines/templates/release-SetTagAndChangelog.yml`
+
+**Priority:** Medium
+**Labels:** `Issue-Bug`, `Area-Build`, `Area-Pipeline`
+```
+
+## Issue Template Format
+
+When creating an issue template for the default branch, use this structure:
+
+```markdown
+### Issue Title
+[Clear, concise description of the problem]
+
+### Description
+[Detailed explanation of the issue]
+
+**Current State:**
+- [What's happening now]
+- [Why it's problematic]
+
+**Expected State:**
+- [What should happen]
+- [Proposed solution(s)]
+
+**Files Affected:**
+- [List of files]
+
+**Priority:** [Low/Medium/High/Critical]
+**Labels:** [Suggested labels like `Issue-Bug`, `Area-*`]
+
+**Additional Context:**
+[Any additional information, links to related issues, etc.]
+```
+
+## Common Scenarios
+
+### Scenario 1: Template Dependency Issues
+
+**Indicators:**
+- Missing template includes
+- Undefined variables from other templates
+- Assumptions about pipeline execution order
+
+**Action:** Suggest fixing template dependencies in the default branch.
+
+### Scenario 2: Hardcoded Values
+
+**Indicators:**
+- Hardcoded paths replacing variables
+- Environment-specific values in shared code
+- Magic strings or numbers
+
+**Action:** Suggest proper variable/parameter usage in the default branch.
+
+### Scenario 3: Logic Errors
+
+**Indicators:**
+- Incorrect conditional logic
+- Missing error handling
+- Race conditions
+
+**Action:** Suggest fixing the logic in the default branch unless it's release-specific.
+
+### Scenario 4: Legitimate Release Branch Fixes
+
+**Indicators:**
+- Version-specific configuration
+- Release packaging changes
+- Backport of already-fixed default branch issue
+
+**Action:** Provide normal code review feedback for the current PR.
+
+## Best Practices
+
+1. **Always check if the issue exists in the default branch** before suggesting a release-branch-only fix
+2. **Prefer fixing root causes over workarounds**
+3. **Provide clear rationale** for why a fix belongs in the default branch
+4. **Include actionable issue templates** so users can easily create issues
+5. **Be helpful, not blocking** - provide the feedback even if you can't enforce where it's fixed
+
+## Examples of Good vs. Bad Approaches
+
+### ❌ Bad: Workaround in Release Branch Only
+
+```yaml
+# In release/v7.5 only
+- pwsh: |
+    $metadata = Get-Content "$(Build.SourcesDirectory)/PowerShell/tools/metadata.json" -Raw
+```
+
+**Why bad:** Hardcodes path to work around missing `$repoRoot`, doesn't fix the default branch.
+
+### ✅ Good: Fix in Default Branch, Then Backport
+
+```yaml
+# In default branch first
+- template: SetVersionVariables.yml@self  # Ensures $repoRoot is set
+- template: channelSelection.yml@self     # Now can use $repoRoot
+```
+
+**Why good:** Fixes the root cause by ensuring dependencies are declared, then backport to release if needed.
+
+## When in Doubt
+
+If you're unsure whether an issue should be fixed in the current branch or the default branch, ask yourself:
+
+1. Does this issue exist in the default branch?
+2. Is this a workaround or a proper fix?
+3. Will other branches/releases benefit from this fix?
+
+If the answer to any of these is "yes," suggest fixing it in the default branch first.
@@ -0,0 +1,83 @@
+---
+applyTo: ".pipelines/**/*.{yml,yaml}"
+---
+
+# OneBranch Restore Phase Pattern
+
+## Overview
+When steps need to run in the OneBranch restore phase (before the main build phase), the `ob_restore_phase` environment variable must be set in the `env:` block of **each individual step**.
+
+## Pattern
+
+### ✅ Correct (Working Pattern)
+```yaml
+parameters:
+- name: "ob_restore_phase"
+  type: boolean
+  default: true  # or false if you don't want restore phase
+
+steps:
+- powershell: |
+    # script content
+  displayName: 'Step Name'
+  env:
+    ob_restore_phase: ${{ parameters.ob_restore_phase }}
+```
+
+The key is to:
+1. Define `ob_restore_phase` as a **boolean** parameter
+2. Set `ob_restore_phase: ${{ parameters.ob_restore_phase }}` directly in each step's `env:` block
+3. Pass `true` to run in restore phase, `false` to run in normal build phase
+
+### ❌ Incorrect (Does Not Work)
+```yaml
+steps:
+- powershell: |
+    # script content
+  displayName: 'Step Name'
+  ${{ if eq(parameters.useRestorePhase, 'yes') }}:
+    env:
+      ob_restore_phase: true
+```
+
+Using conditionals at the same indentation level as `env:` causes only the first step to execute in restore phase.
+
+## Parameters
+
+Templates using this pattern should accept an `ob_restore_phase` boolean parameter:
+
+```yaml
+parameters:
+- name: "ob_restore_phase"
+  type: boolean
+  default: true  # Set to true to run in restore phase by default
+```
+
+## Reference Examples
+
+Working examples of this pattern can be found in:
+- `.pipelines/templates/insert-nuget-config-azfeed.yml` - Demonstrates the correct pattern
+- `.pipelines/templates/SetVersionVariables.yml` - Updated to use this pattern
+
+## Why This Matters
+
+The restore phase in OneBranch pipelines runs before signing and other build operations. Steps that need to:
+- Set environment variables for the entire build
+- Configure authentication
+- Prepare the repository structure
+
+Must run in the restore phase to be available when subsequent stages execute.
+
+## Common Use Cases
+
+- Setting `REPOROOT` variable
+- Configuring NuGet feeds with authentication
+- Setting version variables
+- Repository preparation and validation
+
+## Troubleshooting
+
+If only the first step in your template is running in restore phase:
+1. Check that `env:` block exists for **each step**
+2. Verify the conditional `${{ if ... }}:` is **inside** the `env:` block
+3. Confirm indentation is correct (conditional is indented under `env:`)
@@ -0,0 +1,195 @@
+---
+applyTo:
+  - ".pipelines/**/*.yml"
+  - ".pipelines/**/*.yaml"
+---
+
+# OneBranch Signing Configuration
+
+This guide explains how to configure OneBranch signing variables in Azure Pipeline jobs, particularly when signing is not required.
+
+## Purpose
+
+OneBranch pipelines include signing infrastructure by default. For build-only jobs where signing happens in a separate stage, you should disable signing setup to improve performance and avoid unnecessary overhead.
+
+## Disable Signing for Build-Only Jobs
+
+When a job does not perform signing (e.g., it only builds artifacts that will be signed in a later stage), disable both signing setup and code sign validation:
+
+```yaml
+variables:
+  - name: ob_signing_setup_enabled
+    value: false  # Disable signing setup - this is a build-only stage
+  - name: ob_sdl_codeSignValidation_enabled
+    value: false  # Skip signing validation in build-only stage
+```
+
+### Why Disable These Variables?
+
+**`ob_signing_setup_enabled: false`**
+- Prevents OneBranch from setting up the signing infrastructure
+- Reduces job startup time
+- Avoids unnecessary credential validation
+- Only disable when the job will NOT sign any artifacts
+
+**`ob_sdl_codeSignValidation_enabled: false`**
+- Skips validation that checks if files are properly signed
+- Appropriate for build stages where artifacts are unsigned
+- Must be enabled in signing/release stages to validate signatures
+
+## Common Patterns
+
+### Build-Only Job (No Signing)
+
+```yaml
+jobs:
+- job: build_artifacts
+  variables:
+    - name: ob_signing_setup_enabled
+      value: false
+    - name: ob_sdl_codeSignValidation_enabled
+      value: false
+  steps:
+    - checkout: self
+    - pwsh: |
+        # Build unsigned artifacts
+        Start-PSBuild
+```
+
+### Signing Job
+
+```yaml
+jobs:
+- job: sign_artifacts
+  variables:
+    - name: ob_signing_setup_enabled
+      value: true
+    - name: ob_sdl_codeSignValidation_enabled
+      value: true
+  steps:
+    - checkout: self
+      env:
+        ob_restore_phase: true  # Steps before first signing operation
+    - pwsh: |
+        # Prepare artifacts for signing
+      env:
+        ob_restore_phase: true  # Steps before first signing operation
+    - task: onebranch.pipeline.signing@1
+      displayName: 'Sign artifacts'
+      # Signing step runs in build phase (no ob_restore_phase)
+    - pwsh: |
+        # Post-signing validation
+      # Post-signing steps run in build phase (no ob_restore_phase)
+```
+
+## Restore Phase Usage with Signing
+
+**The restore phase (`ob_restore_phase: true`) should only be used in jobs that perform signing operations.** It separates preparation steps from the actual signing and build steps.
+
+### When to Use Restore Phase
+
+Use `ob_restore_phase: true` **only** in jobs where `ob_signing_setup_enabled: true`:
+
+```yaml
+jobs:
+- job: sign_artifacts
+  variables:
+    - name: ob_signing_setup_enabled
+      value: true  # Signing enabled
+  steps:
+    # Steps BEFORE first signing operation: use restore phase
+    - checkout: self
+      env:
+        ob_restore_phase: true
+    - template: prepare-for-signing.yml
+      parameters:
+        ob_restore_phase: true
+
+    # SIGNING STEP: runs in build phase (no ob_restore_phase)
+    - task: onebranch.pipeline.signing@1
+      displayName: 'Sign artifacts'
+
+    # Steps AFTER signing: run in build phase (no ob_restore_phase)
+    - pwsh: |
+        # Validation or packaging
+```
+
+### When NOT to Use Restore Phase
+
+**Do not use restore phase in build-only jobs** where `ob_signing_setup_enabled: false`:
+
+```yaml
+jobs:
+- job: build_artifacts
+  variables:
+    - name: ob_signing_setup_enabled
+      value: false  # No signing
+    - name: ob_sdl_codeSignValidation_enabled
+      value: false
+  steps:
+    - checkout: self
+      # NO ob_restore_phase - not needed without signing
+    - pwsh: |
+        Start-PSBuild
+```
+
+**Why?** The restore phase is part of OneBranch's signing infrastructure. Using it without signing enabled adds unnecessary overhead without benefit.
+
+## Related Variables
+
+Other OneBranch signing-related variables:
+
+- `ob_sdl_binskim_enabled`: Controls BinSkim security analysis (can be false in build-only, true in signing stages)
+
+## Best Practices
+
+1. **Separate build and signing stages**: Build artifacts in one job, sign in another
+2. **Disable signing in build stages**: Improves performance and clarifies intent
+3. **Only use restore phase with signing**: The restore phase should only be used in jobs where signing is enabled (`ob_signing_setup_enabled: true`)
+4. **Restore phase before first signing step**: All steps before the first signing operation should use `ob_restore_phase: true`
+5. **Always validate after signing**: Enable validation in signing stages to catch issues
+6. **Document the reason**: Add comments explaining why signing is disabled or why restore phase is used
+
+## Example: Split Build and Sign Pipeline
+
+```yaml
+stages:
+  - stage: Build
+    jobs:
+    - job: build_windows
+      variables:
+        - name: ob_signing_setup_enabled
+          value: false  # Build-only, no signing
+        - name: ob_sdl_codeSignValidation_enabled
+          value: false  # Artifacts are unsigned
+      steps:
+        - template: templates/build-unsigned.yml
+
+  - stage: Sign
+    dependsOn: Build
+    jobs:
+    - job: sign_windows
+      variables:
+        - name: ob_signing_setup_enabled
+          value: true   # Enable signing infrastructure
+        - name: ob_sdl_codeSignValidation_enabled
+          value: true   # Validate signatures
+      steps:
+        - template: templates/sign-artifacts.yml
+```
+
+## Troubleshooting
+
+**Job fails with signing-related errors but signing is disabled:**
+- Verify `ob_signing_setup_enabled: false` is set in variables
+- Check that no template is overriding the setting
+- Ensure `ob_sdl_codeSignValidation_enabled: false` is also set
+
+**Signed artifacts fail validation:**
+- Confirm `ob_sdl_codeSignValidation_enabled: true` in signing job
+- Verify signing actually occurred
+- Check certificate configuration
+
+## Reference
+
+- PowerShell signing templates: `.pipelines/templates/packaging/windows/sign.yml`