refactor(hooks)!: retire the telemetry hook in favor of opt-in copilot-otel-metrics capture - #2719
refactor(hooks)!: retire the telemetry hook in favor of opt-in copilot-otel-metrics capture#2719Bill Berry (WilliamBerryiii) wants to merge 23 commits into
Conversation
Capture stays off until the developer enables the editor setting, and the skill presents the settings diff before writing it. - fail-closed redaction allow-list across trace, metric, and log pipelines - carrier test measures what survives the allow-list instead of asserting it - STRIDE security model over seven trust boundaries with registered gaps - local, local-stack, org-distribution, and azure-capture modes Refs #2717 📊 - Generated by Copilot
The manifest registered one command against eleven lifecycle events, so installing the package enrolled the developer in collection across every prompt, tool call, and subagent dispatch. Hooks are fail-closed, so a telemetry defect could deny every tool call in a session. BREAKING CHANGE: automatic hook-driven telemetry collection is removed. Capture is now opt-in through an editor setting handled by the copilot-otel-metrics skill. Local JSONL to static HTML reporting is replaced by Grafana dashboards and query helpers. Resolves #2496 Supersedes #2562 Refs #2717 🧹 - Generated by Copilot
- remove the hooks key and its maturity entry from affected package recipes - register copilot-otel-metrics in the experimental package - replace fixtures that referenced the real manifest with synthetic samples Refs #2717 🔧 - Generated by Copilot
Replaces the local telemetry guide with the copilot-otel-metrics guide and updates the security model and transparency note to describe opt-in capture. Refs #2717 📝 - Generated by Copilot
otelcol, OTTL, and spanlink appear in the copilot-otel-metrics guide. Refs #2717 📖 - Generated by Copilot
…metry-hook # Conflicts: # .github/skills/experimental/copilot-otel-metrics/SECURITY.md # docs/security/security-model.md
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF ScorecardScorecard details
Scanned Files
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2719 +/- ##
==========================================
- Coverage 83.17% 82.91% -0.26%
==========================================
Files 180 181 +1
Lines 32201 32553 +352
Branches 25 25
==========================================
+ Hits 26782 26992 +210
- Misses 5416 5558 +142
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Eval Execution✅ Status: Passed — no merge-blocking failures (46 advisory assertion failure(s) present)
|
CI runs pytest with --cov and requires tests/corpus/; the skill shipped neither. Seeds cover collector config, loopback and rejected URLs, JSONC settings, and redaction keys. Refs #2717 🧪 - Generated by Copilot
… test The catalog ships no hook manifest after this change, so the guard could never be satisfied. The contract under test is that the hook root is never allow-listed, which holds regardless. Refs #2717 🧰 - Generated by Copilot
Katrien De Graeve (katriendg)
left a comment
There was a problem hiding this comment.
Thanks for this — the privacy argument for retiring the hook is convincing, and the carrier harness that measures what the Collector filter actually reaches, rather than asserting it, is the strongest thing in the change. The threat model retracting the claims that harness disproved is exactly the right instinct.
Full findings with file:line references and suggested patches are in the review comments. Happy to re-review once the generated docs are regenerated.
Findings in files this PR does not touch
These could not be attached inline because the files carry no diff hunk. They are the documentation the hook retirement did not reach.
High — Customization packages guide states both packages include the telemetry hook
docs/customization/packages.md lines 25, 47-49
Two places contradict the new catalog. Line 25 gives 'the telemetry hook' as a reason not to install both packages together, and the dedicated ## Hooks section at lines 47-49 asserts that hve-core and hve-core-all each include it and that extension users must configure hook locations manually. Both statements are false against the current .github/plugin/marketplace.json, and the manual-configuration instruction is unactionable.
Suggested fix
Line 25: "Do not install `hve-core` and `hve-core-all` together. Both include overlapping content."
Lines 47-49: "## Hooks\n\nHooks are per-plugin declarations through the optional `hooks` field. No package currently declares one. VS Code has no declarative hook contribution point, so a package that adds a hook requires extension users to configure its location manually. Hooks are not copied during selective installation."High — Extension install guidance directs users to configure a hook location that no longer ships
docs/getting-started/install.md lines 112
The installation guide tells extension users to perform a manual configuration step for a component the repository no longer contains. A reader following this instruction after the merge will look for a hook manifest path to enter into their VS Code configuration and find nothing, and any reader who already wired .github/hooks/shared/telemetry.json into their settings from a previous release keeps a dangling path with no documented removal step.
Suggested fix
Delete the sentence, or replace it with a removal note: "HVE Core ships no hook manifest. If a previous release left a hook path in your VS Code configuration, remove it."High — Package selection guide still declares the telemetry hook and its manual setup step
docs/getting-started/packages.md lines 109
The Selective Clone Adoption section states that both packages declare the telemetry hook and instructs extension users to configure hook locations manually. Neither package declares a hooks field any more, so the declaration is false and the instruction cannot be completed.
Suggested fix
Remove the paragraph. The surrounding text at line 111 already covers the installer behaviour ("never copies hooks"), which stays correct as a generic contract statement.High — Generated package documents still advertise the deleted telemetry hook
docs/plugins/hve-core.md lines 131-135 (and docs/plugins/hve-core-all.md 272-276)
The PR removes the 'hooks' key and its componentMaturity entry from both the hve-core and hve-core-all recipes in .github/plugin/marketplace.json, but neither package document was regenerated. Both files still carry a generator-owned '### Hooks' table inside the AUTO-GENERATED ARTIFACTS region listing 'telemetry - experimental - Records Copilot session lifecycle events to local telemetry for reporting.' The repository convention is that a marketplace recipe change is followed by 'npm run docs:generate' with the regenerated pages committed, and 'npm run docs:generate:check' validates the region against the catalog. As shipped, both package documents describe a component the catalog no longer declares and the repository no longer contains, and the drift is inside a region a human is not permitted to hand-edit. This is the only place in the repository where the hook removal is functionally incomplete.
Also raised by the Readiness perspective: Both package landing pages carry a ### Hooks table inside the <!-- BEGIN AUTO-GENERATED ARTIFACTS --> region that lists a telemetry component as an included artifact of the package. .github/plugin/marketplace.json now contains no hooks key at all (grep for hooks in that file returns nothing) and git ls-files .github/hooks is empty, so the table advertises a component the package cannot deliver. These blocks are written by Update-PluginDocumentationSource in scripts/plugins/Generate-Plugins.ps1 (the section list at line 163 includes @{ Title = 'Hooks'; Kind = 'hook' }), which runs only under npm run plugin:generate. The validations named in the PR do not regenerate them: plugin:validate is an alias for lint:marketplace, docs:generate covers docs/reference/ only, and scripts/plugins/Modules/PluginHelpers.psm1:454 checks that the markers exist rather than that their content is current. The stale table therefore passes every check that was run.
Suggested fix
Run `npm run docs:generate` and commit the regenerated `docs/plugins/hve-core.md` and `docs/plugins/hve-core-all.md`; the `### Hooks` section should disappear with the recipe key. Confirm with `npm run docs:generate:check` and `npm run plugin:validate`. Do not hand-edit the generated region.Medium — Architecture reference states both plugin entries include the telemetry hook
docs/architecture/ai-artifacts.md lines 272
The Distribution section asserts that both plugin entries include the telemetry hook and that extension users configure hook locations manually. This is the architecture page a contributor reads to understand what a package contains, so a stale claim here propagates into further work. The neighbouring statements about the optional hooks field (line 213) and the closure model (line 222) remain correct as generic contract text.
Suggested fix
Choose the catalog entry that matches the required scope. Do not install `hve-core` and `hve-core-all` together because their content overlaps. No catalog entry currently declares a hook. VS Code has no declarative hook contribution point, so a package that adds one requires extension users to configure its location manually.Medium — Package landing page claims hve-core-all includes the shared telemetry hook
docs/plugins/hve-core-all.md lines 12, 15
Same defect as finding 8 on the aggregate package page: the membership sentence ends "It also includes the shared telemetry hook" and the CAUTION callout repeats the hook as shared content.
Suggested fix
Its membership spans planning, backlog integrations, data science, Design Thinking, security, accessibility, privacy, RPI, HVE Builder, and supporting operational tooling.
> [!CAUTION]
> Do not install `hve-core` and `hve-core-all` together. Both include shared content, so install one package based on the scope you need.Medium — Package landing page claims hve-core includes the shared telemetry hook
docs/plugins/hve-core.md lines 12, 15
The hand-written intro and the CAUTION callout both name the shared telemetry hook as package content. The callout also uses it as a reason not to install both packages together. Neither claim holds after the hooks key was removed from the recipe.
Suggested fix
It combines lifecycle coordination, prompt-engineering authoring and validation, documentation, Git and pull request workflows, and code review.
> [!CAUTION]
> Do not install `hve-core` and `hve-core-all` together. Both include shared content, so install one package based on the scope you need.AI-assisted review (Functional, Standards, Security, Readiness at comprehensive depth): 0 Critical, 10 High, 29 Medium, 18 Low across 57 findings. 50 posted inline, 7 above. PR description accuracy and mergeability were excluded from scope at the reviewer's request. Findings are suggestions for a human reviewer to weigh, not engineering sign-off.
…metry-hook # Conflicts: # .github/plugin/marketplace.json # TRANSPARENCY-NOTE.md # docs/customization/README.md # docs/docusaurus/src/data/__tests__/packageCards.test.ts # scripts/tests/extension/Cross-ChannelProjection.Tests.ps1 # scripts/tests/extension/Workflow-PackagingContracts.Tests.ps1 # scripts/tests/plugins/PluginHelpers.Materialization.Tests.ps1
|
Runtime and security (Medium)
Please add split-invocation settings tests, fresh/preinitialized Grafana volume tests, and an ambient-proxy regression test. |
|
Engineering standards and tests (Medium)
|
|
Accessibility and migration (Medium)
|
|
PR scope and readiness (Medium)
|
|
Documentation accuracy (4 Medium, 1 Low)
|
Emmeline Hoops (chaosdinosaur)
left a comment
There was a problem hiding this comment.
The hook retirement resolves the Windows fail-closed tool-call denial in #2496, but changes are requested before merge. The fleet workflow requires a workstation relay that is not shipped, existing Grafana volumes can retain stale credentials, settings validation can miss conflicts in the effective configuration, and local credentialed requests can traverse ambient proxies. Please also address the zero-hook regression coverage, migration guidance, documentation inconsistencies, and current branch/checklist blockers detailed in the inline and categorized comments.
…t lanes Move the shared path constants, redaction-policy accessors, and OTTL scrub inspectors into a non-collected support module so the fuzz harness and every test module import from one place instead of from another test module. Register a slow marker and deselect it by default so the fast lane needs no container runtime and the runtime lane is selected explicitly.
…agent-host relay Replace the fleet collector's narrow attribute delete-list with the same redaction allow-list, blocked-value patterns, and OTTL scrub statements the local collector uses, so a workstation cannot ship content the local pipeline would have removed. Add a two-file Compose-owned relay that terminates loopback OTLP/HTTP on the agent host, applies the same policy a second time, and forwards to the fleet endpoint over TLS with a bearer token and a pinned CA bundle. Runtime tests cover the trusted path plus wrong-bearer, untrusted-CA, and hostname-mismatch rejection.
…tainer bounds Resolve endpoint hosts before deciding loopback so a name that answers with both a loopback and a routable address is refused instead of accepted, and require a globally routable address for an opted-in remote endpoint. Write settings through a staged temporary file and an atomic replace, preserve a UTF-8 BOM, handle empty and comments-only files, evaluate policy against the merged result rather than the incoming fragment, drop userinfo from endpoint summaries, and retain a bounded number of backups in creation order. Bound the local Collector container with a memory limit, a read-only root, dropped capabilities, and no-new-privileges, and widen the blocked-value patterns to cover temporary AWS keys, unsigned JWTs, Azure account and shared-access keys, and common model-provider key shapes.
Parameterize the reusable pytest workflow with a changed-paths pattern, a marker expression, a strict-runtime toggle, an artifact suffix, and an optional Compose service to pre-pull, so a skill can select a container-backed subset without a second workflow. Pre-pull the digest-pinned Collector image with bounded retries and assert the pin before the tests run, then select the slow marker in a dedicated pull-request job so runtime TLS and authentication evidence is produced in CI rather than only on a developer machine.
…ill docs Rewrite the skill security model so the trust-boundary diagram, adversary entries, image inventory, and executive summary describe the shipped relay, double filtering, atomic settings write, and the availability cost of a relay outage, and record the new denial-of-service and infrastructure gaps. Correct the documented artifact inventory, helper counts, hook event list, and transparency note now that the telemetry hook is retired and the skill is opt-in, and reconcile the repository security model with the skill it describes.
…iants Multi-paragraph rationale essays in the Collector configurations, Compose files, and Python helpers duplicated material already owned by SECURITY.md and the references pages. Comments now state behavior, intent, invariants, and edge cases, and point to the documents that own the rationale. Also removes revision-history narration from module and function docstrings, which described how earlier versions of the skill behaved rather than the current contract.
…metry-hook # Conflicts: # .github/hooks/shared/telemetry.json # .github/plugin.json # TRANSPARENCY-NOTE.md # docs/contributing/hooks.md # docs/customization/README.md # docs/plugins/hve-core.md # scripts/plugins/Sync-PluginManifest.ps1
backup_path_for filled the lowest free collision slot, so when retention deleted a low index and another apply landed inside the same second, the new backup reclaimed that index and sorted ahead of older backups. Retention reads name order, so it would again delete the newest copy. The counter now resumes after the highest index already used for that second. The regression test pins the interaction with a fixed timestamp rather than depending on execution speed.
…metry-hook Resolve the skill security-model table: take the incoming jira, gitlab, and mural rows from the provider-authentication hardening, and keep this branch's copilot-otel-metrics row, which records the measured carrier map, the registered scope and schema gap, and the workstation relay.
The relay TLS cases could not read their own private key on the runner. OpenSSL writes keys 0600 for the creating user, the Collector image runs as 10001:10001, and the bind mount carries host mode through, so every case failed at receiver startup. They passed on Windows only because the WSL mount presents files as world-readable. Backup retention could also delete the newest backup. Name order is creation order only while the clock advances; the zero-padded counter orders within a second but nothing held the stamp when the wall clock stepped back, so the newest backup took the lowest name and was pruned first. The stamp is now held at the newest backup on disk, with a regression test driving a clock that moves backwards.
Close the descriptor that silence_broken_pipe duplicates onto stdout (py/file-not-closed). Give resolve_runtime a single exit so no path falls off the end implicitly (py/mixed-returns). Patch resolve_addresses by name so _input_policy is imported one way only (py/import-and-import-from).
The telemetry hook is retired, so the plugin identity and extension sections no longer claim it ships or needs manual configuration. The customization page said three carriers reach the store unfiltered and listed two; the instrumentation scope and schema fields are now stated as the third, matching the gap registered in the skill security model.
Pull Request
Description
This retires the local telemetry hook and makes the
copilot-otel-metricsskill the single supported path for Copilot telemetry capture.The replacement inverts that. Nothing is captured until the developer enables an editor setting, and the skill presents the exact settings diff before writing it. Someone who installs the package and never opts in emits nothing at all.
The hook also wrote a raw input artifact to local disk before any filtering decision, which put the privacy boundary after collection. The skill puts it first: otel-collector-local.yaml applies a fail-closed redaction processor across the trace, metric, and log pipelines, with
allow_all_keys: falsemaking the allow-list authoritative andblocked_valuesrunning as a second independent pass over whatever survives. An attribute introduced by a future extension release is dropped by default rather than stored by default.That filtering is measured rather than asserted. test_collector_carriers.py starts the pinned Collector with the shipped configuration, places a distinct marker in every OTLP carrier the signal model can transport, and records what actually survives. A carrier that opens fails the test rather than passing quietly.
Residual exposure is documented rather than papered over. SECURITY.md is a STRIDE model across seven trust boundaries with sixteen registered gaps.
G-INF-1states plainly that spans still carry prompt text, tool call arguments and results, and system instructions on a configuration left at its documented default, and that the plaintext loopback hop remains outside the skill's control.There is a reliability dimension as well. Hooks are fail-closed, so a telemetry defect could deny every tool call in a session, which is what #2496 reports. Ancillary observability code held the power to halt all work, and removing it from the hook path removes that class of failure.
Hook retirement
Removed telemetry.json and its entire owned runtime: three PowerShell entry points, three bash entry points, the Python core, the static report template, and the test package including its fuzz corpus. The repository now ships zero hook manifests, and the validator accepts that state.
Skill as the supported path
Added the files that complete the skill package, including pyproject.toml, uv.lock, the input-policy and settings-upsert helpers, the local Collector configuration, and a six-file test package covering carrier behavior, local configuration, helper guard placement, Azure templates, settings mutation, and fuzzing.
Packaging and fixtures
hookskey and its maturity entry from the affected package recipescopilot-otel-metricsin the experimental packageRelated Issue(s)
Resolves #2496
Refs #2717
Supersedes #2562
Type of Change
Select all that apply:
Code & Documentation:
Infrastructure & Configuration:
AI Artifacts:
hve-builderand addressed all actionable findings.github/instructions/*.instructions.md).github/prompts/*.prompt.md).github/agents/*.agent.md).github/skills/*/SKILL.md).github/hooks/*/*.json)evals/)Other:
.ps1,.sh,.py)Sample Prompts (for AI Artifact Contributions)
User Request:
Execution Flow:
The skill resolves one of four modes:
local-setup,local-stack,org-distribution, orazure-capture. For local capture it explains what the editor setting enables, presents the exact settings.json diff before writing, and applies the change through settings_upsert.py, which validates the schema, refuses the write when unrelated settings would change or the result would not parse, takes a backup first, restores on failure, and records a redacted audit entry. It then brings up the Collector and Grafana stack from compose.yaml, both images pinned by digest, with the fail-closed redaction allow-list applied before anything is stored.Output Artifacts:
An updated global settings.json with a backup alongside it, a running local stack, and a Grafana dashboard. No repository files are created.
Success Indicators:
verify.pyconfirms the pipeline is receiving signals,baseline.pycaptures a snapshot of metric and service names, and the Grafana dashboard renders usage and cost panels. Nothing is captured before the setting is enabled.For detailed contribution requirements, see:
Testing
Automated validation run locally:
npm run lint:hooksnpm run plugin:validatenpm run validate:skillsnpm run docs:generate:checkcomponent-copy.Tests.ps1PluginHelpers.Materialization.Tests.ps1Diff-based assessment: searched the repository for references to every deleted path and filename. Three matches were found and all three are false positives naming the
otel-collectorservice and thecopilot-otel-collectorcontainer rather than the deleted collector script. No real dangling references remain.Security analysis: no secrets appear in the diff. The Application Insights connection string is documented as operator-supplied and never written into a generated file. Both container images are pinned by manifest digest. Helper scripts route HTTP through a shared input policy that refuses non-http schemes, credentials in the authority, and non-loopback hosts without an explicit opt-in.
Two pre-existing failures in
scripts/tests/docs/Generate-AssetDocs.Tests.ps1were reproduced on a clean checkout of the base commit in an isolated worktree, confirming they are not introduced here. Four failures in the widerscripts/tests/plugins/directory remain unattributed; the suite modified by this PR passes in isolation.Manual testing was not performed.
Checklist
Required Checks
AI Artifact Contributions
hve-builderreview mode to review contributionhve-builderreviewRequired Local Checks
The following local-safe validation commands must pass before merging:
npm run validate:localnpm run validate:docsnpm run spell-checknpm run lint:md-linksSecurity Considerations
GHCP Maturity
Warning
This PR includes experimental GHCP artifacts that may have breaking changes.
.github/skills/experimental/copilot-otel-metrics/SKILL.mdAdditional Notes
Two capabilities are removed rather than replaced, and the change should be reviewed with that in mind:
Hook events are application-level and OTel traces are instrumentation-level, so the replacement is not a like-for-like substitute for lifecycle event capture. Fleet and organization capture through the Azure path is new capability with no hook equivalent.
This supersedes #2562, which proposed keeping a reduced three-hook spine with
sessionStartas the opt-in consent gate. The position taken here is that an editor setting is a better consent gate than a hook, because it is owned by the developer, visible in their own settings, and revocable without touching the package. The trade-off that issue raises still applies and is accepted: some report surfaces become dependent on what the developer chose to enable, which is the intended consequence of moving from ambient to opt-in collection..github/plugin/marketplace.jsonis also modified by open PR #2712, so one of the two will need a conflict resolution pass.