Skip to content

Separate telemetry hook maintenance from skills bundles - #20283

Closed
Ella Hathaway (ellahathaway) wants to merge 13 commits into
mainfrom
ellahathaway-hook-only-maintenance
Closed

Ella Hathaway (ellahathaway) wants to merge 13 commits into
mainfrom
ellahathaway-hook-only-maintenance

Conversation

@ellahathaway

@ellahathaway Ella Hathaway (ellahathaway) commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Description

Important

Superseded by #20947, which follows native replacement #20946 in the recovered open-draft chain #20932 → #20946 → #20947. This original draft is being closed unmerged to remove the misleading duplicate retirement review. Its published branch and original stack membership are preserved.

This original PR's branch contains the client-selection, native-plugin, and retirement chain while targeting main. Pushing that branch caused GitHub to auto-mark original native PR #20287 as merged because its old base advanced to contain native head 1edda7dfea84c159232d7aee4a4c049ace961b43. No merge endpoint was called, and that operation did not land code on main. Original stack record 20288 remains ordered [20283, 20287], preserving the historical open/merged states.

The historical retirement scope removes the dormant embedded AspireSkills bundle/provider/archive surface after native agent setup, while preserving direct CLI telemetry execution. Telemetry hooks are maintained independently with immutable released-source lineage, stale/replay/version-regression protection, atomic publication, and standalone provenance.

Part of #20246 and epic #20245.

Historical merge gates

These gates carry forward to successor #20947:

  • Stack recovery: register and verify the recovered open-draft replacement chain before merge.
  • Catalog and provenance: preserve the native catalog gate associated with Establish source release flow for aspire-skills aspire-skills#72 and the genuine v0.0.3 receipt. Standalone metadata pins a83ca78b9d35b8238f9c4fdba72cb779defe2a90; normalized SHA-512 values are 6fa64f350e2709a7abb52842232cc8d584e24d104b17a200a93ca4010d36a838dd1f6f73b83e92cd2ad0d8ad4abf9753323f840dc28625ccaf2f082097667181 for track-telemetry.sh and 303dc8d0eba181154222dacf6151d6bbe24614dd0e3144704fab1cb83c81f149e95cac47e87488357f0c04d232629a2f91f5551fe33e5c6c91f25a8f97aa0af3 for track-telemetry.ps1. No version floor or legacy fallback is introduced.
  • Security: complete security review for executable-source synchronization and provenance validation.

Validation

Final native parent receipt:

  • CLI/XLF, CLI.Tests, and CLI E2E builds passed with 0 warnings and 0 errors.
  • Supported native/catalog/mask/configuration/onboarding/command-selection coverage: 1,434 total; 1,410 passed; 24 platform/privilege skips; 0 failed.

Final retirement tip receipt:

  • dotnet build src\Aspire.Cli\Aspire.Cli.csproj /t:UpdateXlf /p:SkipNativeBuild=true — passed, 0 warnings and 0 errors.
  • dotnet build tests\Aspire.Cli.Tests\Aspire.Cli.Tests.csproj /p:SkipNativeBuild=true — passed, 0 warnings and 0 errors.
  • dotnet build tests\Aspire.Cli.EndToEnd.Tests\Aspire.Cli.EndToEnd.Tests.csproj /p:SkipNativeBuild=true — passed, 0 warnings and 0 errors.
  • Focused retirement/native coupling tests — 287 total; 286 passed; 1 platform skip; 0 failed.
  • Hook maintenance, workflow, routing, and monitoring tests — 444/444 passed.
  • pwsh -NoProfile -File eng\scripts\verify-telemetry-hooks.ps1 — verified v0.0.3 against canonical commit a83ca78b9d35b8238f9c4fdba72cb779defe2a90.
  • git diff --check — clean.

Security considerations

The updater downloads executable scripts only from a pinned full commit on the canonical released-main first-parent lineage, verifies the exact declared version and normalized SHA-512 hashes, checks tracked and pending-PR baselines, and stages all outputs before publication. Security review remains pending on successor #20947.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No

@github-actions

Copy link
Copy Markdown
Contributor

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20283

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20283"

@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot September 21, 2026 18:39
@github-actions github-actions Bot added the area-engineering-systems infrastructure helix infra engineering repo stuff label Sep 21, 2026
@github-actions

This comment has been minimized.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Removing the bundle verifier leaves the retained embedded skills archive without its artifact-attestation PR gate.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Separates telemetry-hook synchronization from the embedded Aspire skills bundle while preserving runtime hook installation and the current bundle fallback.

Changes:

  • Adds standalone hook provenance, update/verification scripts, and workflows.
  • Adds extensive offline maintenance and workflow coverage.
  • Removes archive-coupled bundle maintenance and scheduled monitoring.
File Description
.gitattributes Enforces LF for hook metadata.
.github/​workflows/​monitor-scheduled-workflows.config.json Removes retired scheduled updater.
.github/​workflows/​update-aspire-skills-bundle.yml Removes bundle updater workflow.
.github/​workflows/​update-telemetry-hooks.yml Adds hook synchronization workflow.
.github/​workflows/​verify-aspire-skills-bundle.yml Removes bundle verifier workflow.
.github/​workflows/​verify-telemetry-hook-changes.yml Updates protected branch name.
.github/​workflows/​verify-telemetry-hooks.yml Adds hook provenance verification.
docs/​ci/​monitor-scheduled-workflows.md Updates monitored workflow documentation.
eng/​github-ci/​test-trigger-map.yml Routes hook maintenance inputs to tests.
eng/​scripts/​aspire-skills-bundle.common.ps1 Removes bundle-coupled helpers.
eng/​scripts/​telemetry-hooks.common.ps1 Adds validation, synchronization, and publication logic.
eng/​scripts/​update-aspire-skills-bundle.ps1 Removes bundle updater.
eng/​scripts/​update-telemetry-hooks.ps1 Adds hook updater entry point.
eng/​scripts/​verify-aspire-skills-bundle.ps1 Removes bundle attestation verifier.
eng/​scripts/​verify-telemetry-hooks.ps1 Adds hook verifier entry point.
src/​Aspire.Cli/​Agents/​Hooks/​README.md Documents hook maintenance.
src/​Aspire.Cli/​Agents/​Hooks/​telemetry-hooks.metadata.json Records standalone provenance.
src/​Aspire.Cli/​Aspire.Cli.csproj Embeds provenance metadata.
tests/​Aspire.Cli.Tests/​Agents/​TelemetryHookInstallerTests.cs Validates installed hooks against standalone metadata.
tests/​Infrastructure.Tests/​PowerShellScripts/​AspireSkillsBundleHashTests.cs Removes obsolete bundle helper tests.
tests/​Infrastructure.Tests/​PowerShellScripts/​TelemetryHookMaintenanceTests.cs Adds comprehensive maintenance coverage.
tests/​Infrastructure.Tests/​TestTriggerMap/​TestTriggerMapTests.cs Updates routing expectations.
tests/​Infrastructure.Tests/​WorkflowScripts/​TelemetryHookWorkflowTests.cs Adds workflow contract coverage.

Comment thread .github/workflows/verify-telemetry-hooks.yml
@github-actions

This comment has been minimized.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Manual dispatch can base updates on arbitrary refs, and the retained skills archive loses its attestation gate.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Pin manual workflow checkout to main

.github/​workflows/​update-telemetry-hooks.yml:28

workflow_dispatch can be run against a non-main ref, and this checkout defaults to that selected ref. The subsequent force-push then creates the synchronization branch from that ref, so a manual replay can accidentally include unrelated branch commits in the PR to main. Pin the checkout to main (as update-nix-cli-flake.yml does) and cover that invariant in TelemetryHookWorkflowTests.

@github-actions

This comment has been minimized.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The provenance automation is security-sensitive, security review remains pending, and repository testing guidance still references a deleted verifier.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Update PR testing guidance after removing verifier

eng/​scripts/​verify-aspire-skills-bundle.ps1:1

Deleting this verifier leaves .agents/skills/pr-testing/ci-infra-testing.md:552-555 instructing PR testers to run verify-aspire-skills-bundle.ps1. Update that guidance in this PR so automated testing does not invoke a nonexistent script.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

@github-actions

This comment has been minimized.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The updater rewrites valid CRLF PowerShell hooks during an otherwise idempotent replay.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Normalize hook bytes to make Windows replay idempotent

eng/​scripts/​telemetry-hooks.common.ps1:403

A replay is not idempotent on Windows. .gitattributes leaves *.ps1 under text=auto, and Assert-TelemetryHooksLocalFiles correctly accepts a CRLF checkout after normalization, but this raw-byte comparison then treats the same canonical script as changed and rewrites it to LF, changing both the worktree and timestamp. Compare normalized bytes for hook files (while keeping metadata byte-exact), and cover this in ReplayingSameCommitPreservesBytesAndTimestamps with a CRLF/BOM fixture.

@github-actions

Copy link
Copy Markdown
Contributor

Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c0f73663-c37c-4c5a-b83d-89f0b8d13c5f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c0f73663-c37c-4c5a-b83d-89f0b8d13c5f
Retain main's dead Playwright scanner cleanup and resolve the overlapping Copilot detection test using editor evidence without a fabricated CLI probe.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c0f73663-c37c-4c5a-b83d-89f0b8d13c5f
Use --agents and project/global scope, default to detected and universal-skill clients, and leave agent-specific legacy skill directories untouched.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c0f73663-c37c-4c5a-b83d-89f0b8d13c5f
Use ID-free localized client instances, detected-agent defaults, direct bindings, and flags-based detection. Keep copied skill writes scoped to selected agents and retain standalone MCP consent.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c0f73663-c37c-4c5a-b83d-89f0b8d13c5f
Switch production agent initialization to independent native assets and shared consent-based onboarding. Preserve agent-first intent, direct bindings, actual discovery evidence, policies, pinned settings, atomic JSON edits, and standalone-only MCP. Keep dormant copied-bundle retirement in the upper layer.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@ellahathaway
Ella Hathaway (ellahathaway) force-pushed the ellahathaway-hook-only-maintenance branch from 2fb3da8 to ea92732 Compare October 13, 2026 00:17
@aspire-repo-bot
aspire-repo-bot Bot requested a balanced review from Copilot October 13, 2026 00:17

- name: Generate GitHub App Token for hook update
id: update-token
uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1

- name: Generate GitHub App Token for pull request
id: pr-token
uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
Comment on lines +28 to +30
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
ref: main
private-key: ${{ secrets.ASPIRE_BOT_PRIVATE_KEY }}

- name: Create or update pull request
uses: ./.github/actions/create-pull-request
runs-on: ubuntu-latest
if: ${{ github.repository_owner == 'microsoft' }}
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
@github-actions

Copy link
Copy Markdown
Contributor

Tests selector

Selects the full PR test matrix + all PR-gated jobs (ALL) — a rule matching '.gitattributes' selects ALL


Selection computed for commit ea92732.

Copilot AI 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.

🟡 Changes recommended

The workflow currently creates overprivileged App tokens with an unconfirmed Action pin, and its branch-name-only authorization can be spoofed by forked pull requests.

6 open findings
Files not reviewed (1)
  • src/Aspire.Cli/Resources/AgentCommandStrings.Designer.cs: Generated file

🧠 Review effort: Balanced

Comment on lines 41 to +42
if ($changedPaths.Count -gt 0 -and $env:PR_HEAD_REF -cne $allowedHeadRef) {
# A feature PR may consume a newly published bundle directly, but only when it carries
# every generated companion change. The separate bundle-verification workflow checks
# the archive attestation and the hook bytes against the pinned aspire-skills commit.
$bundleChangedPaths = @(& git --no-pager diff --no-ext-diff --no-textconv --no-renames --name-only 'HEAD^1' HEAD -- @requiredBundlePaths $bundleDirectory)
if ($LASTEXITCODE -ne 0) {
throw 'Could not inspect the generated Aspire skills bundle changes.'
}

$missingBundlePaths = @($requiredBundlePaths | Where-Object { $_ -notin $bundleChangedPaths })
$archiveChanged = $bundleChangedPaths | Where-Object {
$_ -match '^src/Aspire\.Cli/Agents/AspireSkills/Embedded/aspire-skills-.*\.(zip|tar\.gz|tgz)$'
}

if ($missingBundlePaths.Count -gt 0 -or -not $archiveChanged) {
throw "Telemetry hook scripts can only change on '$allowedHeadRef' or as part of a complete generated bundle update. Make changes in microsoft/aspire-skills and run the synchronization workflow instead of editing these copies manually."
}

Write-Host "Telemetry hook changes are part of a complete generated bundle update; canonical content is checked separately."
throw "Telemetry hook scripts can only change on '$allowedHeadRef'. Make changes in microsoft/aspire-skills and run the synchronization workflow instead of editing these copies manually."
Comment on lines +34 to +37
uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
with:
client-id: ${{ secrets.ASPIRE_BOT_APP_ID }}
private-key: ${{ secrets.ASPIRE_BOT_PRIVATE_KEY }}
Comment on lines +91 to +94
uses: actions/create-github-app-token@1b10c78c7865c340bc4f6099eb2f838309f1e8c3 # v3.1.1
with:
client-id: ${{ secrets.ASPIRE_BOT_APP_ID }}
private-key: ${{ secrets.ASPIRE_BOT_PRIVATE_KEY }}
### Step 6: Generate and publish skill files

**Action:** Run `playwright-cli install --skills` to generate agent skill files in the primary skill directory (`.claude/skills/playwright-cli/`), then mirror the skill directory to all other detected agent environment skill directories (e.g., `.github/skills/playwright-cli/`, `.opencode/skill/playwright-cli/`). The mirror is a full sync — files are created, updated, and stale files are removed so all environments have identical skill content.
**Action:** Run `playwright-cli install --skills` once in an owned temporary workspace, where it generates `.claude/skills/playwright-cli/`. Capture the complete payload, including supporting files, and publish it only to the selected agents' deduplicated skill directories in the requested scope (`--scope project|user`, default `project`). Copilot and OpenCode share `.agents/skills/playwright-cli/`; Claude uses `.claude/skills/playwright-cli/` and honors its user configuration-directory override. These common skill locations also work with compatible editor-hosted agents, without a separate VS Code installation target.
Comment on lines +50 to +51
Global telemetry hooks are eligible only for selected, actually detected
environments. Copilot CLI and App share a hook target; VS Code hints do not invent
Comment thread src/Aspire.Cli/Program.cs
Comment on lines +617 to +621
builder.Services.AddSingleton<IAgentSkillInstaller, AgentSkillInstaller>();
builder.Services.AddSingleton<AgentConfigurationWriter>();
builder.Services.AddSingleton<AgentSetupService>();
builder.Services.AddSingleton<ITelemetryHookInstaller, TelemetryHookInstaller>();
builder.Services.AddSingleton<ITelemetryHookConfigurator, TelemetryHookConfigurator>();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-engineering-systems infrastructure helix infra engineering repo stuff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants