From e05e70a9b932678ae221c7735115dbdddc03b502 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 8 Sep 2026 09:33:35 +0000 Subject: [PATCH 1/5] Fix shared skill packing and restore review panel workflow payload Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com> --- .../owners/install-deployment.json | 7 +- .github/workflows/pr-review-panel.lock.yml | 14 +- .github/workflows/pr-review-panel.md | 28 ++- .../workflows/verify-shared-apm-matrix.yml | 84 ++++++- docs/src/content/docs/integrations/gh-aw.md | 10 + docs/src/content/docs/reference/cli/pack.md | 4 +- .../.apm/skills/apm-usage/commands.md | 2 +- .../marketplace_package_and_registration.py | 37 ++- src/apm_cli/integration/targets.py | 24 +- ...architecture_install_compound_mutations.py | 31 +++ .../test_pack_shared_skill_roundtrip.py | 105 +++++++++ .../test_targets_registry_completeness.py | 40 +++- tests/unit/test_lockfile_enrichment.py | 37 +++ .../unit/test_shared_apm_workflow_contract.py | 221 +++++++++++++++++- 14 files changed, 612 insertions(+), 32 deletions(-) create mode 100644 tests/integration/test_pack_shared_skill_roundtrip.py diff --git a/.apm/architecture/owners/install-deployment.json b/.apm/architecture/owners/install-deployment.json index a0872223aa..d3f48ebf7c 100644 --- a/.apm/architecture/owners/install-deployment.json +++ b/.apm/architecture/owners/install-deployment.json @@ -73,11 +73,12 @@ }, { "id": "bundle-native-layout-lowering", - "decision": "Bundle-native directory and filename lowering to APM primitive kinds and target deploy layout", - "owner": "bundle/plugin_layout.py (PLUGIN_LAYOUT, plugin_command_prompt_name) consumed by install/local_bundle_paths.py (_lower_to_target)", + "decision": "Bundle-native directory and filename lowering to APM primitive kinds, target deploy layout, and pack eligibility", + "owner": "bundle/plugin_layout.py (PLUGIN_LAYOUT, plugin_command_prompt_name) and integration/targets.py (TargetProfile) consumed by install/local_bundle_paths.py and bundle/lockfile_enrichment.py", "selectors": [ "src/apm_cli/bundle/plugin_layout.py", - "src/apm_cli/install/local_bundle_paths.py" + "src/apm_cli/install/local_bundle_paths.py", + "src/apm_cli/bundle/lockfile_enrichment.py" ], "guards": ["install-deployment-bundle-native-layout"] }, diff --git a/.github/workflows/pr-review-panel.lock.yml b/.github/workflows/pr-review-panel.lock.yml index f4ea631636..85a0805523 100644 --- a/.github/workflows/pr-review-panel.lock.yml +++ b/.github/workflows/pr-review-panel.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"bb3a51a96859048b339270f886e0b1d3f360bfa40174d8d369be217d5dcd452e","body_hash":"aa0c074035e68f2ea03c7c732f9a28e1737ab64d54e8a11aabd0c4bdc6702213","compiler_version":"v0.87.8","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.80"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b6995da6ca222f76b2b1dee723500e90efd4602573176d3ea9963599931f1319","body_hash":"aa0c074035e68f2ea03c7c732f9a28e1737ab64d54e8a11aabd0c4bdc6702213","compiler_version":"v0.87.8","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.80"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_GITHUB_TOKEN","GH_AW_DEFAULT_OTLP_HEADERS","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_PLUGINS_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/create-github-app-token","sha":"bcd2ba49218906704ab6c1aa796996da409d3eb1","version":"v3.2.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"1aa033c7bf25ac9428fe521065b90c30a7070c4e","version":"v0.87.8"},{"repo":"microsoft/apm-action","sha":"d723bb64ed70c135bbaf87d126b721dd2dae0439","version":"v1.10.0"},{"repo":"ruby/setup-ruby","sha":"95ef2b042f9d7a56d8268cba8559e2842e2ad01b","version":"v1.321.0"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.28.7","digest":"sha256:40a1e30b1b8d70642d4292485146cd5af612730d7a6a2e12706ddd13df375059","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.28.7@sha256:40a1e30b1b8d70642d4292485146cd5af612730d7a6a2e12706ddd13df375059"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.28.7","digest":"sha256:4f209dd4cbc74d47a6c7379956143de293429d1b1b2fb2647776cdcbf65836a1","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.28.7@sha256:4f209dd4cbc74d47a6c7379956143de293429d1b1b2fb2647776cdcbf65836a1"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.28.7","digest":"sha256:fb362a08d4d2f0da6c036e3f5d3b2fd87931e857fec3ca4a241cd2f2b61131f9","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.28.7@sha256:fb362a08d4d2f0da6c036e3f5d3b2fd87931e857fec3ca4a241cd2f2b61131f9"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.12","digest":"sha256:92d5377b6bd32cd5b9306b2a553f7ef3549bccff9207e46f931e7249bc718713","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.12@sha256:92d5377b6bd32cd5b9306b2a553f7ef3549bccff9207e46f931e7249bc718713"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:bac2192f6374d6262116399b34fc5e143d576f82719e90a18261cae7480f4d4e","pinned_image":"ghcr.io/github/gh-aw-node@sha256:bac2192f6374d6262116399b34fc5e143d576f82719e90a18261cae7480f4d4e"},{"image":"ghcr.io/github/github-mcp-server:v1.11.0","digest":"sha256:fbec75de11c255213fa08d80fb166abe73d851fff631c51c0079872967720699","pinned_image":"ghcr.io/github/github-mcp-server:v1.11.0@sha256:fbec75de11c255213fa08d80fb166abe73d851fff631c51c0079872967720699"}],"has_pull_request_target":true,"mcp_servers":[{"name":"github","tools":["get_commit","get_file_contents","get_latest_release","get_me","get_pull_request","get_pull_request_comments","get_pull_request_diff","get_pull_request_files","get_pull_request_review_comments","get_pull_request_reviews","get_pull_request_status","get_release_by_tag","get_tag","issue_read","list_branches","list_commits","list_issue_types","list_issues","list_pull_requests","list_releases","list_starred_repositories","list_tags","pull_request_read","search_code","search_issues","search_pull_requests","search_repositories"]},{"name":"safeoutputs","tools":["add_comment","missing_data","missing_tool","noop","remove_labels"]}]} # This file was automatically generated by gh-aw (v0.87.8). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -532,7 +532,7 @@ jobs: - name: Restore APM packages (all bundles) uses: microsoft/apm-action@d723bb64ed70c135bbaf87d126b721dd2dae0439 # v1.10.0 with: - apm-version: 0.28.0 + apm-version: 0.30.0 bundles-file: /tmp/gh-aw/apm-bundle-list.txt - name: Install GitHub Copilot CLI @@ -572,6 +572,10 @@ jobs: env: GH_AW_SKILL_DIR: ".github/skills" run: bash "${RUNNER_TEMP}/gh-aw/actions/restore_inline_skills.sh" + - name: Verify restored review panel skill and resources + run: "set -euo pipefail\ncd \"$GITHUB_WORKSPACE\"\nskill=.agents/skills/apm-review-panel\nfor resource in \\\n SKILL.md \\\n assets/panelist-return-schema.json \\\n assets/ceo-return-schema.json \\\n assets/recommendation-template.md\ndo\n if [ ! -f \"$skill/$resource\" ] || [ ! -s \"$skill/$resource\" ]; then\n echo \"::error::Missing or empty required review panel file: $skill/$resource. Check the APM install/pack/restore bundle before retrying.\"\n exit 1\n fi\ndone\n" + shell: bash + - name: Download container images run: bash "${RUNNER_TEMP}/gh-aw/actions/download_docker_images.sh" ghcr.io/github/gh-aw-firewall/agent:0.28.7@sha256:40a1e30b1b8d70642d4292485146cd5af612730d7a6a2e12706ddd13df375059 ghcr.io/github/gh-aw-firewall/api-proxy:0.28.7@sha256:4f209dd4cbc74d47a6c7379956143de293429d1b1b2fb2647776cdcbf65836a1 ghcr.io/github/gh-aw-firewall/squid:0.28.7@sha256:fb362a08d4d2f0da6c036e3f5d3b2fd87931e857fec3ca4a241cd2f2b61131f9 ghcr.io/github/gh-aw-mcpg:v0.4.12@sha256:92d5377b6bd32cd5b9306b2a553f7ef3549bccff9207e46f931e7249bc718713 ghcr.io/github/gh-aw-node@sha256:bac2192f6374d6262116399b34fc5e143d576f82719e90a18261cae7480f4d4e ghcr.io/github/github-mcp-server:v1.11.0@sha256:fbec75de11c255213fa08d80fb166abe73d851fff631c51c0079872967720699 - name: Prepare Safe Outputs Directories @@ -1331,12 +1335,12 @@ jobs: GITHUB_APM_PAT: ${{ steps.package-token.outputs.token }} GITHUB_TOKEN: ${{ steps.package-token.outputs.token }} with: - apm-version: 0.28.0 + apm-version: 0.30.0 archive: "true" dependencies: ${{ steps.list.outputs.deps }} isolated: "true" pack: "true" - target: copilot + target: copilot,agent-skills working-directory: /tmp/gh-aw/apm-workspace - name: Upload APM bundle artifact if: success() @@ -1379,7 +1383,7 @@ jobs: fi done env: - AW_APM_TARGET: copilot + AW_APM_TARGET: copilot,agent-skills - name: Validate APM token source run: | set -euo pipefail diff --git a/.github/workflows/pr-review-panel.md b/.github/workflows/pr-review-panel.md index 1823fd265f..881fb42825 100644 --- a/.github/workflows/pr-review-panel.md +++ b/.github/workflows/pr-review-panel.md @@ -87,10 +87,36 @@ permissions: imports: - uses: shared/apm.md with: - target: copilot + apm-version: '0.30.0' + # Temporary workaround: 0.30 installs Copilot skills under .agents, + # but sole-target copilot packing omits that tree. Keep the shared + # default unchanged; remove agent-skills after the pack fix ships. + target: copilot,agent-skills packages: - microsoft/apm#main +# Fail before inference if the trusted bundle lost the panel or its resources. +# This hook runs after shared/apm.md's restore and framework initialization. +# Only inspect file metadata here; never execute bundled scripts or fetch PR head. +pre-agent-steps: + - name: Verify restored review panel skill and resources + shell: bash + run: | + set -euo pipefail + cd "$GITHUB_WORKSPACE" + skill=.agents/skills/apm-review-panel + for resource in \ + SKILL.md \ + assets/panelist-return-schema.json \ + assets/ceo-return-schema.json \ + assets/recommendation-template.md + do + if [ ! -f "$skill/$resource" ] || [ ! -s "$skill/$resource" ]; then + echo "::error::Missing or empty required review panel file: $skill/$resource. Check the APM install/pack/restore bundle before retrying." + exit 1 + fi + done + tools: github: toolsets: [default] diff --git a/.github/workflows/verify-shared-apm-matrix.yml b/.github/workflows/verify-shared-apm-matrix.yml index ff24a6a23e..18135603bc 100644 --- a/.github/workflows/verify-shared-apm-matrix.yml +++ b/.github/workflows/verify-shared-apm-matrix.yml @@ -2,7 +2,7 @@ name: Verify shared/apm.md matrix secret-stripping fix # Empirical proof for fixes in .github/workflows/shared/apm.md. # -# Three job sets: +# Four job sets: # # A. prove-old-pattern-strips-output (regression sentinel) # Replicates the pre-fix shape (PEM embedded in a job output). Asserts @@ -34,6 +34,9 @@ name: Verify shared/apm.md matrix secret-stripping fix # at runtime, so apm-action resolves '' || 'latest' and floats. # (3) Drift guard (source of truth): the schema default's pack format # must be detected by the pinned microsoft/apm-action ref. +# Retains the 0.28.0 default multi-bundle proof and independently checks +# the review panel's 0.30.0 copilot,agent-skills override: real install, +# archive and clean restore must preserve each required resource's bytes. # # D. pack-format-consumer-compat (cross-repo drift guard) # Proves the `apm pack --archive` default archive format (.zip) stays @@ -539,6 +542,85 @@ jobs: test -n "$(find "$RESTORE_ROOT" -type f -print -quit)" echo "[+] APM 0.28 packed two explicit targets and restored both bundles." + c-apm-030-panel-compat: + name: C. APM 0.30 review panel skill roundtrip + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + steps: + # No checkout: installed and restored files cannot be confused with + # the repository's committed deployment tree. Match the production + # dependency and per-consumer override, not the shared 0.28.0 default. + - name: Pack real review panel with APM 0.30 + id: pack-panel + uses: microsoft/apm-action@d723bb64ed70c135bbaf87d126b721dd2dae0439 # v1.10.0 + with: + dependencies: | + - microsoft/apm#main + isolated: 'true' + pack: 'true' + archive: 'true' + target: copilot,agent-skills + apm-version: '0.30.0' + working-directory: ${{ runner.temp }}/apm-030-panel-pack + + - name: Require a fresh panel restore destination + env: + RESTORE_ROOT: ${{ runner.temp }}/apm-030-panel-restore + run: | + set -euo pipefail + test ! -e "$RESTORE_ROOT" + + - name: Restore review panel bundle with APM 0.30 + uses: microsoft/apm-action@d723bb64ed70c135bbaf87d126b721dd2dae0439 # v1.10.0 + with: + apm-version: '0.30.0' + bundle: ${{ steps.pack-panel.outputs.bundle-path }} + working-directory: ${{ runner.temp }}/apm-030-panel-restore + + - name: Compare installed, archived, and restored panel bytes + shell: python + env: + PACK_ROOT: ${{ runner.temp }}/apm-030-panel-pack + BUNDLE: ${{ steps.pack-panel.outputs.bundle-path }} + RESTORE_ROOT: ${{ runner.temp }}/apm-030-panel-restore + run: | + import os + from pathlib import Path + from zipfile import ZipFile + + # Read only the four required skill/resource files, not persona + # definitions. A nonempty .github tree alone missed this regression. + required = ( + "SKILL.md", + "assets/panelist-return-schema.json", + "assets/ceo-return-schema.json", + "assets/recommendation-template.md", + ) + skill = Path(".agents/skills/apm-review-panel") + with ZipFile(os.environ["BUNDLE"]) as archive: + names = archive.namelist() + markers = [ + name for name in names + if name.count("/") == 1 and name.endswith("/apm.lock.yaml") + ] + assert len(markers) == 1, "Expected one APM bundle wrapper" + wrapper = markers[0].split("/")[0] + for resource in required: + deployed = skill / resource + installed = Path(os.environ["PACK_ROOT"]) / deployed + restored = Path(os.environ["RESTORE_ROOT"]) / deployed + assert installed.is_file(), f"Not installed: {deployed}" + expected = installed.read_bytes() + assert expected, f"Empty installed resource: {deployed}" + member = f"{wrapper}/{deployed.as_posix()}" + assert names.count(member) == 1, f"Missing or duplicate archive entry: {deployed}" + assert archive.read(member) == expected, f"Archive bytes differ: {deployed}" + assert restored.is_file(), f"Not restored: {deployed}" + assert restored.read_bytes() == expected, f"Restored bytes differ: {deployed}" + print("[+] 4 review panel files survived install, pack, and restore byte-for-byte.") + # ===================================================================== # Job set D: apm pack archive-format <-> apm-action consumer detection. # Outage shape: GH-AW Compatibility job 'apm pack produced no bundle'. diff --git a/docs/src/content/docs/integrations/gh-aw.md b/docs/src/content/docs/integrations/gh-aw.md index 5ba6f2205f..12090534bb 100644 --- a/docs/src/content/docs/integrations/gh-aw.md +++ b/docs/src/content/docs/integrations/gh-aw.md @@ -93,6 +93,16 @@ imports: Use a bare semver tag (e.g. `'0.28.0'`). Pass `'latest'` to opt into floating to the newest release; omit the input entirely to keep the workflow's pinned default. +:::caution[Temporary Copilot skill-bundle workaround] +APM 0.30.0 still omits `.agents/skills/` under `--format apm --target copilot`. Only target-filtered legacy APM packaging is affected, not target-agnostic plugin formats. The PR review panel temporarily sets `apm-version: '0.30.0'` and `target: 'copilot,agent-skills'`; the shared default remains 0.28.0. + +Change the source workflow import, then run `gh aw compile` so the generated `.lock.yml` upgrades both pack and restore. Do not hand-edit generated locks or use `apm self-update`. Before agent launch, the workflow checks restored `.agents/skills/apm-review-panel/SKILL.md` and its three required assets. + +After these changes merge into trusted `main`, maintainers must start a fresh `workflow_dispatch` for PR #2741 and verify an actual recommendation. Re-running an old run does not validate the new workflow. + +The permanent fix is not yet published. Keep `agent-skills` until you pin a release containing that fix and recompile. +::: + Copies vendored before this change default to APM 0.21.0, the repository's current CLI line when that default was selected. If a copy's `apm-action pin:` line reads `v1.4.2`, its target input applies only to packing and does not reach the isolated install. To migrate: 1. Replace `.github/workflows/shared/apm.md` with the [canonical file](https://github.com/microsoft/apm/blob/main/.github/workflows/shared/apm.md). This also moves the default to the compatibility-tested 0.28.0. diff --git a/docs/src/content/docs/reference/cli/pack.md b/docs/src/content/docs/reference/cli/pack.md index 434fc6ed93..3dc981ad27 100644 --- a/docs/src/content/docs/reference/cli/pack.md +++ b/docs/src/content/docs/reference/cli/pack.md @@ -22,7 +22,7 @@ apm pack [OPTIONS] The bundle is built from `apm.lock.yaml`. An enriched copy of the lockfile (per-file SHA-256 in `bundle_files`, plus `pack:` metadata) is embedded inside the bundle so `apm install ` can verify integrity at install time. -Bundles are target-agnostic. The consumer's project decides where files land at install time -- the bundle carries no harness binding. Flags whose scope does not match the detected outputs are silent no-ops, not errors, so the same `apm pack` invocation works in CI across projects that produce only a bundle, only a marketplace, or both. +Plugin bundles are target-agnostic. The consumer's project decides where files land at install time -- the bundle carries no harness binding. Legacy `--format apm` packaging filters paths by target; see the [gh-aw workaround](../../../integrations/gh-aw/) for omitted Copilot skills. Flags whose scope does not match the detected outputs are silent no-ops, not errors, so the same `apm pack` invocation works in CI across projects that produce only a bundle, only a marketplace, or both. ## Options @@ -45,7 +45,7 @@ Bundles are target-agnostic. The consumer's project decides where files land at | `--check-versions` | off | Release gate: verify per-package versions agree with the configured `marketplace.versioning.strategy` (`lockstep`, `tag_pattern`, or `per_package`). Exits `3` on misalignment. Composes with `--check-clean` and `--dry-run`. | | `--check-clean` | off | Read-only release gate: regenerate every configured marketplace output in memory and diff against the same effective path used by `apm pack`, including `--marketplace-path` overrides. Exits `4` for drift or uncertifiable remote Claude metadata. It automatically suppresses normal pack writes. | | `--strict-metadata` | off | Claude marketplace: fail before writing when remote package metadata cannot be fetched. Use it in publishing CI to require those fetches to succeed. Exits `5` before `--check-clean` runs when both flags are present. | -| `--target`, `-t VALUE` | auto-detect | **Deprecated.** Recorded as informational `pack.target` metadata only; ignored by `apm install`. Will be removed in a future release. | +| `--target`, `-t VALUE` | auto-detect | **Deprecated.** Filters paths for legacy `--format apm` bundles; plugin formats remain target-agnostic. Recorded as informational `pack.target` metadata; ignored by `apm install`. | :::caution[Migrating automation from `.tar.gz`?] `apm pack --archive` now produces `.zip`. If your CI release, checksum, or diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 1ec5ae5235..85595f42c1 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -266,7 +266,7 @@ Lifecycle scripts fire on six events: `pre-install`, `post-install`, `pre-update | Command | Purpose | Key flags | |---------|---------|-----------| -| `apm pack` | Build distributable artifacts (bundle and/or marketplace.json -- driven by `apm.yml`). A `dependencies:` mapping, including `dependencies: {}`, produces a bundle of local package content; omitted or null `dependencies:` does not. Default output (no format flag) is a Claude Code plugin directory. Pass `--format agent-plugin` to opt into a portable Agent Plugins v1 bundle instead -- strict portable core only (root `plugin.json`, `skills/`, root `mcp.json` written even when empty; no `agents/`, `commands/`, `instructions/`, `extensions/`, `hooks/`, or LSP payload). That bundle build fails before any output is written if the source project has non-portable agents/commands/instructions/extensions/hooks/LSP, naming the surfaces and pointing to `--format claude-plugin` (and to configuring LSP in the target directly, since neither pack format carries it). A packed Agent Plugin installs through the declarative route: declare it as a dependency in `apm.yml` and run `apm install --target copilot`, which keeps the unit whole under `apm_modules/` and registers it without locating or executing Copilot; stable Copilot CLI 1.0.81 or newer is required when loading the projection. The imperative local-bundle route still fails closed for Agent Plugin bundles. Bundles are **target-agnostic**: `pack.target` is recorded in every bundle for diagnostic purposes (typically `"all"` for target-agnostic packs, or the project's detected target) and is not authoritative at install time; `pack.bundle_files` (path -> sha256) drives integrity verification. The consumer's project decides where files land. Dependency content is packed **exclusively** from lockfile-attested `deployed_files` (in every bundle format); the `apm_modules` cache is never packed. Each file is verified against its `deployed_file_hashes` SHA-256 before inclusion, so a file tampered after `apm install` (hash mismatch) or deleted (missing on disk) fails the pack with a message pointing at `apm install`; files with no recorded hash (older lockfiles) pack unverified. Dependency hooks-config / MCP-config is not attested, so it is not packed -- `apm pack` warns (`[!]`) and names the dependency (first-party root hooks/MCP are still packed). Marketplace-publishing projects (`marketplace:` block, no `dependencies:`) no longer emit the misleading "No plugin.json found" warning; after a successful build, a vendor-neutral catalog of artifact paths is appended together with a single docs pointer (`producer/publish-to-a-marketplace/#consume-from-any-assistant`) listing per-assistant install paths. Release-time gates `--check-versions` and `--check-clean` are opt-in and exit non-zero on misalignment / drift (codes 3 and 4 respectively) so release pipelines can fail fast; `--check-clean` is always read-only and never writes pack outputs. The version gate reads a local package's `apm.yml` first; Plugin collections without `apm.yml` use `plugin.json`'s `version`. Invalid or versionless `apm.yml` fails closed, and the fallback likewise rejects malformed or non-object JSON and a missing or blank version. When `apm.yml` declares `target: claude` or `target: copilot` (or the plural `targets:` equivalent), `apm pack` also generates an ecosystem-specific `plugin.json`: `.claude-plugin/plugin.json` for Claude (includes `mcpServers` from `.mcp.json` if present) and `.github/plugin/plugin.json` for Copilot (omits `mcpServers`). An existing file at the target path is preserved (a warning is emitted and the write is skipped) unless `--force` is passed; `--dry-run` prevents writes. Credential-bearing keys and secret-shaped values in `.mcp.json` are stripped recursively at any depth from the Claude manifest before writing, so a committed manifest never leaks secrets (see the apm pack reference, `reference/cli/pack/#credential-stripping-claude-mcpservers`). | `-o PATH`, `--archive` (produce a `.zip` archive instead of a directory; changed from `.tar.gz`), `--archive-format [zip\|tar.gz]` (default `zip`; use `tar.gz` for smaller legacy CI artifacts; only active with `--archive`), `--dry-run`, `--format [plugin\|agent-plugin\|claude\|claude-plugin\|apm]` (`agent-plugin` is the sole selector for the Agent Plugin bundle; `plugin` is a compatibility alias for the Claude plugin bundle, not for `agent-plugin`; `claude`/`claude-plugin` also select the Claude plugin bundle; `apm` selects the legacy APM layout; default `claude-plugin`), `--claude-plugin` (shortcut for `--format claude-plugin`; passing more than one of `--claude-plugin`/`--format` is a usage error), `--force`, `--offline`, `--include-prerelease`, `--marketplace=FORMATS`, `--marketplace-path FORMAT=PATH`, `--json`, `--check-versions` (release gate: per-package versions match `marketplace.versioning.strategy`; exit 3 on failure), `--check-clean` (read-only release gate: regenerate-and-diff against the effective marketplace path, including `--marketplace-path` overrides; never writes pack outputs; exit 4 on drift). `-t/--target` is **deprecated** (warn only). Exit codes: `0` success, `1` build/runtime error, `2` schema validation error, `3` `--check-versions` misalignment, `4` `--check-clean` drift. | +| `apm pack` | Build distributable artifacts (bundle and/or marketplace.json -- driven by `apm.yml`). A `dependencies:` mapping, including `dependencies: {}`, produces a bundle of local package content; omitted or null `dependencies:` does not. Default output (no format flag) is a Claude Code plugin directory. Pass `--format agent-plugin` to opt into a portable Agent Plugins v1 bundle instead -- strict portable core only (root `plugin.json`, `skills/`, root `mcp.json` written even when empty; no `agents/`, `commands/`, `instructions/`, `extensions/`, `hooks/`, or LSP payload). That bundle build fails before any output is written if the source project has non-portable agents/commands/instructions/extensions/hooks/LSP, naming the surfaces and pointing to `--format claude-plugin` (and to configuring LSP in the target directly, since neither pack format carries it). A packed Agent Plugin installs through the declarative route: declare it as a dependency in `apm.yml` and run `apm install --target copilot`, which keeps the unit whole under `apm_modules/` and registers it without locating or executing Copilot; stable Copilot CLI 1.0.81 or newer is required when loading the projection. The imperative local-bundle route still fails closed for Agent Plugin bundles. Plugin bundles are **target-agnostic**; legacy `--format apm` packaging filters paths by target (see `reference/cli/pack/`). `pack.target` is diagnostic metadata, not authoritative at install time; `pack.bundle_files` (path -> sha256) drives integrity verification. The consumer decides where files land. Dependency content is packed **exclusively** from lockfile-attested `deployed_files` (in every bundle format); the `apm_modules` cache is never packed. Each file is verified against its `deployed_file_hashes` SHA-256 before inclusion, so a file tampered after `apm install` (hash mismatch) or deleted (missing on disk) fails the pack with a message pointing at `apm install`; files with no recorded hash (older lockfiles) pack unverified. Dependency hooks-config / MCP-config is not attested, so it is not packed -- `apm pack` warns (`[!]`) and names the dependency (first-party root hooks/MCP are still packed). Marketplace-publishing projects (`marketplace:` block, no `dependencies:`) no longer emit the misleading "No plugin.json found" warning; after a successful build, a vendor-neutral catalog of artifact paths is appended together with a single docs pointer (`producer/publish-to-a-marketplace/#consume-from-any-assistant`) listing per-assistant install paths. Release-time gates `--check-versions` and `--check-clean` are opt-in and exit non-zero on misalignment / drift (codes 3 and 4 respectively) so release pipelines can fail fast; `--check-clean` is always read-only and never writes pack outputs. The version gate reads a local package's `apm.yml` first; Plugin collections without `apm.yml` use `plugin.json`'s `version`. Invalid or versionless `apm.yml` fails closed, and the fallback likewise rejects malformed or non-object JSON and a missing or blank version. When `apm.yml` declares `target: claude` or `target: copilot` (or the plural `targets:` equivalent), `apm pack` also generates an ecosystem-specific `plugin.json`: `.claude-plugin/plugin.json` for Claude (includes `mcpServers` from `.mcp.json` if present) and `.github/plugin/plugin.json` for Copilot (omits `mcpServers`). An existing file at the target path is preserved (a warning is emitted and the write is skipped) unless `--force` is passed; `--dry-run` prevents writes. Credential-bearing keys and secret-shaped values in `.mcp.json` are stripped recursively at any depth from the Claude manifest before writing, so a committed manifest never leaks secrets (see the apm pack reference, `reference/cli/pack/#credential-stripping-claude-mcpservers`). | `-o PATH`, `--archive` (produce a `.zip` archive instead of a directory; changed from `.tar.gz`), `--archive-format [zip\|tar.gz]` (default `zip`; use `tar.gz` for smaller legacy CI artifacts; only active with `--archive`), `--dry-run`, `--format [plugin\|agent-plugin\|claude\|claude-plugin\|apm]` (`agent-plugin` is the sole selector for the Agent Plugin bundle; `plugin` is a compatibility alias for the Claude plugin bundle, not for `agent-plugin`; `claude`/`claude-plugin` also select the Claude plugin bundle; `apm` selects the legacy APM layout; default `claude-plugin`), `--claude-plugin` (shortcut for `--format claude-plugin`; passing more than one of `--claude-plugin`/`--format` is a usage error), `--force`, `--offline`, `--include-prerelease`, `--marketplace=FORMATS`, `--marketplace-path FORMAT=PATH`, `--json`, `--check-versions` (release gate: per-package versions match `marketplace.versioning.strategy`; exit 3 on failure), `--check-clean` (read-only release gate: regenerate-and-diff against the effective marketplace path, including `--marketplace-path` overrides; never writes pack outputs; exit 4 on drift). `-t/--target` is **deprecated** but still filters legacy APM paths. Exit codes: `0` success, `1` build/runtime error, `2` schema validation error, `3` `--check-versions` misalignment, `4` `--check-clean` drift. | | `apm unpack BUNDLE` | **[Deprecated]** Extract a bundle. Use `apm install ` instead -- it deploys directly with integrity verification and target resolution. | `-o PATH`, `--skip-verify`, `--force`, `--dry-run` | For marketplace publishing, `--strict-metadata` preflights every selected diff --git a/scripts/architecture_linter/checks/marketplace_package_and_registration.py b/scripts/architecture_linter/checks/marketplace_package_and_registration.py index 26ad48a3a3..1a85474b21 100644 --- a/scripts/architecture_linter/checks/marketplace_package_and_registration.py +++ b/scripts/architecture_linter/checks/marketplace_package_and_registration.py @@ -7,6 +7,7 @@ from __future__ import annotations +import ast import re from collections.abc import Iterable @@ -271,11 +272,12 @@ def _check_legacy_skill_membership(provider: FactsProvider) -> tuple[Violation, _PLUGIN_LAYOUT = "src/apm_cli/bundle/plugin_layout.py" _LOCAL_BUNDLE_PATHS = "src/apm_cli/install/local_bundle_paths.py" _INSTALL_SERVICES = "src/apm_cli/install/services.py" +_PACK_ENRICHMENT = "src/apm_cli/bundle/lockfile_enrichment.py" _TARGET_NAME_COMPARISON = re.compile(r"\btarget\.name\s*(?:==|!=)|(?:==|!=)\s*target\.name\b") def _check_bundle_native_layout(provider: FactsProvider) -> tuple[Violation, ...]: - """Local-bundle layout lowering must stay target-profile driven.""" + """Bundle layout lowering and pack eligibility stay target-profile driven.""" inv = frozenset(provider.inventory) findings: list[Violation] = [] findings.extend( @@ -324,6 +326,37 @@ def _check_bundle_native_layout(provider: FactsProvider) -> tuple[Violation, ... line=number, ) ) + facts, failures = checked_facts( + provider, _PACK_ENRICHMENT, _RID_BUNDLE_LAYOUT, require_python=True + ) + findings.extend(failures) + if not failures and facts.tree_index is not None: + for name in ("_all_target_prefixes", "_get_target_prefixes"): + function = facts.tree_index.function(name) + nodes = facts.tree_index.own_scope(function) if function is not None else () + attributes = {node.attr for node in nodes if isinstance(node, ast.Attribute)} + hardcoded_prefix = any( + isinstance(node, ast.Constant) + and isinstance(node.value, str) + and re.fullmatch(r"\.[^\s]+/", node.value) + for node in nodes + ) + if ( + "effective_pack_prefixes" not in attributes + or hardcoded_prefix + or attributes.intersection( + {"prefix", "root_dir", "deploy_root", "subdir", "pack_prefixes", "primitives"} + ) + ): + findings.append( + violation( + _RID_BUNDLE_LAYOUT, + _PACK_ENRICHMENT, + f"{name} must consume TargetProfile.effective_pack_prefixes, " + "not re-derive pack eligibility from target roots or primitives", + line=getattr(function, "lineno", 1), + ) + ) return tuple(findings) @@ -454,7 +487,7 @@ def _check_copilot_ownership(provider: FactsProvider) -> tuple[Violation, ...]: id=_RID_BUNDLE_LAYOUT, group=GROUP, guard_ids=(_RID_BUNDLE_LAYOUT,), - description="Local bundle layout lowering stays owned by bundle/plugin_layout.py.", + description="Bundle layout and pack eligibility route through plugin_layout and TargetProfile.", check=_check_bundle_native_layout, ), Rule( diff --git a/src/apm_cli/integration/targets.py b/src/apm_cli/integration/targets.py index a327df2164..dd638292a6 100644 --- a/src/apm_cli/integration/targets.py +++ b/src/apm_cli/integration/targets.py @@ -20,7 +20,7 @@ from collections.abc import Callable from dataclasses import dataclass -from pathlib import Path +from pathlib import Path, PurePosixPath from apm_cli.core.target_catalog import ( TARGET_CAPABILITIES, @@ -247,10 +247,9 @@ class TargetProfile: pack_prefixes: tuple[str, ...] = () """Path prefixes that identify this target's deployed files when packing. - When empty, ``bundle.lockfile_enrichment`` derives ``(f"{root_dir}/",)`` - from :attr:`root_dir`. Override only when the target deploys to multiple - top-level directories (e.g. Codex deploys both ``.codex/`` and - ``.agents/``). + When empty, :attr:`effective_pack_prefixes` derives the target root plus + any overridden primitive directories. Explicit prefixes remain authoritative + for targets that need a custom packing surface. """ hooks_config_display: str | None = None @@ -300,10 +299,19 @@ def prefix(self) -> str: def effective_pack_prefixes(self) -> tuple[str, ...]: """Return the path prefixes used by pack-time file filtering. - Falls back to ``(self.prefix,)`` when :attr:`pack_prefixes` is empty, - so most targets need not override the field explicitly. + Default prefixes cover the root and each overridden primitive directory, + not its entire shared root (which may contain another client's hooks or + plugins). Explicit :attr:`pack_prefixes` retain their configured semantics. """ - return self.pack_prefixes if self.pack_prefixes else (self.prefix,) + if self.pack_prefixes: + return self.pack_prefixes + prefixes = {self.prefix: None} + for mapping in self.primitives.values(): + if mapping.deploy_root is not None: + prefix = f"{(PurePosixPath(mapping.deploy_root) / mapping.subdir).as_posix()}/" + if not prefix.startswith(self.prefix): + prefixes[prefix] = None + return tuple(prefixes) def supports(self, primitive: str) -> bool: """Return ``True`` if this target accepts *primitive*.""" diff --git a/tests/integration/test_architecture_install_compound_mutations.py b/tests/integration/test_architecture_install_compound_mutations.py index cf7048ade6..45331ba590 100644 --- a/tests/integration/test_architecture_install_compound_mutations.py +++ b/tests/integration/test_architecture_install_compound_mutations.py @@ -38,8 +38,39 @@ def _replace(old: str, new: str) -> tuple[tuple[str, str], ...]: REPLACEMENT_RULE = "install-deployment-resolution-replacement" TARGET_RULE = "install-deployment-package-target-authorization" REQUEST_DEFAULTS_RULE = "install-deployment-request-defaults" +BUNDLE_LAYOUT_RULE = "install-deployment-bundle-native-layout" MUTATIONS: tuple[CompoundMutation, ...] = ( + *( + CompoundMutation( + f"pack-prefix-owner-{name}", + BUNDLE_LAYOUT_RULE, + "src/apm_cli/bundle/lockfile_enrichment.py", + _replace(old, new), + ) + for name, old, new in ( + ( + "all-targets", + "for prefix in profile.effective_pack_prefixes:", + "for prefix in (profile.prefix,):", + ), + ( + "named-target", + "return list(profile.effective_pack_prefixes)", + "return [profile.prefix]", + ), + ( + "alias-target", + 'return list(KNOWN_TARGETS["copilot"].effective_pack_prefixes)', + 'return [KNOWN_TARGETS["copilot"].prefix]', + ), + ( + "hardcoded-target", + "return list(profile.effective_pack_prefixes)", + 'return [".github/"]', + ), + ) + ), CompoundMutation( "target-owner-ignores-nested-mask", TARGET_RULE, diff --git a/tests/integration/test_pack_shared_skill_roundtrip.py b/tests/integration/test_pack_shared_skill_roundtrip.py new file mode 100644 index 0000000000..76424e119f --- /dev/null +++ b/tests/integration/test_pack_shared_skill_roundtrip.py @@ -0,0 +1,105 @@ +"""Hermetic install -> APM ZIP -> restore proof for split-root target layouts.""" + +from __future__ import annotations + +import hashlib +import os +import zipfile +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml +from click.testing import CliRunner + +from apm_cli.cli import cli +from apm_cli.deps.lockfile import LockFile +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.local_git_repository import LocalGitRepositoryFactory +from tests.utils.local_package import LocalPackageFactory + +pytestmark = pytest.mark.component + + +def test_install_pack_zip_restore_preserves_shared_skill(tmp_path: Path) -> None: + """Pack installed bytes (including nested assets) and their real install hashes.""" + isolated = IsolatedApmEnvironment.create(tmp_path / "isolated", base_env=os.environ) + packages = LocalPackageFactory(isolated.package_root) + source = packages.create("review-kit", targets=("copilot",)) + skill = packages.add_skill( + source, + "review", + "---\nname: review\ndescription: Review code using the bundled rules\n---\n" + "# Review\nRead [rules](assets/rules.json).\n", + ) + asset = skill.parent / "assets" / "rules.json" + asset.parent.mkdir() + asset.write_bytes(b'{"rules": ["check tests"]}\n') + packages.add_agent( + source, "reviewer", "---\ndescription: Review changes\n---\n# Reviewer\nCheck tests.\n" + ) + packages.add_instruction( + source, "style", "---\napplyTo: '**/*.py'\n---\n# Style\nUse type hints.\n" + ) + repositories = LocalGitRepositoryFactory( + isolated.repository_root, env=isolated.subprocess_env() + ) + repository = repositories.create(source.name, source_tree=source.root) + repositories.commit(repository, message="seed pack roundtrip fixture") + remote = "https://github.com/fixture/review-kit" + environment = repositories.url_rewrite_subprocess_env(repository, remote) + projects = LocalPackageFactory(isolated.work_root) + producer = projects.create("producer", dependencies=({"git": remote},), targets=("copilot",)) + consumer = projects.create("consumer", targets=("copilot",)) + runner = CliRunner() + + # Only local file:// Git transport is permitted; any attempted HTTP is denied. + # Keep the actual resolver, integrators, hashing, packer and restore path real. + with ( + patch.dict(os.environ, environment, clear=True), + patch("requests.sessions.Session.request", side_effect=OSError("HTTP disabled in test")), + pytest.MonkeyPatch.context() as monkeypatch, + ): + monkeypatch.chdir(producer.root) + installed = runner.invoke(cli, ["install", "--target", "copilot", "--no-policy"]) + assert installed.exit_code == 0, installed.output + lockfile = LockFile.read(producer.root / "apm.lock.yaml") + assert lockfile is not None + dependencies = lockfile.get_all_dependencies() + assert len(dependencies) == 1 + dependency = dependencies[0] + assert dependency.source != "local" + skill_dir = ".agents/skills/review/" + assert skill_dir.rstrip("/") in dependency.deployed_files + hashes = dependency.deployed_file_hashes + expected_files = {path: (producer.root / path).read_bytes() for path in hashes} + assert {f"{skill_dir}SKILL.md", f"{skill_dir}assets/rules.json"} <= expected_files.keys() + assert any(path.startswith(".github/agents/") for path in expected_files) + assert any(path.startswith(".github/instructions/") for path in expected_files) + for path, content in expected_files.items(): + assert hashes[path] == f"sha256:{hashlib.sha256(content).hexdigest()}" + + packed = runner.invoke(cli, ["pack", "--format", "apm", "--archive", "--target", "copilot"]) + assert packed.exit_code == 0, packed.output + archives = list((producer.root / "build").glob("*.zip")) + assert len(archives) == 1 + with zipfile.ZipFile(archives[0]) as archive: + lock_names = [name for name in archive.namelist() if name.endswith("/apm.lock.yaml")] + assert len(lock_names) == 1 + prefix = lock_names[0].removesuffix("apm.lock.yaml") + packed_lock = yaml.safe_load(archive.read(lock_names[0])) + packed_dep = packed_lock["dependencies"][0] + assert skill_dir.rstrip("/") in packed_dep["deployed_files"] + assert packed_dep["deployed_file_hashes"] == hashes + for path, content in expected_files.items(): + assert archive.read(prefix + path) == content + + monkeypatch.chdir(consumer.root) + # APM-format archives use unpack's project-relative restore path; + # imperative `install ` accepts plugin-format bundles only. + restored = runner.invoke(cli, ["unpack", str(archives[0])]) + assert restored.exit_code == 0, restored.output + for path, content in expected_files.items(): + restored_content = (consumer.root / path).read_bytes() + assert restored_content == content + assert f"sha256:{hashlib.sha256(restored_content).hexdigest()}" == hashes[path] diff --git a/tests/unit/integration/test_targets_registry_completeness.py b/tests/unit/integration/test_targets_registry_completeness.py index 47d5a5269d..c0cd36e782 100644 --- a/tests/unit/integration/test_targets_registry_completeness.py +++ b/tests/unit/integration/test_targets_registry_completeness.py @@ -13,6 +13,8 @@ from __future__ import annotations +from dataclasses import replace + import pytest from apm_cli.adapters.client.antigravity import AntigravityClientAdapter @@ -28,7 +30,7 @@ from apm_cli.adapters.client.opencode import OpenCodeClientAdapter from apm_cli.adapters.client.vscode import VSCodeClientAdapter from apm_cli.adapters.client.windsurf import WindsurfClientAdapter -from apm_cli.integration.targets import KNOWN_TARGETS, TargetProfile +from apm_cli.integration.targets import KNOWN_TARGETS, PrimitiveMapping, TargetProfile # Recognised values for ``TargetProfile.compile_family``. Adding a new family # requires touching ``apm_cli.commands.compile.cli._resolve_compile_target`` @@ -70,9 +72,7 @@ def test_pack_prefixes_are_resolvable(name: str, profile: TargetProfile) -> None: """Every target must yield a non-empty pack-prefix tuple. - ``effective_pack_prefixes`` falls back to ``(profile.prefix,)`` when - ``pack_prefixes`` is empty, so this test fails only when both the - field AND the fallback are degenerate. + Defaults include the target root and overridden primitive directories. """ prefixes = profile.effective_pack_prefixes assert prefixes, f"target {name!r} has no pack prefixes" @@ -86,6 +86,38 @@ def test_pack_prefixes_are_resolvable(name: str, profile: TargetProfile) -> None ) +@pytest.mark.parametrize("name,profile", sorted(KNOWN_TARGETS.items())) +def test_pack_prefixes_cover_primitive_deploy_roots(name: str, profile: TargetProfile) -> None: + """Every deployed primitive must survive its own target's pack filter.""" + for mapping in profile.primitives.values(): + directory = f"{mapping.deploy_root or profile.root_dir}/{mapping.subdir}".rstrip("/") + "/" + assert any(directory.startswith(prefix) for prefix in profile.effective_pack_prefixes), ( + f"{name} drops deployed primitive directory {directory}" + ) + + +def test_default_pack_prefixes_derive_narrow_deduplicated_overrides() -> None: + """Derivation is generic, bounded to primitive dirs, and stable in order.""" + profile = replace( + KNOWN_TARGETS["copilot"], + root_dir=".client", + primitives={ + "skills": PrimitiveMapping("skills", "/SKILL.md", "skill_standard", ".shared"), + "other-skills": PrimitiveMapping("skills", "/SKILL.md", "skill_standard", ".shared"), + "agents": PrimitiveMapping("agents", ".md", "agent", ".client"), + "commands": PrimitiveMapping("commands", ".md", "command", ".extra/"), + }, + ) + assert profile.effective_pack_prefixes == (".client/", ".shared/skills/", ".extra/commands/") + + +def test_explicit_pack_prefixes_remain_authoritative() -> None: + """Configured prefixes are not narrowed, reordered, or augmented.""" + configured = (".legacy/", ".agents/") + profile = replace(KNOWN_TARGETS["copilot"], pack_prefixes=configured) + assert profile.effective_pack_prefixes == configured + + @pytest.mark.parametrize("name,profile", sorted(KNOWN_TARGETS.items())) def test_compile_family_is_recognised(name: str, profile: TargetProfile) -> None: """A target's ``compile_family`` must be ``None`` or a recognised family. diff --git a/tests/unit/test_lockfile_enrichment.py b/tests/unit/test_lockfile_enrichment.py index 3d8872933e..ebecb82d43 100644 --- a/tests/unit/test_lockfile_enrichment.py +++ b/tests/unit/test_lockfile_enrichment.py @@ -453,6 +453,43 @@ def test_filter_files_windsurf_includes_agents_skills_prefix(self): class TestFilterFilesByTargetList: """Tests for _filter_files_by_target with list targets.""" + @pytest.mark.parametrize( + "target", + ["copilot", "vscode", ["copilot"], ["vscode"], ["copilot", "vscode"]], + ) + def test_shared_skills_survive_without_cross_target_leakage( + self, target: str | list[str] + ) -> None: + """Copilot aliases and list targets retain only the shared skill subtree.""" + from apm_cli.bundle.lockfile_enrichment import _filter_files_by_target + + expected = [ + ".github/agents/reviewer.agent.md", + ".github/instructions/style.instructions.md", + ".agents/skills/review/", + ".agents/skills/review/SKILL.md", + ".agents/skills/review/assets/rules.json", + ] + unrelated = [ + ".agents/hooks/other-client.json", + ".agents/plugins/other/plugin.json", + ".agents/skills-extra/secret.txt", + ".agents/agents/other.md", + ".claude/settings.json", + ".cursor/hooks.json", + ] + assert _filter_files_by_target(expected + unrelated, target) == (expected, {}) + + @pytest.mark.parametrize("target", ["cursor", "gemini", "opencode"]) + def test_other_default_profiles_retain_shared_skills(self, target: str) -> None: + """All default-prefix profiles honor primitive overrides, not just Copilot.""" + from apm_cli.bundle.lockfile_enrichment import _filter_files_by_target + + skill = ".agents/skills/review/assets/rules.json" + assert _filter_files_by_target( + [skill, ".agents/hooks/other.json", ".agents/plugins/other/plugin.json"], target + ) == ([skill], {}) + def test_list_claude_copilot_includes_both_prefixes(self): from apm_cli.bundle.lockfile_enrichment import _filter_files_by_target diff --git a/tests/unit/test_shared_apm_workflow_contract.py b/tests/unit/test_shared_apm_workflow_contract.py index 6f548bf493..f3ac8ea61e 100644 --- a/tests/unit/test_shared_apm_workflow_contract.py +++ b/tests/unit/test_shared_apm_workflow_contract.py @@ -8,6 +8,7 @@ import shutil import subprocess import sys +import zipfile from pathlib import Path from unittest.mock import patch @@ -17,6 +18,14 @@ ROOT = Path(__file__).resolve().parents[2] SHARED_APM = ROOT / ".github" / "workflows" / "shared" / "apm.md" VERIFY_SHARED_APM = ROOT / ".github" / "workflows" / "verify-shared-apm-matrix.yml" +REVIEW_PANEL = ROOT / ".github" / "workflows" / "pr-review-panel.md" +PANEL_GATE_NAME = "Verify restored review panel skill and resources" +PANEL_FILES = ( + "SKILL.md", + "assets/panelist-return-schema.json", + "assets/ceo-return-schema.json", + "assets/recommendation-template.md", +) GH_AW_GUIDE = ROOT / "docs" / "src" / "content" / "docs" / "integrations" / "gh-aw.md" GH_AW_ACTIONS_LOCK = ROOT / ".github" / "aw" / "actions-lock.json" GH_AW_MAINTENANCE = ROOT / ".github" / "workflows" / "agentics-maintenance.yml" @@ -80,8 +89,8 @@ } -def _frontmatter() -> dict: - source = SHARED_APM.read_text(encoding="utf-8") +def _frontmatter(path: Path = SHARED_APM) -> dict: + source = path.read_text(encoding="utf-8") _prefix, frontmatter, _body = source.split("---", 2) loaded = yaml.safe_load(frontmatter) assert isinstance(loaded, dict) @@ -364,6 +373,204 @@ def test_verify_workflow_exercises_apm_028_pack_and_multibundle_restore() -> Non assert "steps.restore.outputs.bundles-restored" in VERIFY_SHARED_APM.read_text(encoding="utf-8") +def test_review_panel_pins_temporary_skills_pack_workaround() -> None: + imported = next( + entry for entry in _frontmatter(REVIEW_PANEL)["imports"] if entry["uses"] == "shared/apm.md" + ) + assert imported["with"] == { + "apm-version": "0.30.0", + "target": "copilot,agent-skills", + "packages": ["microsoft/apm#main"], + } + assert _frontmatter()["import-schema"]["apm-version"]["default"] == DEFAULT_APM_VERSION + + +def _panel_gate(compiled: bool = False) -> dict: + if compiled: + lock = yaml.safe_load(REVIEW_PANEL.with_suffix(".lock.yml").read_text(encoding="utf-8")) + steps = lock["jobs"]["agent"]["steps"] + else: + steps = _frontmatter(REVIEW_PANEL)["pre-agent-steps"] + return next(step for step in steps if step.get("name") == PANEL_GATE_NAME) + + +def test_review_panel_gate_runs_after_restore_before_agent_execution() -> None: + lock = yaml.safe_load(REVIEW_PANEL.with_suffix(".lock.yml").read_text(encoding="utf-8")) + steps = lock["jobs"]["agent"]["steps"] + names = [step.get("name") for step in steps] + assert names.count(PANEL_GATE_NAME) == 1 + assert ( + names.index("Restore APM packages (all bundles)") + < names.index(PANEL_GATE_NAME) + < names.index("Execute GitHub Copilot CLI") + ) + gate = _panel_gate(compiled=True) + assert gate == _panel_gate() + assert gate["shell"] == "bash" + assert "if" not in gate + assert "continue-on-error" not in gate + + +@pytest.mark.parametrize("compiled", [False, True], ids=["source", "lock"]) +@pytest.mark.parametrize("required_file", PANEL_FILES) +@pytest.mark.parametrize("defect", ["absent", "empty", "directory"]) +def test_review_panel_gate_rejects_incomplete_payload( + tmp_path: Path, compiled: bool, required_file: str, defect: str +) -> None: + skill = tmp_path / ".agents" / "skills" / "apm-review-panel" + for relative in PANEL_FILES: + path = skill / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(f"fixture: {relative}\n", encoding="utf-8") + broken = skill / required_file + broken.unlink() + if defect == "empty": + broken.touch() + elif defect == "directory": + broken.mkdir() + + result = _run_bash(_panel_gate(compiled)["run"], env={"GITHUB_WORKSPACE": tmp_path.as_posix()}) + + assert result.returncode == 1, result.stdout + result.stderr + assert f".agents/skills/apm-review-panel/{required_file}" in result.stdout + assert "::error::" in result.stdout + + +@pytest.mark.parametrize("compiled", [False, True], ids=["source", "lock"]) +def test_review_panel_gate_accepts_complete_payload(tmp_path: Path, compiled: bool) -> None: + skill = tmp_path / ".agents" / "skills" / "apm-review-panel" + for relative in PANEL_FILES: + path = skill / relative + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(f"fixture: {relative}\n".encode()) + + result = _run_bash(_panel_gate(compiled)["run"], env={"GITHUB_WORKSPACE": tmp_path.as_posix()}) + + assert result.returncode == 0, result.stdout + result.stderr + + +def _panel_compat_job() -> dict: + workflow = yaml.safe_load(VERIFY_SHARED_APM.read_text(encoding="utf-8")) + return workflow["jobs"]["c-apm-030-panel-compat"] + + +def test_verify_workflow_roundtrips_real_panel_without_checkout_or_secrets() -> None: + workflow = yaml.safe_load(VERIFY_SHARED_APM.read_text(encoding="utf-8")) + job = _panel_compat_job() + assert workflow["permissions"] == {"contents": "read"} + assert job["permissions"] == {"contents": "read"} + assert job["timeout-minutes"] == 10 + steps = job["steps"] + actions = [step for step in steps if "uses" in step] + assert len(actions) == 2 + assert {step["uses"] for step in actions} == {f"microsoft/apm-action@{APM_ACTION_SHA}"} + pack, restore = actions + assert pack["with"] == { + "apm-version": "0.30.0", + "dependencies": "- microsoft/apm#main\n", + "target": "copilot,agent-skills", + "isolated": "true", + "pack": "true", + "archive": "true", + "working-directory": "${{ runner.temp }}/apm-030-panel-pack", + } + assert restore["with"] == { + "apm-version": "0.30.0", + "bundle": "${{ steps.pack-panel.outputs.bundle-path }}", + "working-directory": "${{ runner.temp }}/apm-030-panel-restore", + } + assert [step["name"] for step in steps] == [ + "Pack real review panel with APM 0.30", + "Require a fresh panel restore destination", + "Restore review panel bundle with APM 0.30", + "Compare installed, archived, and restored panel bytes", + ] + assert "secrets." not in json.dumps(job) + assert steps[-1]["shell"] == "python" + assert steps[-1]["env"] == { + "PACK_ROOT": "${{ runner.temp }}/apm-030-panel-pack", + "BUNDLE": "${{ steps.pack-panel.outputs.bundle-path }}", + "RESTORE_ROOT": "${{ runner.temp }}/apm-030-panel-restore", + } + + +@pytest.mark.parametrize("existing_destination", [False, True]) +def test_panel_compat_requires_an_unpopulated_restore_destination( + tmp_path: Path, existing_destination: bool +) -> None: + destination = tmp_path / "restore" + if existing_destination: + destination.mkdir() + step = _panel_compat_job()["steps"][1] + assert step["env"] == {"RESTORE_ROOT": "${{ runner.temp }}/apm-030-panel-restore"} + + result = _run_bash(step["run"], env={"RESTORE_ROOT": destination.as_posix()}) + + assert result.returncode == int(existing_destination), result.stdout + result.stderr + + +@pytest.mark.parametrize( + ("stage", "defect", "required_file"), + [ + (stage, defect, relative) + for stage in ("archive", "restore") + for defect in ("absent", "empty", "changed") + for relative in PANEL_FILES + ] + + [(None, None, None)], +) +def test_panel_compat_checks_exact_resource_bytes( + tmp_path: Path, stage: str | None, defect: str | None, required_file: str | None +) -> None: + pack_root, restore_root = tmp_path / "pack", tmp_path / "restore" + bundle = tmp_path / "panel.zip" + with zipfile.ZipFile(bundle, "w") as archive: + archive.writestr("panel-1.0.0/apm.lock.yaml", "lockfile_version: '1'\n") + for relative in PANEL_FILES: + deployed = Path(".agents/skills/apm-review-panel") / relative + expected = f"real-panel-fixture: {relative}\n".encode() + for root in (pack_root, restore_root): + path = root / deployed + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(expected) + damaged = b"" if defect == "empty" else b"not the installed resource\n" + if stage == "restore" and relative == required_file: + path = restore_root / deployed + path.unlink() + if defect != "absent": + path.write_bytes(damaged) + if stage == "archive" and relative == required_file: + if defect != "absent": + archive.writestr(f"panel-1.0.0/{deployed.as_posix()}", damaged) + else: + archive.writestr(f"panel-1.0.0/{deployed.as_posix()}", expected) + + step = _panel_compat_job()["steps"][-1] + result = subprocess.run( + (sys.executable, "-c", step["run"]), + env={ + **os.environ, + "PACK_ROOT": str(pack_root), + "RESTORE_ROOT": str(restore_root), + "BUNDLE": str(bundle), + }, + capture_output=True, + text=True, + check=False, + timeout=30, + ) + + if stage is None: + assert result.returncode == 0, result.stdout + result.stderr + assert ( + "4 review panel files survived install, pack, and restore byte-for-byte" + in result.stdout + ) + else: + assert result.returncode != 0 + assert required_file in result.stdout + result.stderr + + def test_shared_apm_fallback_token_has_current_repo_read_only() -> None: frontmatter = _frontmatter() apm_prep = frontmatter["jobs"]["apm-prep"] @@ -711,8 +918,10 @@ def test_compiled_consumer_locks_carry_target_validation() -> None: assert compiled["jobs"]["apm"]["permissions"] == {"contents": "read"} -def test_compiled_consumers_pin_the_shared_runtime_default() -> None: - for path, _imported in _shared_apm_consumers(): +def test_compiled_consumers_pin_their_effective_runtime_version() -> None: + for path, imported in _shared_apm_consumers(): + expected_version = imported.get("with", {}).get("apm-version", DEFAULT_APM_VERSION) + assert re.fullmatch(r"\d+\.\d+\.\d+", expected_version), path.name lock = yaml.safe_load(path.with_suffix(".lock.yml").read_text(encoding="utf-8")) action_steps = [ step @@ -721,9 +930,11 @@ def test_compiled_consumers_pin_the_shared_runtime_default() -> None: if step.get("uses") == f"microsoft/apm-action@{APM_ACTION_SHA}" ] assert len(action_steps) == 2, path.name - assert {step["with"]["apm-version"] for step in action_steps} == {DEFAULT_APM_VERSION}, ( + assert {step["with"]["apm-version"] for step in action_steps} == {expected_version}, ( path.name ) + pack = next(step for step in action_steps if step["with"].get("pack") == "true") + assert pack["with"]["target"] == imported["with"]["target"], path.name def test_repository_pins_exact_gh_aw_compiler_and_generated_locks() -> None: From 1b83ebb1e34afd27585b38ae40739a49a81bad75 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Wed, 9 Sep 2026 11:23:51 +0200 Subject: [PATCH 2/5] Preserve structured panel delivery and correct legacy bundle handoff Carry the distinct #1844 safeguard into the canonical panel package and deployed ledger without recreating historical mirrors. Keep safe outputs fail-closed, prove transport clauses and provenance, and distinguish legacy unpack from plugin install in the actual pack handoff. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .agents/skills/apm-review-panel/SKILL.md | 280 ++++++------------ apm.lock.yaml | 4 +- docs/src/content/docs/reference/cli/pack.md | 8 +- .../.apm/skills/apm-usage/commands.md | 2 +- packages/apm-review-panel/SKILL.md | 280 ++++++------------ src/apm_cli/commands/pack.py | 13 +- .../test_pack_shared_skill_roundtrip.py | 3 + tests/unit/commands/test_pack_cli_surface.py | 12 +- tests/unit/commands/test_pack_phase3.py | 4 +- .../test_review_panel_transport_contract.py | 79 +++++ 10 files changed, 301 insertions(+), 384 deletions(-) create mode 100644 tests/unit/test_review_panel_transport_contract.py diff --git a/.agents/skills/apm-review-panel/SKILL.md b/.agents/skills/apm-review-panel/SKILL.md index 403845415f..70e7aff024 100644 --- a/.agents/skills/apm-review-panel/SKILL.md +++ b/.agents/skills/apm-review-panel/SKILL.md @@ -18,64 +18,39 @@ description: >- # APM Review Panel - Fan-Out Advisory Review -The panel is FAN-OUT + SYNTHESIZER. Each persona runs in its own agent -thread (via the `task` tool) and returns JSON matching -`assets/panelist-return-schema.json`. The orchestrator schema-validates -each return, hands all returns to the apm-ceo synthesizer (also a task -thread, returns JSON matching `assets/ceo-return-schema.json`), then -renders ONE recommendation comment from `assets/recommendation-template.md`. - -This skill is ADVISORY by design. It does not compute a binary verdict, it -does not apply verdict labels, and it does not gate merge. The panel -surfaces findings; the maintainer and the PR author decide ship. +FAN-OUT + SYNTHESIZER: each persona returns schema-valid JSON in its +own `task` thread; apm-ceo synthesizes in another. The orchestrator +renders ONE advisory comment. The maintainer and PR author decide ship. ## Architecture invariants -- **Advisory regime, not gate regime.** There is no `APPROVE` / `REJECT`, - no `panel-approved` / `panel-rejected` label, no deterministic verdict - computation. The CEO returns a `ship_recommendation.stance` (`ship_now` +- **Advisory, never a gate.** No binary verdict computation, `APPROVE` / + `REJECT`, or verdict labels. The CEO's `ship_recommendation.stance` (`ship_now` / `ship_with_followups` / `needs_discussion` / `needs_rework`); this is - prose for the human reviewer, never auto-applied as a label or status - check. This is the architectural fix for the previous regime's - over-strictness: removing the binary gate removes the incentive for - panelists to inflate `required[]` defensively. + prose for humans, never a label or status check. - **Three severity buckets, none of them gate.** Findings carry `severity: blocking | recommended | nit`. `blocking` is the highest signal a panelist can send and renders prominently in the comment; it still does not block merge. `recommended` is the default for substantive feedback. `nit` is one-line polish. The orchestrator never reads severity to gate anything. -- **Single-writer interlock.** Only the orchestrator writes to the PR: - exactly one `add-comment` and one `remove-labels` call. The - `remove-labels` call always sweeps `panel-review` (trigger - idempotency) AND defensively removes `panel-approved` / - `panel-rejected` if present (legacy verdict labels from the - pre-advisory regime; they have no meaning here and would mislead - readers if left on a PR after a fresh advisory pass). NO `add-labels` - call -- there are no verdict labels to apply. Panelist subagents and - the CEO subagent return JSON only and MUST NOT call any `gh` write - command, post comments, apply labels, or touch the PR state. +- **Single-writer interlock.** Only the orchestrator emits one comment + and one label sweep using step 7's transport. Sweep `panel-review` + (trigger reset), `panel-approved`, and `panel-rejected` (obsolete, + misleading verdict labels). NO `add-labels`. Panelists and CEO return + JSON only: no `gh` writes, comments, labels, or PR-state mutations. - **Single-emission discipline.** Exactly one comment per panel run, rendered from `assets/recommendation-template.md` after all subagents return. -- **Non-empty turn exit (the run's hard contract).** gh-aw decides - success by inspecting `agent_output` AFTER your turn ends: a turn that - ends with zero safe outputs (`agent_output = {"items":[]}`) is detected - as a failure, the safe-output detection job is skipped, the - `add-comment` job never runs, and the workflow opens a "No Safe Outputs - Generated" issue. Therefore your turn MUST end with at least one safe - output -- the rendered comment on success (step 7), or an explicit - `noop` if the run genuinely cannot produce one. NEVER end the turn - empty. -- **Synchronous fan-out -- never spawn-and-forget.** Every `task` spawn - (each panelist AND the CEO synthesizer) is BLOCKING: spawn it, WAIT for - its JSON return, then continue. Use the `task` tool's synchronous mode; - do NOT use its background/detached mode -- the variant that returns an - `agent_id` immediately and runs the subagent in the background -- for - any panelist or the CEO. Their returns are LOAD-BEARING: the comment - cannot be rendered without them. Spawning the CEO (or a panelist) - detached and then ending the turn while it is still running is the - documented cause of the empty-output failure above. +- **Non-empty turn exit.** In gh-aw, zero safe outputs + (`agent_output = {"items":[]}`) cause "No Safe Outputs Generated": + detection is skipped and `add-comment` never runs. Emit the comment + or, if genuinely impossible, explicit `noop`; never end empty. + Step 9 scopes verification to the selected transport. +- **Synchronous fan-out -- never spawn-and-forget.** Every panelist and + CEO `task` MUST use synchronous mode and be awaited for its JSON. + Never use background/detached mode returning an `agent_id`, or end + the turn while a child runs: their returns are required for rendering. ## Agent roster @@ -95,39 +70,9 @@ surfaces findings; the maintainer and the PR author decide ship. ## Topology ``` - apm-review-panel SKILL (orchestrator thread) - | - FAN-OUT via task tool (panelists in parallel) - | - +-----+-------+-------+-----+-----+------+-----------+----------+ - v v v v v v v v v (cond.) - py cli dx-ux sec grw auth doc-writer test-cov - | | | | | | | | - | each returns JSON per panelist-return-schema.json - +-----+-------+-------+-----+-----+------+-----------+----------+ - | - v <-- S4 schema-validate - v <-- on malformed: re-spawn that persona - v - task: apm-ceo synthesizer - - aggregates findings across panelists - - resolves dissent - - emits headline + arbitration prose + principle alignment - - emits curated recommended_followups (prioritized) - - emits ship_recommendation (stance + prose) - - returns ceo-return-schema.json - | - v <-- S4 schema-validate - v - orchestrator (sole writer) - | | - v v - add-comment remove-labels - (max:2) [panel-review, - panel-approved, - panel-rejected] - (trigger reset + - legacy verdict sweep) +orchestrator -> nine parallel panelist tasks -> S4 panelist schema gate + -> apm-ceo task (aggregate, arbitrate dissent, curate follow-ups) + -> S4 CEO schema gate -> orchestrator: one comment + one label sweep ``` ## Conditional panelists @@ -178,14 +123,10 @@ change user-facing documentation, agent or skill prose, instruction files, CHANGELOG entries, README claims, or any natural-language artifact a reader will rely on? If unsure, answer YES." -When the doc-writer is active and the PR includes documentation changes, -the persona reviews them for: (a) consistency with the existing voice -and structure, (b) accuracy against the code being changed, (c) -completeness for the typical reader (no orphan claims, no missing -prerequisites), (d) discoverability (cross-links, sidebar order if -Starlight content). When the doc-writer is active because of code -changes that SHOULD have updated docs but did not, the persona surfaces -that gap as a finding. +When active, doc-writer checks changed docs for voice/structure +consistency, code accuracy, completeness (no orphan claims or missing +prerequisites), and discoverability (cross-links, Starlight sidebar +order). Surface missing doc updates required by code changes as findings. ### Performance Expert @@ -241,32 +182,21 @@ documentation-only PR -- the diff contains zero `src/**/*.py` files. In that case set `inactive_reason: "documentation-only PR -- no runtime code paths to defend"`. -The activation rule is intentionally narrow: under the advisory regime, -test outcomes are LOAD-BEARING for CEO arbitration (passed / failed / -missing test evidence outranks opinion-only findings -- see -`apm-ceo.agent.md` and `panelist-return-schema.json` evidence block). -A persona whose findings carry that weight cannot be silently skipped -on a heuristic. Better to spawn it on a pure refactor and have it -return a single `nit`-severity "no behavior surface touched -- no -coverage finding" line than to skip it and leave the CEO without -evidence to weigh. (Earlier revisions of this skill paired test-coverage -with auth and doc-writer as conditional for symmetry; that symmetry -broke when test evidence became load-bearing.) - -The test-coverage-expert is paired with the devx-ux-expert lens and -defends the user-promise contracts the DevX persona enumerates (CLI -surface, error wording, install idempotency, lockfile determinism, auth -resolution). It MUST verify "no test exists" claims with `view`/`grep` -on the test tree before emitting a finding -- false-positive coverage -findings destroy trust in the field. It does NOT compute coverage -percentages, does NOT flag tests for pure refactors, and does NOT -duplicate python-architect on test-code design. +Test evidence (passed / failed / missing) outranks opinion in CEO +arbitration; see `apm-ceo.agent.md` and the panelist schema evidence +block. Never skip this persona heuristically. On a pure refactor, +return a `nit` "no behavior surface touched -- no coverage finding" +rather than leaving the CEO without evidence. + +Paired with devx-ux-expert, it defends CLI surface, error wording, +install idempotency, lockfile determinism, and auth resolution. +Verify "no test exists" with `view`/`grep` on the test tree before +reporting. No coverage percentages, pure-refactor test findings, or +duplication of python-architect's test-code design review. ## Routing matrix (CEO synthesis emphasis only) -These routes describe WHICH specialist's findings the CEO weights more -heavily for a given PR type. They do NOT change which personas run -- -every mandatory persona always runs. Routing is a CEO synthesis hint. +These synthesis weights NEVER change which personas run. - **Architecture-heavy PR** -> CEO weights Python Architect on abstraction calls; CLI Logging on consistency. @@ -288,12 +218,9 @@ every mandatory persona always runs. Routing is a CEO synthesis hint. ## Execution checklist -Work through these steps in order. Do not skip ahead. Do not emit any -output to the PR before step 6. Every `task` spawn below is BLOCKING: -wait for the subagent to return before continuing, and never end your -turn while a panelist or the CEO synthesizer is still running. The turn -ends only after the comment (step 7) and label sweep (step 8) -- or, if -no comment can be rendered, an explicit `noop` (step 9) -- are emitted. +Follow in order; no PR output before step 6. Await every child. +Finish only after comment + label sweep, or step 9's explicit failure/ +no-action path. 1. **Read PR context** (the orchestrating workflow already fetched it via `gh pr view` / `gh pr diff`). Identify changed files for the @@ -349,12 +276,8 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. 5. **Spawn the CEO synthesizer task.** Pass the full set of validated panelist JSON returns to a `task` invocation that loads - `../../agents/apm-ceo.agent.md`. Run it as a BLOCKING task and WAIT - for its JSON return -- do NOT spawn it detached (background mode that - returns an `agent_id`) and do NOT end your turn while it runs. Its - return is required to render the comment; ending the turn here is the - exact cause of the "No Safe Outputs Generated" failure. The prompt - MUST: + `../../agents/apm-ceo.agent.md`. Run synchronously and WAIT for its + JSON before rendering; never detach or end the turn early. Its prompt MUST: - Provide all panelist returns as structured input. - Ask for: headline, arbitration prose, principle alignment (only applicable principles), curated recommended_followups (prioritized @@ -393,14 +316,28 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. handles over individual logins. Pass the resulting list to the template renderer as `notify_audience`. - This step replaces the maintainer-notification signal that the - pre-advisory verdict labels carried. It is the only mechanism by - which a fresh panel pass announces itself. + This is the fresh panel pass's only notification mechanism. 7. **Render the comment.** Load `assets/recommendation-template.md`, fill the placeholders from the panelist + CEO JSON, and emit it as exactly ONE comment. + **Transport boundary:** Discover the runtime's advertised tools and + schemas. With gh-aw safeoutputs, invoke structured `add_comment` + ONCE with the complete markdown directly in its `body` argument. + No shell staging, wrapper, or intermediate comment file. Configured + safeoutputs that are missing, failed, or uncertain NEVER authorize + direct GitHub writes or a second comment. Unknown is not absent. + + Only outside a safe-output workflow, when safeoutputs are absent + AND the caller authorizes interactive writes, create the body via + a native file-edit tool, then use + `gh pr comment --repo --body-file `. + Shell text contains only identifiers/path, never final, panelist, + or CEO prose: no heredocs, `echo`, `printf`, inline scripts, + substitutions, or encoding workarounds. Without the required tools + or authority, stop and report explicitly; never bypass the boundary. + Filling rules: - The per-persona summary table renders ONLY active panelists, one row per persona, with finding counts by severity and the persona's @@ -413,33 +350,29 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. - NEVER render the words "Verdict", "APPROVE", "REJECT", "blocked", "merge gate", or any equivalent. The panel is advisory. -8. **Sweep labels** via `safe-outputs.remove-labels`. The list MUST be - `[panel-review, panel-approved, panel-rejected]` -- always all three, - regardless of which are currently on the PR. `panel-review` is the - re-run idempotency reset; the other two are LEGACY VERDICT LABELS - from the pre-advisory regime that have no meaning under the advisory - contract and would mislead readers if left on a freshly-reviewed PR. - `safe-outputs.remove-labels` is idempotent on missing labels, so - sweeping all three on every run is safe and self-healing. NO - verdict labels are applied. - -9. **Guarantee a non-empty exit.** Your final action this turn MUST be a - safe output. In the normal path that is the single `add-comment` from - step 7 (the `remove-labels` sweep alone does NOT count -- it is not - the run's required output). Before ending the turn, confirm step 7 - actually issued the `add-comment` call and it did not error. If, after - every subagent has returned, you genuinely cannot render a comment - (e.g. a fatal upstream error), call `noop` so the run records an - intentional no-action rather than an empty `agent_output`. Ending the - turn with zero safe outputs is a FAILURE, not a success -- see the - "Non-empty turn exit" architecture invariant. +8. **Sweep labels** once: `[panel-review, panel-approved, panel-rejected]`, + always all three, even if absent. Use structured `remove_labels` in + gh-aw (idempotent on missing labels); on step 7's authorized CLI + path, perform the same cleanup via CLI. Reset the trigger and remove + obsolete verdict labels; NEVER apply verdict labels. + +9. **Verify exit.** In gh-aw, confirm step 7 issued one accepted + `add_comment` without error; label cleanup alone is not the required + output. Buffered acceptance is NOT publication: workflow + post-processing verifies delivery. If all children returned but no + comment can be produced, call advertised `noop` for intentional + no-action. If that tool is missing/fails, report failure explicitly, + never success or a CLI bypass. Zero safe outputs is a FAILURE. + On the authorized no-safeoutputs CLI path, verify the posted comment + and label state via CLI read-back; report failures, not fabricated + delivery or calls to nonexistent safe-output tools. ## Output contract (non-negotiable) - Exactly ONE comment per panel run, rendered from `assets/recommendation-template.md`. The `safe-outputs.add-comment.max: 2` is a fail-soft ceiling; the discipline lives here. -- Exactly ONE `remove-labels` call sweeping +- Exactly ONE label sweep using step 7's selected transport: `[panel-review, panel-approved, panel-rejected]`. - NO `add-labels` call. The advisory regime has no verdict to encode. - Subagents (panelists + CEO) NEVER write to PR state, NEVER call `gh @@ -453,28 +386,19 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. the conditional rules, the recommendation template, and the JSON schema MUST agree on the persona set. If you change one, change all in the same edit. -- **Calibrated severity discipline.** The advisory regime relies on - panelists honestly distinguishing `blocking` from `recommended`. If a - panelist marks everything `blocking`, the comment becomes noisy and - the maintainer learns to ignore the field. The panelist prompts state - the contract explicitly; the CEO arbitration prose is the safety - valve when a panelist over-flags. -- **Mermaid diagrams are template-required.** The python-architect - persona is asked to supply `extras.diagrams.class_diagram`, - `extras.diagrams.component`, and the OPTIONAL - `extras.diagrams.sequence`. The template renders nothing when they - are missing -- it does NOT invent diagrams. Real diagrams are - what makes the comment scannable for the human reviewer. +- **Calibrated severity.** Distinguish `blocking` from `recommended`; + CEO arbitration corrects over-flagging, not a merge gate. +- **Mermaid diagrams are template-required.** Request + `extras.diagrams.class_diagram`, `extras.diagrams.component`, and + OPTIONAL `extras.diagrams.sequence` from python-architect. Missing + diagrams remain absent; never invent them. - **Mermaid `classDiagram` `:::cssClass` shorthand gotcha.** GitHub's mermaid renderer rejects `:::cssClass` appended to relationship lines (e.g. `A *-- B:::touched`); use standalone `class Name:::cssClass` declarations instead. Authority: `python-architect.agent.md:146-154`. -- **Doc-writer detects DRIFT, not just edits.** When the PR changes - user-facing code that SHOULD have updated docs but did not, doc-writer - surfaces that as a finding. The conditional rule above is necessary - but not sufficient -- doc-writer reasons about doc consistency given - the diff, not just whether doc files were touched. +- **Doc-writer detects DRIFT, not just edits.** Review consistency + against the diff, including missing updates, not just touched docs. - **False-negative auth gotcha.** Auth regressions can be introduced from non-auth files that change the inputs to auth -- host classification, dependency parsing, clone URL construction, HTTP @@ -482,12 +406,8 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. a diff changes how a remote host, org, token source, or fallback path is selected and you are not certain it is auth-neutral, activate auth-expert as `active: true`. -- **Test-coverage probe is mandatory.** The test-coverage-expert MUST - verify "no test exists for X" via `view`/`grep` on the `tests/` tree - before emitting a finding. A false-positive coverage finding (test - exists but persona claimed it does not) destroys maintainer trust in - the field. The persona scope file enforces this; the orchestrator - passes the diff and trusts the persona to probe. +- **Test-coverage probe is mandatory.** The persona verifies missing + tests via `view`/`grep` on `tests/`; the orchestrator supplies the diff. - **Subagent write enforcement is contract-based, not sandbox-based.** Tool permissions are workflow-scoped, not subagent-scoped, so every spawned task technically inherits the same `gh` toolset. The @@ -495,17 +415,13 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. each `.agent.md` plus the `safe-outputs.add-comment.max: 2` fail-soft. If a subagent ever tries to post a comment, the cap catches it. -- **Empty-safe-output failure (background spawn-and-forget).** The single - most common way this panel "succeeds" yet posts nothing is spawning the - CEO synthesizer (or a panelist) as a background/detached task and then - ending the turn while it is still running. The harness exits with - `agent_output = {"items":[]}`, gh-aw skips safe-output detection, the - `add-comment` job never runs, and the workflow opens a "No Safe Outputs - Generated" issue. Every `task` spawn MUST be awaited to completion, and - the turn MUST end with a safe output -- the comment, or an explicit - `noop`. See the "Synchronous fan-out" and "Non-empty turn exit" - architecture invariants and step 9. -- **No verdict-label reset workflow.** The previous regime had a - companion workflow `pr-panel-label-reset.yml` that stripped verdict - labels on every push. The advisory regime has no verdict labels to - strip; that workflow is removed. +- **Empty-safe-output failure.** Background spawn-and-forget can end + gh-aw with no comment. Await every child and follow step 9. +- **Prose is data, not shell source.** PR #1844 documents run + `27815857237`: the command-safety parser scanned a heredoc and + rejected a wrapped prose line beginning with `kill`. Quoting does + not remove this hazard. Preserve words like `kill`, `rm`, `sudo` + as data; follow step 7, never shell-stage the prose. +- **No verdict-label reset workflow.** The obsolete + `pr-panel-label-reset.yml` is removed; the advisory regime adds no + verdict labels. diff --git a/apm.lock.yaml b/apm.lock.yaml index c4404b22b4..f2ede829ec 100644 --- a/apm.lock.yaml +++ b/apm.lock.yaml @@ -254,7 +254,7 @@ dependencies: - .agents/skills/apm-review-panel/evals/render_eval.py - .agents/skills/apm-review-panel/evals/trigger-evals.json deployed_file_hashes: - .agents/skills/apm-review-panel/SKILL.md: sha256:c9fee6c311b23b6ea72b06a424c8ba21c5d88bcc833e65259b7e0212aa73abbb + .agents/skills/apm-review-panel/SKILL.md: sha256:495c7d5b703b93e3046f341324e90806fe96ee44a93f92a2dea53bc684ed6b83 .agents/skills/apm-review-panel/apm.yml: sha256:80c403c4d3c59a91bb27b85650e21a466e9cd0cbc28e92bf0f3e53c6ea71cb2c .agents/skills/apm-review-panel/assets/ceo-return-schema.json: sha256:d8707211968efb0471d083f880d5353d66a0eda84635e803a930f60c91837468 .agents/skills/apm-review-panel/assets/panelist-return-schema.json: sha256:e3cf2ae17e93dd934f2659ed77d2640ea9601fbe8698f63ba800edf046691f99 @@ -1945,7 +1945,7 @@ deployments: owners: - local:packages/apm-review-panel active_owner: local:packages/apm-review-panel - content_hash: sha256:c9fee6c311b23b6ea72b06a424c8ba21c5d88bcc833e65259b7e0212aa73abbb + content_hash: sha256:495c7d5b703b93e3046f341324e90806fe96ee44a93f92a2dea53bc684ed6b83 - kind: project-relative target: copilot value: .agents/skills/apm-review-panel/apm.yml diff --git a/docs/src/content/docs/reference/cli/pack.md b/docs/src/content/docs/reference/cli/pack.md index 3dc981ad27..0ce034fe8c 100644 --- a/docs/src/content/docs/reference/cli/pack.md +++ b/docs/src/content/docs/reference/cli/pack.md @@ -20,7 +20,7 @@ apm pack [OPTIONS] - `target:` (or `targets:`) field containing `claude` or `copilot` -> ecosystem-specific `plugin.json` files. - Both blocks present -> bundle plus selected marketplace artifacts in a single run. -The bundle is built from `apm.lock.yaml`. An enriched copy of the lockfile (per-file SHA-256 in `bundle_files`, plus `pack:` metadata) is embedded inside the bundle so `apm install ` can verify integrity at install time. +The bundle embeds an enriched `apm.lock.yaml` (per-file SHA-256 in `bundle_files`, plus `pack:` metadata) for integrity verification. Install Claude plugin bundles with `apm install `; restore legacy `--format apm` archives with `apm unpack `. Plugin bundles are target-agnostic. The consumer's project decides where files land at install time -- the bundle carries no harness binding. Legacy `--format apm` packaging filters paths by target; see the [gh-aw workaround](../../../integrations/gh-aw/) for omitted Copilot skills. Flags whose scope does not match the detected outputs are silent no-ops, not errors, so the same `apm pack` invocation works in CI across projects that produce only a bundle, only a marketplace, or both. @@ -281,7 +281,7 @@ Plugin manifest generation runs after BUNDLE and MARKETPLACE phases so the gener - **Lockfile-attested dependencies.** Dependency content is packed exclusively from lockfile `deployed_files` and verified against `deployed_file_hashes`; the `apm_modules` cache is never packed. If a dependency has cached primitives but no `deployed_files`, `apm pack` errors and tells you to run `apm install`. - **Hidden-character scan.** Source files are scanned before bundling. Findings are reported as warnings only -- packing is non-blocking. Consumers are protected at install time, where critical findings block. - **Empty bundle warning.** If no package files match after dependency resolution, `apm pack` emits a warning and exits `0` with an empty bundle. Missing dependency content is an error, not an empty bundle. -- **Share line.** On success, `apm pack` prints `Share with: apm install ` so the produced bundle is immediately copy-pasteable. +- **Share line.** Plugin bundles print `Share with: apm install `; legacy APM bundles print `Share with: apm unpack `. - **Marketplace fallback.** With no `marketplace:` block in `apm.yml`, a legacy `marketplace.yml` file is read with a deprecation warning. Both files present is a hard error. - **Marketplace outputs.** Configure via `marketplace.outputs` map (keyed by format). Claude is included by default. The legacy list form (`outputs: [claude]`) still parses with a deprecation warning. Use `--marketplace=` to filter which formats are built in a given invocation. - **JSON mode.** `--json` makes `apm pack` machine-friendly: stdout is a single JSON object, all human-readable logs move to stderr. Combine with `--marketplace=` for selective CI matrix builds. @@ -299,8 +299,8 @@ Plugin manifest generation runs after BUNDLE and MARKETPLACE phases so the gener ## Related -- [`apm unpack`](../unpack/) -- inverse, deprecated; prefer `apm install `. -- [`apm install`](../install/) -- consumer side; installs a packed bundle directory, `.zip`, or `.tar.gz`. +- [`apm unpack`](../unpack/) -- deprecated; restores legacy APM bundles. +- [`apm install`](../install/) -- installs a Claude plugin bundle directory, `.zip`, or `.tar.gz`. - [Pack a bundle (producer guide)](../../../producer/pack-a-bundle/) -- task-oriented walkthrough. - [Publish to a marketplace](../../../producer/publish-to-a-marketplace/) -- end-to-end marketplace flow. - [Lockfile spec](../../lockfile-spec/) -- `pack:` metadata and `bundle_files` schema. diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index 85595f42c1..aa6bf6ee10 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -267,7 +267,7 @@ Lifecycle scripts fire on six events: `pre-install`, `post-install`, `pre-update | Command | Purpose | Key flags | |---------|---------|-----------| | `apm pack` | Build distributable artifacts (bundle and/or marketplace.json -- driven by `apm.yml`). A `dependencies:` mapping, including `dependencies: {}`, produces a bundle of local package content; omitted or null `dependencies:` does not. Default output (no format flag) is a Claude Code plugin directory. Pass `--format agent-plugin` to opt into a portable Agent Plugins v1 bundle instead -- strict portable core only (root `plugin.json`, `skills/`, root `mcp.json` written even when empty; no `agents/`, `commands/`, `instructions/`, `extensions/`, `hooks/`, or LSP payload). That bundle build fails before any output is written if the source project has non-portable agents/commands/instructions/extensions/hooks/LSP, naming the surfaces and pointing to `--format claude-plugin` (and to configuring LSP in the target directly, since neither pack format carries it). A packed Agent Plugin installs through the declarative route: declare it as a dependency in `apm.yml` and run `apm install --target copilot`, which keeps the unit whole under `apm_modules/` and registers it without locating or executing Copilot; stable Copilot CLI 1.0.81 or newer is required when loading the projection. The imperative local-bundle route still fails closed for Agent Plugin bundles. Plugin bundles are **target-agnostic**; legacy `--format apm` packaging filters paths by target (see `reference/cli/pack/`). `pack.target` is diagnostic metadata, not authoritative at install time; `pack.bundle_files` (path -> sha256) drives integrity verification. The consumer decides where files land. Dependency content is packed **exclusively** from lockfile-attested `deployed_files` (in every bundle format); the `apm_modules` cache is never packed. Each file is verified against its `deployed_file_hashes` SHA-256 before inclusion, so a file tampered after `apm install` (hash mismatch) or deleted (missing on disk) fails the pack with a message pointing at `apm install`; files with no recorded hash (older lockfiles) pack unverified. Dependency hooks-config / MCP-config is not attested, so it is not packed -- `apm pack` warns (`[!]`) and names the dependency (first-party root hooks/MCP are still packed). Marketplace-publishing projects (`marketplace:` block, no `dependencies:`) no longer emit the misleading "No plugin.json found" warning; after a successful build, a vendor-neutral catalog of artifact paths is appended together with a single docs pointer (`producer/publish-to-a-marketplace/#consume-from-any-assistant`) listing per-assistant install paths. Release-time gates `--check-versions` and `--check-clean` are opt-in and exit non-zero on misalignment / drift (codes 3 and 4 respectively) so release pipelines can fail fast; `--check-clean` is always read-only and never writes pack outputs. The version gate reads a local package's `apm.yml` first; Plugin collections without `apm.yml` use `plugin.json`'s `version`. Invalid or versionless `apm.yml` fails closed, and the fallback likewise rejects malformed or non-object JSON and a missing or blank version. When `apm.yml` declares `target: claude` or `target: copilot` (or the plural `targets:` equivalent), `apm pack` also generates an ecosystem-specific `plugin.json`: `.claude-plugin/plugin.json` for Claude (includes `mcpServers` from `.mcp.json` if present) and `.github/plugin/plugin.json` for Copilot (omits `mcpServers`). An existing file at the target path is preserved (a warning is emitted and the write is skipped) unless `--force` is passed; `--dry-run` prevents writes. Credential-bearing keys and secret-shaped values in `.mcp.json` are stripped recursively at any depth from the Claude manifest before writing, so a committed manifest never leaks secrets (see the apm pack reference, `reference/cli/pack/#credential-stripping-claude-mcpservers`). | `-o PATH`, `--archive` (produce a `.zip` archive instead of a directory; changed from `.tar.gz`), `--archive-format [zip\|tar.gz]` (default `zip`; use `tar.gz` for smaller legacy CI artifacts; only active with `--archive`), `--dry-run`, `--format [plugin\|agent-plugin\|claude\|claude-plugin\|apm]` (`agent-plugin` is the sole selector for the Agent Plugin bundle; `plugin` is a compatibility alias for the Claude plugin bundle, not for `agent-plugin`; `claude`/`claude-plugin` also select the Claude plugin bundle; `apm` selects the legacy APM layout; default `claude-plugin`), `--claude-plugin` (shortcut for `--format claude-plugin`; passing more than one of `--claude-plugin`/`--format` is a usage error), `--force`, `--offline`, `--include-prerelease`, `--marketplace=FORMATS`, `--marketplace-path FORMAT=PATH`, `--json`, `--check-versions` (release gate: per-package versions match `marketplace.versioning.strategy`; exit 3 on failure), `--check-clean` (read-only release gate: regenerate-and-diff against the effective marketplace path, including `--marketplace-path` overrides; never writes pack outputs; exit 4 on drift). `-t/--target` is **deprecated** but still filters legacy APM paths. Exit codes: `0` success, `1` build/runtime error, `2` schema validation error, `3` `--check-versions` misalignment, `4` `--check-clean` drift. | -| `apm unpack BUNDLE` | **[Deprecated]** Extract a bundle. Use `apm install ` instead -- it deploys directly with integrity verification and target resolution. | `-o PATH`, `--skip-verify`, `--force`, `--dry-run` | +| `apm unpack BUNDLE` | **[Deprecated]** Restore legacy `--format apm` bundles. Use `apm install ` for Claude plugin bundles with integrity verification and target resolution. | `-o PATH`, `--skip-verify`, `--force`, `--dry-run` | For marketplace publishing, `--strict-metadata` preflights every selected marketplace output before any artifact write and exits `5` if remote metadata diff --git a/packages/apm-review-panel/SKILL.md b/packages/apm-review-panel/SKILL.md index 403845415f..70e7aff024 100644 --- a/packages/apm-review-panel/SKILL.md +++ b/packages/apm-review-panel/SKILL.md @@ -18,64 +18,39 @@ description: >- # APM Review Panel - Fan-Out Advisory Review -The panel is FAN-OUT + SYNTHESIZER. Each persona runs in its own agent -thread (via the `task` tool) and returns JSON matching -`assets/panelist-return-schema.json`. The orchestrator schema-validates -each return, hands all returns to the apm-ceo synthesizer (also a task -thread, returns JSON matching `assets/ceo-return-schema.json`), then -renders ONE recommendation comment from `assets/recommendation-template.md`. - -This skill is ADVISORY by design. It does not compute a binary verdict, it -does not apply verdict labels, and it does not gate merge. The panel -surfaces findings; the maintainer and the PR author decide ship. +FAN-OUT + SYNTHESIZER: each persona returns schema-valid JSON in its +own `task` thread; apm-ceo synthesizes in another. The orchestrator +renders ONE advisory comment. The maintainer and PR author decide ship. ## Architecture invariants -- **Advisory regime, not gate regime.** There is no `APPROVE` / `REJECT`, - no `panel-approved` / `panel-rejected` label, no deterministic verdict - computation. The CEO returns a `ship_recommendation.stance` (`ship_now` +- **Advisory, never a gate.** No binary verdict computation, `APPROVE` / + `REJECT`, or verdict labels. The CEO's `ship_recommendation.stance` (`ship_now` / `ship_with_followups` / `needs_discussion` / `needs_rework`); this is - prose for the human reviewer, never auto-applied as a label or status - check. This is the architectural fix for the previous regime's - over-strictness: removing the binary gate removes the incentive for - panelists to inflate `required[]` defensively. + prose for humans, never a label or status check. - **Three severity buckets, none of them gate.** Findings carry `severity: blocking | recommended | nit`. `blocking` is the highest signal a panelist can send and renders prominently in the comment; it still does not block merge. `recommended` is the default for substantive feedback. `nit` is one-line polish. The orchestrator never reads severity to gate anything. -- **Single-writer interlock.** Only the orchestrator writes to the PR: - exactly one `add-comment` and one `remove-labels` call. The - `remove-labels` call always sweeps `panel-review` (trigger - idempotency) AND defensively removes `panel-approved` / - `panel-rejected` if present (legacy verdict labels from the - pre-advisory regime; they have no meaning here and would mislead - readers if left on a PR after a fresh advisory pass). NO `add-labels` - call -- there are no verdict labels to apply. Panelist subagents and - the CEO subagent return JSON only and MUST NOT call any `gh` write - command, post comments, apply labels, or touch the PR state. +- **Single-writer interlock.** Only the orchestrator emits one comment + and one label sweep using step 7's transport. Sweep `panel-review` + (trigger reset), `panel-approved`, and `panel-rejected` (obsolete, + misleading verdict labels). NO `add-labels`. Panelists and CEO return + JSON only: no `gh` writes, comments, labels, or PR-state mutations. - **Single-emission discipline.** Exactly one comment per panel run, rendered from `assets/recommendation-template.md` after all subagents return. -- **Non-empty turn exit (the run's hard contract).** gh-aw decides - success by inspecting `agent_output` AFTER your turn ends: a turn that - ends with zero safe outputs (`agent_output = {"items":[]}`) is detected - as a failure, the safe-output detection job is skipped, the - `add-comment` job never runs, and the workflow opens a "No Safe Outputs - Generated" issue. Therefore your turn MUST end with at least one safe - output -- the rendered comment on success (step 7), or an explicit - `noop` if the run genuinely cannot produce one. NEVER end the turn - empty. -- **Synchronous fan-out -- never spawn-and-forget.** Every `task` spawn - (each panelist AND the CEO synthesizer) is BLOCKING: spawn it, WAIT for - its JSON return, then continue. Use the `task` tool's synchronous mode; - do NOT use its background/detached mode -- the variant that returns an - `agent_id` immediately and runs the subagent in the background -- for - any panelist or the CEO. Their returns are LOAD-BEARING: the comment - cannot be rendered without them. Spawning the CEO (or a panelist) - detached and then ending the turn while it is still running is the - documented cause of the empty-output failure above. +- **Non-empty turn exit.** In gh-aw, zero safe outputs + (`agent_output = {"items":[]}`) cause "No Safe Outputs Generated": + detection is skipped and `add-comment` never runs. Emit the comment + or, if genuinely impossible, explicit `noop`; never end empty. + Step 9 scopes verification to the selected transport. +- **Synchronous fan-out -- never spawn-and-forget.** Every panelist and + CEO `task` MUST use synchronous mode and be awaited for its JSON. + Never use background/detached mode returning an `agent_id`, or end + the turn while a child runs: their returns are required for rendering. ## Agent roster @@ -95,39 +70,9 @@ surfaces findings; the maintainer and the PR author decide ship. ## Topology ``` - apm-review-panel SKILL (orchestrator thread) - | - FAN-OUT via task tool (panelists in parallel) - | - +-----+-------+-------+-----+-----+------+-----------+----------+ - v v v v v v v v v (cond.) - py cli dx-ux sec grw auth doc-writer test-cov - | | | | | | | | - | each returns JSON per panelist-return-schema.json - +-----+-------+-------+-----+-----+------+-----------+----------+ - | - v <-- S4 schema-validate - v <-- on malformed: re-spawn that persona - v - task: apm-ceo synthesizer - - aggregates findings across panelists - - resolves dissent - - emits headline + arbitration prose + principle alignment - - emits curated recommended_followups (prioritized) - - emits ship_recommendation (stance + prose) - - returns ceo-return-schema.json - | - v <-- S4 schema-validate - v - orchestrator (sole writer) - | | - v v - add-comment remove-labels - (max:2) [panel-review, - panel-approved, - panel-rejected] - (trigger reset + - legacy verdict sweep) +orchestrator -> nine parallel panelist tasks -> S4 panelist schema gate + -> apm-ceo task (aggregate, arbitrate dissent, curate follow-ups) + -> S4 CEO schema gate -> orchestrator: one comment + one label sweep ``` ## Conditional panelists @@ -178,14 +123,10 @@ change user-facing documentation, agent or skill prose, instruction files, CHANGELOG entries, README claims, or any natural-language artifact a reader will rely on? If unsure, answer YES." -When the doc-writer is active and the PR includes documentation changes, -the persona reviews them for: (a) consistency with the existing voice -and structure, (b) accuracy against the code being changed, (c) -completeness for the typical reader (no orphan claims, no missing -prerequisites), (d) discoverability (cross-links, sidebar order if -Starlight content). When the doc-writer is active because of code -changes that SHOULD have updated docs but did not, the persona surfaces -that gap as a finding. +When active, doc-writer checks changed docs for voice/structure +consistency, code accuracy, completeness (no orphan claims or missing +prerequisites), and discoverability (cross-links, Starlight sidebar +order). Surface missing doc updates required by code changes as findings. ### Performance Expert @@ -241,32 +182,21 @@ documentation-only PR -- the diff contains zero `src/**/*.py` files. In that case set `inactive_reason: "documentation-only PR -- no runtime code paths to defend"`. -The activation rule is intentionally narrow: under the advisory regime, -test outcomes are LOAD-BEARING for CEO arbitration (passed / failed / -missing test evidence outranks opinion-only findings -- see -`apm-ceo.agent.md` and `panelist-return-schema.json` evidence block). -A persona whose findings carry that weight cannot be silently skipped -on a heuristic. Better to spawn it on a pure refactor and have it -return a single `nit`-severity "no behavior surface touched -- no -coverage finding" line than to skip it and leave the CEO without -evidence to weigh. (Earlier revisions of this skill paired test-coverage -with auth and doc-writer as conditional for symmetry; that symmetry -broke when test evidence became load-bearing.) - -The test-coverage-expert is paired with the devx-ux-expert lens and -defends the user-promise contracts the DevX persona enumerates (CLI -surface, error wording, install idempotency, lockfile determinism, auth -resolution). It MUST verify "no test exists" claims with `view`/`grep` -on the test tree before emitting a finding -- false-positive coverage -findings destroy trust in the field. It does NOT compute coverage -percentages, does NOT flag tests for pure refactors, and does NOT -duplicate python-architect on test-code design. +Test evidence (passed / failed / missing) outranks opinion in CEO +arbitration; see `apm-ceo.agent.md` and the panelist schema evidence +block. Never skip this persona heuristically. On a pure refactor, +return a `nit` "no behavior surface touched -- no coverage finding" +rather than leaving the CEO without evidence. + +Paired with devx-ux-expert, it defends CLI surface, error wording, +install idempotency, lockfile determinism, and auth resolution. +Verify "no test exists" with `view`/`grep` on the test tree before +reporting. No coverage percentages, pure-refactor test findings, or +duplication of python-architect's test-code design review. ## Routing matrix (CEO synthesis emphasis only) -These routes describe WHICH specialist's findings the CEO weights more -heavily for a given PR type. They do NOT change which personas run -- -every mandatory persona always runs. Routing is a CEO synthesis hint. +These synthesis weights NEVER change which personas run. - **Architecture-heavy PR** -> CEO weights Python Architect on abstraction calls; CLI Logging on consistency. @@ -288,12 +218,9 @@ every mandatory persona always runs. Routing is a CEO synthesis hint. ## Execution checklist -Work through these steps in order. Do not skip ahead. Do not emit any -output to the PR before step 6. Every `task` spawn below is BLOCKING: -wait for the subagent to return before continuing, and never end your -turn while a panelist or the CEO synthesizer is still running. The turn -ends only after the comment (step 7) and label sweep (step 8) -- or, if -no comment can be rendered, an explicit `noop` (step 9) -- are emitted. +Follow in order; no PR output before step 6. Await every child. +Finish only after comment + label sweep, or step 9's explicit failure/ +no-action path. 1. **Read PR context** (the orchestrating workflow already fetched it via `gh pr view` / `gh pr diff`). Identify changed files for the @@ -349,12 +276,8 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. 5. **Spawn the CEO synthesizer task.** Pass the full set of validated panelist JSON returns to a `task` invocation that loads - `../../agents/apm-ceo.agent.md`. Run it as a BLOCKING task and WAIT - for its JSON return -- do NOT spawn it detached (background mode that - returns an `agent_id`) and do NOT end your turn while it runs. Its - return is required to render the comment; ending the turn here is the - exact cause of the "No Safe Outputs Generated" failure. The prompt - MUST: + `../../agents/apm-ceo.agent.md`. Run synchronously and WAIT for its + JSON before rendering; never detach or end the turn early. Its prompt MUST: - Provide all panelist returns as structured input. - Ask for: headline, arbitration prose, principle alignment (only applicable principles), curated recommended_followups (prioritized @@ -393,14 +316,28 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. handles over individual logins. Pass the resulting list to the template renderer as `notify_audience`. - This step replaces the maintainer-notification signal that the - pre-advisory verdict labels carried. It is the only mechanism by - which a fresh panel pass announces itself. + This is the fresh panel pass's only notification mechanism. 7. **Render the comment.** Load `assets/recommendation-template.md`, fill the placeholders from the panelist + CEO JSON, and emit it as exactly ONE comment. + **Transport boundary:** Discover the runtime's advertised tools and + schemas. With gh-aw safeoutputs, invoke structured `add_comment` + ONCE with the complete markdown directly in its `body` argument. + No shell staging, wrapper, or intermediate comment file. Configured + safeoutputs that are missing, failed, or uncertain NEVER authorize + direct GitHub writes or a second comment. Unknown is not absent. + + Only outside a safe-output workflow, when safeoutputs are absent + AND the caller authorizes interactive writes, create the body via + a native file-edit tool, then use + `gh pr comment --repo --body-file `. + Shell text contains only identifiers/path, never final, panelist, + or CEO prose: no heredocs, `echo`, `printf`, inline scripts, + substitutions, or encoding workarounds. Without the required tools + or authority, stop and report explicitly; never bypass the boundary. + Filling rules: - The per-persona summary table renders ONLY active panelists, one row per persona, with finding counts by severity and the persona's @@ -413,33 +350,29 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. - NEVER render the words "Verdict", "APPROVE", "REJECT", "blocked", "merge gate", or any equivalent. The panel is advisory. -8. **Sweep labels** via `safe-outputs.remove-labels`. The list MUST be - `[panel-review, panel-approved, panel-rejected]` -- always all three, - regardless of which are currently on the PR. `panel-review` is the - re-run idempotency reset; the other two are LEGACY VERDICT LABELS - from the pre-advisory regime that have no meaning under the advisory - contract and would mislead readers if left on a freshly-reviewed PR. - `safe-outputs.remove-labels` is idempotent on missing labels, so - sweeping all three on every run is safe and self-healing. NO - verdict labels are applied. - -9. **Guarantee a non-empty exit.** Your final action this turn MUST be a - safe output. In the normal path that is the single `add-comment` from - step 7 (the `remove-labels` sweep alone does NOT count -- it is not - the run's required output). Before ending the turn, confirm step 7 - actually issued the `add-comment` call and it did not error. If, after - every subagent has returned, you genuinely cannot render a comment - (e.g. a fatal upstream error), call `noop` so the run records an - intentional no-action rather than an empty `agent_output`. Ending the - turn with zero safe outputs is a FAILURE, not a success -- see the - "Non-empty turn exit" architecture invariant. +8. **Sweep labels** once: `[panel-review, panel-approved, panel-rejected]`, + always all three, even if absent. Use structured `remove_labels` in + gh-aw (idempotent on missing labels); on step 7's authorized CLI + path, perform the same cleanup via CLI. Reset the trigger and remove + obsolete verdict labels; NEVER apply verdict labels. + +9. **Verify exit.** In gh-aw, confirm step 7 issued one accepted + `add_comment` without error; label cleanup alone is not the required + output. Buffered acceptance is NOT publication: workflow + post-processing verifies delivery. If all children returned but no + comment can be produced, call advertised `noop` for intentional + no-action. If that tool is missing/fails, report failure explicitly, + never success or a CLI bypass. Zero safe outputs is a FAILURE. + On the authorized no-safeoutputs CLI path, verify the posted comment + and label state via CLI read-back; report failures, not fabricated + delivery or calls to nonexistent safe-output tools. ## Output contract (non-negotiable) - Exactly ONE comment per panel run, rendered from `assets/recommendation-template.md`. The `safe-outputs.add-comment.max: 2` is a fail-soft ceiling; the discipline lives here. -- Exactly ONE `remove-labels` call sweeping +- Exactly ONE label sweep using step 7's selected transport: `[panel-review, panel-approved, panel-rejected]`. - NO `add-labels` call. The advisory regime has no verdict to encode. - Subagents (panelists + CEO) NEVER write to PR state, NEVER call `gh @@ -453,28 +386,19 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. the conditional rules, the recommendation template, and the JSON schema MUST agree on the persona set. If you change one, change all in the same edit. -- **Calibrated severity discipline.** The advisory regime relies on - panelists honestly distinguishing `blocking` from `recommended`. If a - panelist marks everything `blocking`, the comment becomes noisy and - the maintainer learns to ignore the field. The panelist prompts state - the contract explicitly; the CEO arbitration prose is the safety - valve when a panelist over-flags. -- **Mermaid diagrams are template-required.** The python-architect - persona is asked to supply `extras.diagrams.class_diagram`, - `extras.diagrams.component`, and the OPTIONAL - `extras.diagrams.sequence`. The template renders nothing when they - are missing -- it does NOT invent diagrams. Real diagrams are - what makes the comment scannable for the human reviewer. +- **Calibrated severity.** Distinguish `blocking` from `recommended`; + CEO arbitration corrects over-flagging, not a merge gate. +- **Mermaid diagrams are template-required.** Request + `extras.diagrams.class_diagram`, `extras.diagrams.component`, and + OPTIONAL `extras.diagrams.sequence` from python-architect. Missing + diagrams remain absent; never invent them. - **Mermaid `classDiagram` `:::cssClass` shorthand gotcha.** GitHub's mermaid renderer rejects `:::cssClass` appended to relationship lines (e.g. `A *-- B:::touched`); use standalone `class Name:::cssClass` declarations instead. Authority: `python-architect.agent.md:146-154`. -- **Doc-writer detects DRIFT, not just edits.** When the PR changes - user-facing code that SHOULD have updated docs but did not, doc-writer - surfaces that as a finding. The conditional rule above is necessary - but not sufficient -- doc-writer reasons about doc consistency given - the diff, not just whether doc files were touched. +- **Doc-writer detects DRIFT, not just edits.** Review consistency + against the diff, including missing updates, not just touched docs. - **False-negative auth gotcha.** Auth regressions can be introduced from non-auth files that change the inputs to auth -- host classification, dependency parsing, clone URL construction, HTTP @@ -482,12 +406,8 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. a diff changes how a remote host, org, token source, or fallback path is selected and you are not certain it is auth-neutral, activate auth-expert as `active: true`. -- **Test-coverage probe is mandatory.** The test-coverage-expert MUST - verify "no test exists for X" via `view`/`grep` on the `tests/` tree - before emitting a finding. A false-positive coverage finding (test - exists but persona claimed it does not) destroys maintainer trust in - the field. The persona scope file enforces this; the orchestrator - passes the diff and trusts the persona to probe. +- **Test-coverage probe is mandatory.** The persona verifies missing + tests via `view`/`grep` on `tests/`; the orchestrator supplies the diff. - **Subagent write enforcement is contract-based, not sandbox-based.** Tool permissions are workflow-scoped, not subagent-scoped, so every spawned task technically inherits the same `gh` toolset. The @@ -495,17 +415,13 @@ no comment can be rendered, an explicit `noop` (step 9) -- are emitted. each `.agent.md` plus the `safe-outputs.add-comment.max: 2` fail-soft. If a subagent ever tries to post a comment, the cap catches it. -- **Empty-safe-output failure (background spawn-and-forget).** The single - most common way this panel "succeeds" yet posts nothing is spawning the - CEO synthesizer (or a panelist) as a background/detached task and then - ending the turn while it is still running. The harness exits with - `agent_output = {"items":[]}`, gh-aw skips safe-output detection, the - `add-comment` job never runs, and the workflow opens a "No Safe Outputs - Generated" issue. Every `task` spawn MUST be awaited to completion, and - the turn MUST end with a safe output -- the comment, or an explicit - `noop`. See the "Synchronous fan-out" and "Non-empty turn exit" - architecture invariants and step 9. -- **No verdict-label reset workflow.** The previous regime had a - companion workflow `pr-panel-label-reset.yml` that stripped verdict - labels on every push. The advisory regime has no verdict labels to - strip; that workflow is removed. +- **Empty-safe-output failure.** Background spawn-and-forget can end + gh-aw with no comment. Await every child and follow step 9. +- **Prose is data, not shell source.** PR #1844 documents run + `27815857237`: the command-safety parser scanned a heredoc and + rejected a wrapped prose line beginning with `kill`. Quoting does + not remove this hazard. Preserve words like `kill`, `rm`, `sudo` + as data; follow step 7, never shell-stage the prose. +- **No verdict-label reset workflow.** The obsolete + `pr-panel-label-reset.yml` is removed; the advisory regime adds no + verdict labels. diff --git a/src/apm_cli/commands/pack.py b/src/apm_cli/commands/pack.py index 8ebf782910..7194714d3c 100644 --- a/src/apm_cli/commands/pack.py +++ b/src/apm_cli/commands/pack.py @@ -181,7 +181,7 @@ def _parse_marketplace_filter( "-t", type=TargetParamType(), default=None, - help="[Deprecated] Target platform filter. Bundles are now target-agnostic; the consumer's project decides where files land at install time. Value is recorded in pack.target as informational metadata only and is ignored by 'apm install'. The flag will be removed in a future release.", + help="[Deprecated] Filter paths in legacy --format apm bundles. Plugin formats are target-agnostic. Recorded in pack.target as informational metadata and ignored by 'apm install'. The flag will be removed in a future release.", ) @click.option( "--archive", @@ -384,8 +384,9 @@ def pack_cmd( # noqa: C901, PLR0912, PLR0913 -- Click handler, one param per CL else: logger.warning( "--target is deprecated and will be removed in a future release. " - "Bundles are target-agnostic; the value is recorded as informational " - "pack.target metadata only and is ignored by 'apm install'." + "Legacy APM bundles still filter paths by target; plugin formats " + "are target-agnostic. The value is recorded as informational " + "pack.target metadata and is ignored by 'apm install'." ) effective_target = target options = BuildOptions( @@ -794,11 +795,9 @@ def _render_bundle_result( "Claude plugin bundle ready -- contains plugin.json plus " "plugin-native directories and an embedded apm.lock.yaml." ) - # Issue #1207: target-agnostic bundles install into any consumer - # project. Print a copy-pasteable share line so packing creates - # the social hand-off naturally. if pack_result.bundle_path: - logger.info(f"Share with: apm install {pack_result.bundle_path}") + verb = "unpack" if fmt == BundleFormat.APM else "install" + logger.info(f"Share with: apm {verb} {pack_result.bundle_path}") def _render_marketplace_result(logger, report, dry_run, extra_warnings=None, outputs=None): diff --git a/tests/integration/test_pack_shared_skill_roundtrip.py b/tests/integration/test_pack_shared_skill_roundtrip.py index 76424e119f..5096366cdb 100644 --- a/tests/integration/test_pack_shared_skill_roundtrip.py +++ b/tests/integration/test_pack_shared_skill_roundtrip.py @@ -81,6 +81,9 @@ def test_install_pack_zip_restore_preserves_shared_skill(tmp_path: Path) -> None packed = runner.invoke(cli, ["pack", "--format", "apm", "--archive", "--target", "copilot"]) assert packed.exit_code == 0, packed.output + output = " ".join(packed.output.split()) + assert "Legacy APM bundles still filter paths by target" in output + assert "Share with: apm unpack" in output archives = list((producer.root / "build").glob("*.zip")) assert len(archives) == 1 with zipfile.ZipFile(archives[0]) as archive: diff --git a/tests/unit/commands/test_pack_cli_surface.py b/tests/unit/commands/test_pack_cli_surface.py index 755e169aec..1ce197a2f0 100644 --- a/tests/unit/commands/test_pack_cli_surface.py +++ b/tests/unit/commands/test_pack_cli_surface.py @@ -221,12 +221,16 @@ def test_live_apm_format_no_plugin_message(self) -> None: _render_bundle_result(logger, result, "apm", None, False) assert not any("Plugin bundle ready" in p for p in logger.progresses) - def test_live_share_line_emitted(self) -> None: - """The 'Share with: apm install ...' info line is always emitted.""" + @pytest.mark.parametrize( + ("fmt", "verb"), + [(BundleFormat.APM, "unpack"), (BundleFormat.CLAUDE_PLUGIN, "install")], + ) + def test_live_share_line_emitted(self, fmt: BundleFormat, verb: str) -> None: + """The handoff command must accept the format that was just packed.""" logger = _RecordingLogger() result = _pack_result(files=["file.md"], bundle_path="build/my-bundle") - _render_bundle_result(logger, result, "apm", None, False) - assert any("apm install" in i for i in logger.infos) + _render_bundle_result(logger, result, fmt, None, False) + assert f"Share with: apm {verb} build/my-bundle" in logger.infos def test_live_zip_archive_emits_migration_tip_when_requested(self) -> None: logger = _RecordingLogger() diff --git a/tests/unit/commands/test_pack_phase3.py b/tests/unit/commands/test_pack_phase3.py index cdf677dddb..ebec35072d 100644 --- a/tests/unit/commands/test_pack_phase3.py +++ b/tests/unit/commands/test_pack_phase3.py @@ -222,10 +222,10 @@ def test_live_apm_format_no_plugin_message(self) -> None: assert not any("Plugin bundle ready" in p for p in logger.progresses) def test_live_share_line_emitted(self) -> None: - """The 'Share with: apm install ...' info line is always emitted.""" + """Plugin bundles retain their install handoff.""" logger = _RecordingLogger() result = _pack_result(files=["file.md"], bundle_path="build/my-bundle") - _render_bundle_result(logger, result, "apm", None, False) + _render_bundle_result(logger, result, BundleFormat.CLAUDE_PLUGIN, None, False) assert any("apm install" in i for i in logger.infos) def test_live_no_bundle_path_skips_share_line(self) -> None: diff --git a/tests/unit/test_review_panel_transport_contract.py b/tests/unit/test_review_panel_transport_contract.py new file mode 100644 index 0000000000..3721e13a85 --- /dev/null +++ b/tests/unit/test_review_panel_transport_contract.py @@ -0,0 +1,79 @@ +"""Static instruction/provenance guards, NOT model or hosted-delivery tests.""" + +from pathlib import Path + +import pytest + +from apm_cli.core.deployment_ledger import DeploymentLedgerCodec +from apm_cli.deps.lockfile import LockFile +from apm_cli.utils.content_hash import compute_file_hash + +pytestmark = pytest.mark.component + +ROOT = Path(__file__).resolve().parents[2] +SOURCE = ROOT / "packages/apm-review-panel/SKILL.md" +DEPLOYED = ".agents/skills/apm-review-panel/SKILL.md" + + +def _section(text: str, start: str, end: str) -> str: + return " ".join(text.split(start, 1)[1].split(end, 1)[0].split()) + + +@pytest.mark.parametrize( + "clause", + [ + "invoke structured `add_comment` ONCE with the complete markdown " + "directly in its `body` argument.", + "No shell staging, wrapper, or intermediate comment file.", + "Configured safeoutputs that are missing, failed, or uncertain " + "NEVER authorize direct GitHub writes or a second comment.", + "Unknown is not absent.", + "Only outside a safe-output workflow, when safeoutputs are absent " + "AND the caller authorizes interactive writes", + "create the body via a native file-edit tool", + "`gh pr comment --repo --body-file `", + "never final, panelist, or CEO prose: no heredocs, `echo`, `printf`, " + "inline scripts, substitutions, or encoding workarounds.", + ], +) +def test_emission_boundary_retains_required_clauses(clause: str) -> None: + """Deleting a required transport clause must trip a bounded text guard.""" + emission = _section(SOURCE.read_text(), "7. **Render", "8. **Sweep") + assert clause in emission + + +def test_exit_distinguishes_acceptance_from_delivery() -> None: + text = SOURCE.read_text() + exit_rule = _section(text, "9. **Verify exit.", "## Output contract") + for clause in ( + "label cleanup alone is not the required output", + "Buffered acceptance is NOT publication", + "call advertised `noop`", + "If that tool is missing/fails, report failure explicitly", + "Zero safe outputs is a FAILURE", + "verify the posted comment and label state via CLI read-back", + ): + assert clause in exit_rule + cleanup = _section(text, "8. **Sweep", "9. **Verify") + assert "[panel-review, panel-approved, panel-rejected]" in cleanup + assert "NEVER apply verdict labels" in cleanup + assert "`27815857237`" in text + + +def test_deployment_and_both_hash_views_match_source() -> None: + assert SOURCE.read_bytes() == (ROOT / DEPLOYED).read_bytes() + digest = compute_file_hash(ROOT / DEPLOYED) + lock = LockFile.read(ROOT / "apm.lock.yaml") + assert lock is not None + rows = [ + row + for row in DeploymentLedgerCodec.from_lockfile(lock).records.values() + if row.locator.value == DEPLOYED + ] + assert len(rows) == 1 + row = rows[0] + assert row.locator.target == "copilot" + assert row.locator.scope == "project" + assert row.active_owner == "local:packages/apm-review-panel" + assert row.content_hash == digest + assert lock.dependencies[row.active_owner].deployed_file_hashes[DEPLOYED] == digest From 6524ed3c2580f31acd7808959b29f9be8b22b742 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Wed, 9 Sep 2026 11:42:23 +0200 Subject: [PATCH 3/5] Cover Copilot legacy archives in the native lifecycle contract Reuse the existing local-Git scenario and installed CLI runner to compare a skill and nested resource across installation, Copilot-only legacy ZIP packing, and clean unpacking. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../test_required_lifecycle_state_machine.py | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_required_lifecycle_state_machine.py b/tests/integration/test_required_lifecycle_state_machine.py index d4c35f4e0e..6b71bdef08 100644 --- a/tests/integration/test_required_lifecycle_state_machine.py +++ b/tests/integration/test_required_lifecycle_state_machine.py @@ -145,6 +145,7 @@ def _publish( name: str, *, skill: str | None = None, + skill_resource: str | None = None, instruction: str | None = None, agent: str | None = None, hook_command: str | None = None, @@ -166,7 +167,13 @@ def _publish( mcp_dependencies=mcp_dependencies, ) if skill is not None: - scenario.sources.add_skill(package, skill, _skill(skill)) + skill_path = scenario.sources.add_skill(package, skill, _skill(skill)) + if skill_resource is not None: + resource = skill_path.parent / "assets" / "reference.md" + resource.parent.mkdir() + resource.write_text(skill_resource, encoding="utf-8") + elif skill_resource is not None: + raise ValueError("A skill resource requires a skill") if instruction is not None: scenario.sources.add_instruction(package, instruction, _instruction(instruction)) if agent is not None: @@ -844,6 +851,7 @@ def test_required_pack_install_compile_audit_closes_regular_package_state( scenario, "regular-kit-source", skill="triage", + skill_resource="Nested resource retained by legacy Copilot packing.\n", instruction="guard", agent="reviewer", ) @@ -861,6 +869,35 @@ def test_required_pack_install_compile_audit_closes_regular_package_state( environment=source.environment, scenario_id="pack-closure-install-producer", ) + expected_skill_files = { + f".agents/skills/triage/{name}": (producer.root / ".agents/skills/triage" / name).read_bytes() + for name in ("SKILL.md", "assets/reference.md") + } + _run_success( + scenario, + producer, + ("pack", "--format", "apm", "--archive", "--target", "copilot", "--output", "legacy-build"), + environment=source.environment, + scenario_id="pack-closure-legacy-pack", + ) + legacy_archive = producer.root / "legacy-build" / "regular-kit-0.1.0.zip" + with zipfile.ZipFile(legacy_archive) as bundle: + lock_paths = [name for name in bundle.namelist() if name.endswith("/apm.lock.yaml")] + assert len(lock_paths) == 1 + prefix = lock_paths[0].removesuffix("apm.lock.yaml") + for path, content in expected_skill_files.items(): + assert bundle.read(prefix + path) == content + legacy_consumer = scenario.consumers.create("legacy-consumer", targets=("copilot",)) + assert not (legacy_consumer.root / ".agents").exists() + _run_success( + scenario, + legacy_consumer, + ("unpack", str(legacy_archive)), + environment=scenario.environment, + scenario_id="pack-closure-legacy-unpack", + ) + for path, content in expected_skill_files.items(): + assert (legacy_consumer.root / path).read_bytes() == content _run_success( scenario, producer, From b27c2c09dba5def1114e86c5f1c94c1e98af292d Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Wed, 9 Sep 2026 12:30:25 +0200 Subject: [PATCH 4/5] Format the bounded native archive regression Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/integration/test_required_lifecycle_state_machine.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/test_required_lifecycle_state_machine.py b/tests/integration/test_required_lifecycle_state_machine.py index 6b71bdef08..ed740ef301 100644 --- a/tests/integration/test_required_lifecycle_state_machine.py +++ b/tests/integration/test_required_lifecycle_state_machine.py @@ -870,7 +870,9 @@ def test_required_pack_install_compile_audit_closes_regular_package_state( scenario_id="pack-closure-install-producer", ) expected_skill_files = { - f".agents/skills/triage/{name}": (producer.root / ".agents/skills/triage" / name).read_bytes() + f".agents/skills/triage/{name}": ( + producer.root / ".agents/skills/triage" / name + ).read_bytes() for name in ("SKILL.md", "assets/reference.md") } _run_success( From 4eaaf9dee7934a2733f6fa4ec1291b22ac0b8709 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Wed, 9 Sep 2026 13:51:57 +0200 Subject: [PATCH 5/5] fix: qualify legacy unpack deprecation guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- docs/src/content/docs/reference/cli/unpack.md | 2 +- .../apm-guide/.apm/skills/apm-usage/commands.md | 2 +- src/apm_cli/commands/pack.py | 9 ++++++--- tests/unit/commands/test_pack_cli_surface.py | 13 +++++++++---- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/docs/src/content/docs/reference/cli/unpack.md b/docs/src/content/docs/reference/cli/unpack.md index 33f5268533..3f9ae214c4 100644 --- a/docs/src/content/docs/reference/cli/unpack.md +++ b/docs/src/content/docs/reference/cli/unpack.md @@ -6,7 +6,7 @@ sidebar: --- :::caution[Deprecated] -`apm unpack` is deprecated and will be removed in a future release. For plugin-format bundles, prefer [`apm install `](../install/) -- it shares the same air-gapped path, integrates with target resolution, and records deployed files in the project lockfile. `apm unpack` remains the only deploy path for legacy `--format apm` tarballs (see [Behavior](#behavior)). +`apm unpack` is deprecated and will be removed in a future release. For Claude plugin bundles, prefer [`apm install `](../install/) -- it shares the same air-gapped path, integrates with target resolution, and records deployed files in the project lockfile. Legacy `--format apm` bundles still require `apm unpack`, including ZIP archives, tarballs, and unpacked directories (see [Behavior](#behavior)). ::: ## Synopsis diff --git a/packages/apm-guide/.apm/skills/apm-usage/commands.md b/packages/apm-guide/.apm/skills/apm-usage/commands.md index aa6bf6ee10..1f4f5367a6 100644 --- a/packages/apm-guide/.apm/skills/apm-usage/commands.md +++ b/packages/apm-guide/.apm/skills/apm-usage/commands.md @@ -267,7 +267,7 @@ Lifecycle scripts fire on six events: `pre-install`, `post-install`, `pre-update | Command | Purpose | Key flags | |---------|---------|-----------| | `apm pack` | Build distributable artifacts (bundle and/or marketplace.json -- driven by `apm.yml`). A `dependencies:` mapping, including `dependencies: {}`, produces a bundle of local package content; omitted or null `dependencies:` does not. Default output (no format flag) is a Claude Code plugin directory. Pass `--format agent-plugin` to opt into a portable Agent Plugins v1 bundle instead -- strict portable core only (root `plugin.json`, `skills/`, root `mcp.json` written even when empty; no `agents/`, `commands/`, `instructions/`, `extensions/`, `hooks/`, or LSP payload). That bundle build fails before any output is written if the source project has non-portable agents/commands/instructions/extensions/hooks/LSP, naming the surfaces and pointing to `--format claude-plugin` (and to configuring LSP in the target directly, since neither pack format carries it). A packed Agent Plugin installs through the declarative route: declare it as a dependency in `apm.yml` and run `apm install --target copilot`, which keeps the unit whole under `apm_modules/` and registers it without locating or executing Copilot; stable Copilot CLI 1.0.81 or newer is required when loading the projection. The imperative local-bundle route still fails closed for Agent Plugin bundles. Plugin bundles are **target-agnostic**; legacy `--format apm` packaging filters paths by target (see `reference/cli/pack/`). `pack.target` is diagnostic metadata, not authoritative at install time; `pack.bundle_files` (path -> sha256) drives integrity verification. The consumer decides where files land. Dependency content is packed **exclusively** from lockfile-attested `deployed_files` (in every bundle format); the `apm_modules` cache is never packed. Each file is verified against its `deployed_file_hashes` SHA-256 before inclusion, so a file tampered after `apm install` (hash mismatch) or deleted (missing on disk) fails the pack with a message pointing at `apm install`; files with no recorded hash (older lockfiles) pack unverified. Dependency hooks-config / MCP-config is not attested, so it is not packed -- `apm pack` warns (`[!]`) and names the dependency (first-party root hooks/MCP are still packed). Marketplace-publishing projects (`marketplace:` block, no `dependencies:`) no longer emit the misleading "No plugin.json found" warning; after a successful build, a vendor-neutral catalog of artifact paths is appended together with a single docs pointer (`producer/publish-to-a-marketplace/#consume-from-any-assistant`) listing per-assistant install paths. Release-time gates `--check-versions` and `--check-clean` are opt-in and exit non-zero on misalignment / drift (codes 3 and 4 respectively) so release pipelines can fail fast; `--check-clean` is always read-only and never writes pack outputs. The version gate reads a local package's `apm.yml` first; Plugin collections without `apm.yml` use `plugin.json`'s `version`. Invalid or versionless `apm.yml` fails closed, and the fallback likewise rejects malformed or non-object JSON and a missing or blank version. When `apm.yml` declares `target: claude` or `target: copilot` (or the plural `targets:` equivalent), `apm pack` also generates an ecosystem-specific `plugin.json`: `.claude-plugin/plugin.json` for Claude (includes `mcpServers` from `.mcp.json` if present) and `.github/plugin/plugin.json` for Copilot (omits `mcpServers`). An existing file at the target path is preserved (a warning is emitted and the write is skipped) unless `--force` is passed; `--dry-run` prevents writes. Credential-bearing keys and secret-shaped values in `.mcp.json` are stripped recursively at any depth from the Claude manifest before writing, so a committed manifest never leaks secrets (see the apm pack reference, `reference/cli/pack/#credential-stripping-claude-mcpservers`). | `-o PATH`, `--archive` (produce a `.zip` archive instead of a directory; changed from `.tar.gz`), `--archive-format [zip\|tar.gz]` (default `zip`; use `tar.gz` for smaller legacy CI artifacts; only active with `--archive`), `--dry-run`, `--format [plugin\|agent-plugin\|claude\|claude-plugin\|apm]` (`agent-plugin` is the sole selector for the Agent Plugin bundle; `plugin` is a compatibility alias for the Claude plugin bundle, not for `agent-plugin`; `claude`/`claude-plugin` also select the Claude plugin bundle; `apm` selects the legacy APM layout; default `claude-plugin`), `--claude-plugin` (shortcut for `--format claude-plugin`; passing more than one of `--claude-plugin`/`--format` is a usage error), `--force`, `--offline`, `--include-prerelease`, `--marketplace=FORMATS`, `--marketplace-path FORMAT=PATH`, `--json`, `--check-versions` (release gate: per-package versions match `marketplace.versioning.strategy`; exit 3 on failure), `--check-clean` (read-only release gate: regenerate-and-diff against the effective marketplace path, including `--marketplace-path` overrides; never writes pack outputs; exit 4 on drift). `-t/--target` is **deprecated** but still filters legacy APM paths. Exit codes: `0` success, `1` build/runtime error, `2` schema validation error, `3` `--check-versions` misalignment, `4` `--check-clean` drift. | -| `apm unpack BUNDLE` | **[Deprecated]** Restore legacy `--format apm` bundles. Use `apm install ` for Claude plugin bundles with integrity verification and target resolution. | `-o PATH`, `--skip-verify`, `--force`, `--dry-run` | +| `apm unpack BUNDLE` | **[Deprecated]** Legacy `--format apm` bundles still require this command. Use `apm install ` for Claude plugin bundles with integrity verification and target resolution. | `-o PATH`, `--skip-verify`, `--force`, `--dry-run` | For marketplace publishing, `--strict-metadata` preflights every selected marketplace output before any artifact write and exits `5` if remote metadata diff --git a/src/apm_cli/commands/pack.py b/src/apm_cli/commands/pack.py index 7194714d3c..51b33b6459 100644 --- a/src/apm_cli/commands/pack.py +++ b/src/apm_cli/commands/pack.py @@ -876,8 +876,10 @@ def _render_marketplace_catalog(logger, written: list[tuple[str | None, Path]]) @click.command( name="unpack", help=( - "[Deprecated] Extract an APM bundle into the current project. " - "Use 'apm install ' instead -- this command will be removed in a future release." + "[Deprecated] Restore legacy APM bundles. " + "Legacy '--format apm' bundles still require 'apm unpack'. " + "Use 'apm install ' for Claude plugin bundles. " + "This command will be removed in a future release." ), ) @click.argument("bundle_path", type=click.Path(exists=True)) @@ -905,7 +907,8 @@ def unpack_cmd(ctx, bundle_path, output, skip_verify, dry_run, force, verbose): logger = CommandLogger("unpack", verbose=verbose, dry_run=dry_run) logger.warning( "'apm unpack' is deprecated and will be removed in a future release. " - "Use 'apm install ' instead.", + "Legacy '--format apm' bundles still require 'apm unpack'. " + "Use 'apm install ' for Claude plugin bundles.", ) try: logger.start(f"Unpacking {bundle_path} -> {output}") diff --git a/tests/unit/commands/test_pack_cli_surface.py b/tests/unit/commands/test_pack_cli_surface.py index 1ce197a2f0..352d2cc464 100644 --- a/tests/unit/commands/test_pack_cli_surface.py +++ b/tests/unit/commands/test_pack_cli_surface.py @@ -627,14 +627,17 @@ def test_archive_format_default_without_archive_is_ok( class TestUnpackCmd: def test_unpack_deprecation_warning(self, tmp_path: Path) -> None: - """unpack always emits a deprecation warning.""" + """The warning keeps legacy consumers on the supported restore path.""" bundle = tmp_path / "bundle.zip" bundle.write_bytes(b"fake") result = CliRunner().invoke( unpack_cmd, [str(bundle), "--dry-run"], ) - assert "deprecated" in (result.output or "").lower() or result.exit_code in (0, 1) + output = " ".join(result.output.split()) + assert "deprecated" in output + assert "Legacy '--format apm' bundles still require 'apm unpack'." in output + assert "Use 'apm install ' for Claude plugin bundles." in output def test_unpack_nonexistent_bundle(self, tmp_path: Path) -> None: """Passing a non-existent path exits non-zero.""" @@ -645,10 +648,12 @@ def test_unpack_nonexistent_bundle(self, tmp_path: Path) -> None: assert result.exit_code != 0 def test_unpack_help_shows_install_hint(self) -> None: - """Help text references 'apm install' as the replacement.""" + """Help distinguishes legacy restoration from Claude plugin installation.""" result = CliRunner().invoke(unpack_cmd, ["--help"]) assert result.exit_code == 0 - assert "install" in result.output.lower() + output = " ".join(result.output.split()) + assert "Legacy '--format apm' bundles still require 'apm unpack'." in output + assert "Use 'apm install ' for Claude plugin bundles." in output # ---------------------------------------------------------------------------