fix: source integrity for all integrators + README restructure#82
Merged
danielmeppiel merged 1 commit intomainfrom Feb 6, 2026
Merged
fix: source integrity for all integrators + README restructure#82danielmeppiel merged 1 commit intomainfrom
danielmeppiel merged 1 commit intomainfrom
Conversation
Closes #73 Source integrity (npm-style approach): - Remove metadata injection from all integrators (skills, prompts, agents, commands) - Integrated files are now pure build artifacts — verbatim copies from apm_modules/ - apm.lock is the single source of truth (no more frontmatter metadata) - Nuke-and-regenerate sync for prompts/agents/commands - Name-based orphan detection for skills (like node_modules/) - Fix uninstall to re-integrate remaining packages after nuke Init cleanup: - Remove SKILL.md from apm init (not needed for consumer projects) README restructure: - Lead with dependency manifest value prop, not standards - Add transitive dependency story - Reframe as 'Not Just Skills' — all primitives, not just skills - Fix compile messaging (compiles instructions, not agents) - Add community sources (github/awesome-copilot, anthropics/courses) Tests: 796 unit tests passing (+5 new uninstall re-integration tests)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements source integrity for all integrators by removing metadata injection and switching to a nuke-and-regenerate synchronization approach. It also removes SKILL.md from apm init (since it's only needed for publishable skill packages, not consumer projects) and restructures the README to lead with the dependency manifest value proposition.
Changes:
- Removed metadata injection from prompt, agent, skill, and command integrators — integrated files are now verbatim copies from
apm_modules/ - Implemented nuke-and-regenerate sync: uninstall removes all
-apmfiles, then re-integrates from remaining packages - Skills use npm-style orphan detection (directory name matching against lockfile) instead of metadata parsing
- Removed
SKILL.mdcreation fromapm initand updated README to emphasize APM as a dependency manager
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Version bump from 0.7.1 to 0.7.2 |
| tests/unit/test_uninstall_reintegration.py | New file with 5 tests for nuke-and-regenerate flow |
| tests/unit/test_orphan_detection.py | Updated tests to reflect nuke approach instead of selective orphan removal |
| tests/unit/test_init_command.py | Removed tests for SKILL.md creation, verified it's no longer created |
| tests/unit/integration/test_sync_integration_url_normalization.py | Updated to reflect nuke-and-regenerate behavior |
| tests/unit/integration/test_skill_integrator.py | Removed metadata injection tests, verified verbatim copying |
| tests/unit/integration/test_prompt_integrator.py | Removed metadata/versioning tests, verified verbatim copying |
| tests/unit/integration/test_command_integrator.py | Removed metadata tests, verified nuke-and-regenerate |
| tests/unit/integration/test_agent_integrator.py | Removed metadata tests, verified verbatim copying |
| src/apm_cli/integration/skill_integrator.py | Removed _add_apm_metadata function, implemented npm-style sync |
| src/apm_cli/integration/prompt_integrator.py | Removed metadata injection, simplified to verbatim copy with link resolution |
| src/apm_cli/integration/command_integrator.py | Removed metadata injection, implemented nuke-and-regenerate |
| src/apm_cli/integration/agent_integrator.py | Removed metadata injection, implemented nuke-and-regenerate |
| src/apm_cli/cli.py | Implemented two-phase uninstall: nuke all -apm files, then re-integrate remaining packages |
| docs/skills.md | Updated to reflect that apm init no longer creates SKILL.md |
| docs/integrations.md | Updated to describe verbatim copying instead of metadata injection |
| docs/cli-reference.md | Removed SKILL.md from apm init documentation |
| README.md | Major restructure: lead with dependency manifest, transitive dependencies, "Not Just Skills" |
Comment on lines
+1237
to
+1238
| except Exception as e: | ||
| prompts_failed += 1 |
There was a problem hiding this comment.
The error handling in the outer try-except block is too broad. If any exception occurs during Phase 1 (nuke) or Phase 2 (re-integrate), only prompts_failed is incremented (line 1238). This means errors during agent, skill, or command cleanup/re-integration won't be properly tracked and reported to the user. The error counters should be set based on which phase actually failed.
| then remaining packages are re-integrated from apm_modules/. | ||
| """ | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
Import of 'pytest' is not used.
Suggested change
| import pytest |
danielmeppiel
added a commit
that referenced
this pull request
Apr 29, 2026
… test asserts - pipeline.py:301 — pass ctx.auth_resolver to _preflight_auth_check instead of the local arg, which can be None even after resolve phase populates ctx.auth_resolver (resolve.py:91-92). Prevents an AttributeError on update installs that don't pass an AuthResolver explicitly. Flagged by Copilot reviewer. - 3 test files — replace 'dev.azure.com' in str(...) with bounded full-phrase equality assertion against 'Authentication failed for dev.azure.com'. Satisfies CodeQL #82/#83/#84 (incomplete URL substring sanitization) and our own tests.instructions.md rule banning bare URL/host substring checks. Tests: 20/20 green in tests/unit/install/. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added a commit
that referenced
this pull request
Apr 29, 2026
- pipeline.py:301 -- pass ctx.auth_resolver to _preflight_auth_check instead of the local 'auth_resolver' arg, which can be None even after resolve phase populates ctx.auth_resolver (resolve.py:91-92). Prevents AttributeError on update installs that don't pass an AuthResolver explicitly. Flagged by Copilot reviewer. - 3 test files -- replace 'dev.azure.com' in str(...) with bounded full-phrase equality assertion against 'Authentication failed for dev.azure.com'. Satisfies CodeQL alerts #82/#83/#84 (incomplete URL substring sanitization) and our own tests.instructions.md rule banning bare URL/host substring checks. Verified: 20/20 unit tests in tests/unit/install/test_*.py green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9 tasks
danielmeppiel
added a commit
that referenced
this pull request
Apr 29, 2026
…) (#1031) * fix(workflows): skip-don't-fail panel label gate; bump gh-aw v0.68.3 -> v0.71.1 Replace the on.steps: 'exit 1' label-name guards in pr-review-panel and triage-panel with top-level frontmatter 'if:' fields. gh-aw propagates top-level 'if:' to BOTH the pre_activation and activation jobs, so unmatched label events now render as a clean gray Skipped status instead of red Failed (which was polluting the CI dashboard on every PR labeled with anything other than 'panel-review', and on every issue labeled with anything other than 'status/needs-triage'). Workaround for the lack of native label-name filtering on pull_request_target / issues 'labeled' triggers. Both .md files now carry a TODO marker pointing at github/gh-aw ADR-28737, which adds a first-class 'on.labels:' filter (committed 2026-04-27, post-v0.71.1, not yet released). Once released, both gates can collapse to 'on.labels: [<name>]'. Also bump gh-aw v0.68.3 -> v0.71.1 (latest released) and recompile all workflows. Other lock.yml files and agentics-maintenance.yml change only because of the setup-action SHA bump and the regenerated maintenance-workflow template; no behavioural change there. Repro of the original noise: https://github.com/microsoft/apm/actions/runs/25089778042 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): add AuthenticationError to install/errors.py (#1015) Typed exception for remote-host auth failures (PAT rejected, bearer rejected, no credentials available). Carries a pre-rendered diagnostic_context from build_error_context so the renderer can display actionable guidance on the default output path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): thread auth_scheme + git_env through validation.py (#1015) Three defects fixed: 1. Pass auth_scheme from _dep_ctx to _build_repo_url so bearer tokens use extraheader injection instead of embedding JWT in URL userinfo. 2. Merge _dep_ctx.git_env (GIT_CONFIG_COUNT/KEY_0/VALUE_0 overrides) into the subprocess env so git ls-remote sends the Authorization header for AAD bearer tokens. 3. Detect auth-related failures (401/403/Authentication failed) from git ls-remote stderr and raise AuthenticationError with build_error_context diagnostics instead of returning bare False. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): re-raise AuthenticationError + --update pre-flight probe (#1015) - Add AuthenticationError to the re-raise chain (alongside PolicyViolationError, DirectDependencyError, PathTraversalError) so auth failures are not wrapped as "Failed to resolve APM dependencies: ...". - Add _preflight_auth_check(): when update_refs is set, run one git ls-remote per distinct (host, org) cluster BEFORE any write phase. Aborts with AuthenticationError carrying "No files were modified" + build_error_context diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): render AuthenticationError on default output path (#1015) Add except AuthenticationError handlers at both install entry points in commands/install.py. Renders the diagnostic_context (from build_error_context) on the DEFAULT path -- not --verbose-gated. Catches AuthenticationError BEFORE the generic Exception handler so the "Failed to install APM dependencies:" double-wrap is avoided. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(install): unit tests for ADO bearer auth + AuthenticationError (#1015) - test_errors.py: AuthenticationError attribute roundtrip, isinstance - test_validation_ado_bearer.py: bearer scheme passed to _build_repo_url, git_env merged into subprocess, 401/403 raise AuthenticationError, DNS failure returns False, PAT regression (basic scheme embeds token) - test_pipeline_auth_preflight.py: --update pre-flight rejects bad auth with "No files were modified", passes good auth, skips github.com, deduplicates (host, org) clusters - test_install_cmd_auth_rendering.py: AuthenticationError not caught by PolicyViolationError, bypasses generic RuntimeError wrap - Fix: let AuthenticationError propagate through outer try/except in _validate_package_exists (the catch-all was for parse failures only) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(install): integration tests for #1015 ADO auth regression - bearer-only install (no PAT) succeeds via az cli - bogus PAT + no az -> actionable diagnostic, no legacy wording - --update pre-flight abort: "No files were modified", files untouched - explicit PAT regression after bearer fix Note: integration tests require live ADO access + az login; skipped in CI unless APM_TEST_ADO_BEARER=1 and tenant context is configured. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(auth): lead ADO section with az login, add tenant-discovery guidance - Reorder ADO quick-start to recommend `az login` first, PAT second - Add tenant-discovery guidance (org settings URL + az account show) - Document auth-failure diagnostics and --update pre-flight behavior - Mirror changes to skill resource (authentication.md sync) Closes #1015 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chore: CHANGELOG entry for #1015 + trim install.py to LOC budget Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(install): address PR #1031 review (#1015) - pipeline.py:301 -- pass ctx.auth_resolver to _preflight_auth_check instead of the local 'auth_resolver' arg, which can be None even after resolve phase populates ctx.auth_resolver (resolve.py:91-92). Prevents AttributeError on update installs that don't pass an AuthResolver explicitly. Flagged by Copilot reviewer. - 3 test files -- replace 'dev.azure.com' in str(...) with bounded full-phrase equality assertion against 'Authentication failed for dev.azure.com'. Satisfies CodeQL alerts #82/#83/#84 (incomplete URL substring sanitization) and our own tests.instructions.md rule banning bare URL/host substring checks. Verified: 20/20 unit tests in tests/unit/install/test_*.py green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Daniel Meppiel <danielmeppiel@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #73
Source Integrity (npm-style approach)
apm_modules/apm.lockis the single source of truth (no more frontmatter metadata)node_modules/)Init Cleanup
SKILL.mdfromapm init(not needed for consumer projects)README Restructure
Tests