Repository navigation
Fix shared-skill packing and restore review-panel availability - #2897
Daniel Meppiel (danielmeppiel) with Copilot wants to merge 7 commits into
Conversation
Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 1 | No new architectural concerns at 3ca8dd4. The owned fix remains unchanged, and the merged Agent Plugin no-op outcome rule does not conflict with regular-package packing. |
| CLI Logging Expert | 0 | 0 | 0 | Current-base integration preserves the corrected legacy handoff and unpack guidance. No new logging concerns found; current-head execution remains pending. |
| DevX UX Expert | 0 | 0 | 0 | The current-base merge preserves the reviewed pack/unpack guidance and legacy lifecycle scope. No new DevX findings; current-head execution and publication remain unverified. |
| Supply Chain Security | 0 | 0 | 0 | No supply-chain regression found in the current owned changes or inspected install-outcome interactions at 3ca8dd4. Static confirmation only; current-head qualification remains pending. |
| OSS Growth Hacker | 0 | 0 | 0 | The current-main merge preserves the corrected legacy restore guidance and evidence boundaries. No new adoption-trust concern found in the bounded follow-up. |
| Doc Writer | 0 | 0 | 0 | The owned documentation corrections remain consistent after the main merge; the new Agent Plugin exclusion guidance does not conflict with legacy unpack or Claude plugin install. |
| Test Coverage | 0 | 0 | 0 | Current-base inspection preserves the prior coverage assessment: no new gap found. Native regression remains present; exact-3ca execution and publication are not certified. |
| Performance Expert | 0 | 0 | 0 | No new performance concern after the main merge: owned pack/filter code is unchanged, and inspected install-outcome interactions add no compounding work. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 1 follow-ups
- [DevX UX Expert] POST-MERGE HUMAN/MANUAL: dispatch the trusted-main hosted AI workflow for feat(tls): add corporate CA bundles for package HTTPS #2741 and capture and verify the actual recommendation URL and content. -- This retained, out-of-scope rollout obligation is the only remaining follow-up. Local full-package restoration, deterministic hosted four-file compatibility and local advisory publication do not establish hosted AI delivery; do not declare hosted review availability restored before observing it.
Architecture
classDiagram
direction LR
class TargetProfile {
<<DataclassValueObject>>
+effective_pack_prefixes
+primitives
}
class PrimitiveMapping {
<<DataclassValueObject>>
+deploy_root
+subdir
}
class LockfileEnrichment {
<<Module>>
+_get_target_prefixes()
+_filter_files_by_target()
}
class Packer {
<<Module>>
+pack_bundle()
}
class PackCommands {
<<Module>>
+pack_cmd()
+_render_bundle_result()
+unpack_cmd()
}
class Unpacker {
<<Module>>
+unpack_bundle()
}
class InstallTemplate {
<<Module>>
+_record_agent_plugin_target_skip()
}
class DiagnosticCollector {
+agent_plugin_target_excluded()
+agent_plugin_target_excluded_count
}
class InstallOutcome {
<<Module>>
+finalize_install_result()
}
class InstallResult {
+installed_count
+disposition
}
TargetProfile *-- PrimitiveMapping : deployment policy
LockfileEnrichment ..> TargetProfile : canonical prefix owner
Packer ..> LockfileEnrichment : legacy filtering
PackCommands ..> Packer : build path
PackCommands ..> Unpacker : restore path
InstallTemplate ..> DiagnosticCollector : records exclusion
InstallOutcome ..> DiagnosticCollector : reads exclusion count
InstallOutcome ..> InstallResult : owns final disposition
note for TargetProfile "Dataclass-as-value-object: explicit overrides and narrow derived prefixes remain one policy."
note for PrimitiveMapping "Dataclass-as-value-object: existing deploy_root and subdir metadata."
note for InstallOutcome "New-main context, not an owned PR change: zero-deployment Agent Plugin exclusion affects install outcome, not pack filtering."
class TargetProfile:::touched
class PackCommands:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
PACK["apm pack --format apm --target copilot<br/>src/apm_cli/commands/pack.py: pack_cmd"] --> BUILD["[I/O] src/apm_cli/bundle/packer.py<br/>pack_bundle reads installed lockfile"]
BUILD --> FILTER["bundle/lockfile_enrichment.py<br/>_filter_files_by_target / _get_target_prefixes"]
FILTER --> OWNER["integration/targets.py<br/>TargetProfile.effective_pack_prefixes"]
OWNER --> OVERRIDE{"Explicit pack_prefixes?"}
OVERRIDE -->|yes| EXPLICIT["Use configured prefixes unchanged"]
OVERRIDE -->|no| DERIVE["Root plus narrow primitive deploy_root/subdir prefixes"]
EXPLICIT --> COPY
DERIVE --> COPY
COPY["[I/O] [FS] pack_bundle: existing containment,<br/>verify_attested_file, and file copying"] --> ZIP["[LOCK] [FS] enrich_lockfile_for_pack;<br/>write bundle lockfile and write_zip_archive"]
ZIP --> HANDOFF["[I/O] commands/pack.py: _render_bundle_result<br/>Share with: apm unpack"]
HANDOFF --> UNPACK["[I/O] commands/pack.py: unpack_cmd<br/>legacy-specific deprecation warning"]
UNPACK --> RESTORE["[I/O] [FS] bundle/unpacker.py: unpack_bundle"]
RESTORE --> DRY{"dry_run?"}
DRY -->|yes| PREVIEW["Return planned restore without deployment writes"]
DRY -->|no| FILES["[FS] Existing copytree / copy2 restore"]
RESTORE -->|handled error| ERR["[I/O] unpack_cmd reports failure; exit 1"]
subgraph MainContext["Merged main interaction; not an owned PR change"]
SKIP["install/template.py<br/>_record_agent_plugin_target_skip"] --> DIAG["DiagnosticCollector.agent_plugin_target_excluded"]
DIAG --> FINALIZE["install/outcome.py: finalize_install_result"]
FINALIZE --> ZERO{"installed_count == 0 and<br/>agent_plugin_target_excluded_count > 0?"}
ZERO -->|yes| FAILED["InstallDisposition.FAILED"]
ZERO -->|no| OTHER["Retain other existing outcome checks;<br/>this condition alone does not fail the install"]
end
Recommendation
Recommend ready FOR REVIEW after agent-owned checks are complete, retaining only the post-merge #2741 rollout follow-up. Under existing authorization, the parent may request Sergio, who has eligible write/maintain access but is not yet requested. Zero human reviews exist; Daniel is requested, sole workflow CODEOWNER and latest pusher. CODEOWNER review, independent last-push approval and extra approval for unattributed changes remain required despite numeric approving count zero. This advisory authorizes no PR-state action, approval, merge or waiver. #1844 closure is separately authorized once preserved guard coverage and replacement evidence are linked; no closure decision is made here. Its original e00f905f97eefa6347013ab7a51f83152ce9e898 and branch remain intact; it is not a merge candidate. No external issue is linked to #2897.
Full per-persona findings
Python Architect
- [nit] Retain the existing profile-owned design without additional abstractions. at
src/apm_cli/integration/targets.py:299
The full PR continues to extend TargetProfile.effective_pack_prefixes rather than creating a second prefix authority. The supplied seven replacements alter diff metadata only. Immutable inspection confirms that current main's target-exclusion handling records a typed diagnostic in install/template.py and leaves final command disposition in install/outcome.py. That rule requires zero installed packages plus an Agent Plugin exclusion; it does not redefine legacy pack eligibility or the regular-package fixture's deployment policy. The native closure still installs a regular package, checks shared skill bytes through legacy pack/unpack, and continues into the existing Claude plugin lifecycle. No new split authority or integration conflict was identified. This is source inspection, not successful current-head execution.
Design patterns
- Used in this PR: Dataclass-as-value-object -- TargetProfile and PrimitiveMapping retain deployment policy as the input to pack eligibility.
- Pragmatic suggestion: none -- the bounded correction fits the existing owners.
CLI Logging Expert
No findings.
DevX UX Expert
No findings.
Supply Chain Security
No findings.
OSS Growth Hacker
No findings.
Auth Expert -- inactive
The current owned changes in src/apm_cli/integration/targets.py, src/apm_cli/commands/pack.py, review-panel workflows and skills, architecture guards, documentation, and tests affect packaging and guidance rather than credentials, remote host selection, authorization headers, or auth fallback.
Doc Writer
No findings.
Test Coverage
No findings.
Performance Expert
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently route pack eligibility through the target owner, add concrete workflow/resource guards plus regression coverage, and align CLI/docs messaging without introducing verified defects in the modified surfaces.
Pull request overview
This PR repairs legacy --format apm packing so Copilot-only bundles preserve shared .agents/skills/** payloads (including nested resources), restores PR review-panel workflow availability via a temporary APM 0.30.0 + copilot,agent-skills workaround, and tightens both CLI/docs guidance and regression/contract coverage around pack/unpack and review-panel delivery boundaries.
Changes:
- Extend
TargetProfile.effective_pack_prefixesso legacy pack filtering includes primitive deploy-root overrides (e.g. shared.agents/skills/) while keeping explicitpack_prefixesauthoritative. - Add workflow/runtime guards for the PR review panel: required-resource preflight checks plus a hosted compat job to prove install->pack->restore preserves the panel payload bytes.
- Update pack/unpack CLI messaging + docs + usage skill text, and add tests/architecture guards to prevent re-deriving pack eligibility outside the target owner.
File summaries
| File | Description |
|---|---|
src/apm_cli/integration/targets.py |
Derives default pack prefixes from target root plus overridden primitive deploy roots. |
src/apm_cli/commands/pack.py |
Corrects --target deprecation wording, share-line verb (unpack vs install), and unpack help/warnings. |
scripts/architecture_linter/checks/marketplace_package_and_registration.py |
Enforces pack-eligibility routing through TargetProfile.effective_pack_prefixes (no re-derivation/hardcoded prefixes). |
.apm/architecture/owners/install-deployment.json |
Expands the bundle-native-layout owner decision to include pack eligibility. |
.github/workflows/pr-review-panel.md |
Pins the temporary APM 0.30.0 workaround and adds a pre-agent resource gate for panel files. |
.github/workflows/pr-review-panel.lock.yml |
Regenerates compiled lock to match the new workflow import + pre-agent step. |
.github/workflows/verify-shared-apm-matrix.yml |
Adds an APM 0.30 panel roundtrip compat job (no checkout/secrets) and updates job-set docs. |
apm.lock.yaml |
Updates the deployed review-panel skill hash entries to match the canonical source. |
packages/apm-review-panel/SKILL.md |
Carries forward #1844’s “structured add_comment body, no shell staging” transport boundary + clarified exit verification. |
.agents/skills/apm-review-panel/SKILL.md |
Regenerates the deployed copy to remain byte-identical with the canonical skill. |
tests/unit/test_review_panel_transport_contract.py |
Adds static contract/provenance guards (source == deployed, both hash views match, required transport clauses retained). |
tests/unit/test_shared_apm_workflow_contract.py |
Asserts the workflow pins the workaround and that both source + lock enforce the required panel preflight and compat job shape. |
tests/unit/test_lockfile_enrichment.py |
Verifies shared skills survive target filtering without leaking unrelated shared-root content. |
tests/unit/integration/test_targets_registry_completeness.py |
Adds coverage proving prefixes cover primitive deploy roots and that explicit prefixes remain authoritative. |
tests/integration/test_required_lifecycle_state_machine.py |
Extends the lifecycle closure regression to include nested skill resources preserved through legacy pack + unpack. |
tests/integration/test_pack_shared_skill_roundtrip.py |
New hermetic end-to-end proof that install->pack(zip)->unpack preserves shared skill bytes + recorded hashes. |
tests/integration/test_architecture_install_compound_mutations.py |
Adds mutation cases to ensure pack-prefix ownership can’t regress to prefix-only/hardcoded logic. |
tests/unit/commands/test_pack_phase3.py |
Aligns share-line test coverage for the plugin format handoff. |
tests/unit/commands/test_pack_cli_surface.py |
Verifies share-line verb matches format and that unpack warnings/help distinguish legacy vs plugin bundles. |
packages/apm-guide/.apm/skills/apm-usage/commands.md |
Updates packaged CLI guidance to reflect legacy filtering and the correct restore/install commands. |
docs/src/content/docs/reference/cli/pack.md |
Updates reference text to distinguish plugin install vs legacy unpack and documents target filtering for legacy APM bundles. |
docs/src/content/docs/reference/cli/unpack.md |
Clarifies that legacy APM bundles still require apm unpack across zip/tar/dir forms. |
docs/src/content/docs/integrations/gh-aw.md |
Documents the temporary Copilot skill-bundle workaround and required pre-agent resource checks. |
Review details
- Files reviewed: 23/23 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fix(pack): Preserve shared skills and restore panel delivery
TL;DR
Preserve
.agents/skills/and nested resources in Copilot-only legacy APM bundles by deriving pack eligibility from the existing target owner. Keep the workflow-local APM 0.30.0 workaround until the permanent fix is released, check required panel inputs before inference, and carry #1844's direct structured-comment safeguard into the canonical skill and its deployed ledger.Important
This is #2897's own PR issue-resource bookkeeping, not a newly linked external bug. No merge is authorized. Local panel publication, deterministic hosted bundle compatibility, and a fresh post-merge trusted-main AI run are separate evidence.
Problem (WHY)
KeyError: "There is no item named 'regular-kit-0.1.0/.agents/skills/triage/SKILL.md' in the archive".copilot,agent-skillstarget combination.add_comment.body, without a shell wrapper or intermediate comment file.apm installfor legacy output; the demonstrated restore command isapm unpack.The delivery rule records a concrete incident rather than generic writing advice: "Effective skills are grounded in real expertise.".
Approach (WHAT)
TargetProfileremains the single owner; explicit prefix overrides retain their semantics.unpackin the legacy handoff.install.Implementation (HOW)
src/apm_cli/integration/targets.pyeffective_pack_prefixes; existing lockfile enrichment consumers keep routing through it..apm/architecture/owners/install-deployment.jsonandscripts/architecture_linter/checks/marketplace_package_and_registration.py.github/workflows/pr-review-panel.mdand its compiled lockmain, never PR-head primitives..github/workflows/verify-shared-apm-matrix.ymlpackages/apm-review-panel/SKILL.md.agents/skills/apm-review-panel/SKILL.mdandapm.lock.yamlsrc/apm_cli/commands/pack.pyand pack/usage documentationDiagrams
The highlighted steps preserve the payload and define the publication boundary; the diagram does not claim hosted AI execution.
sequenceDiagram participant I as Install participant P as Legacy pack participant R as Restore participant A as Review agent participant S as Structured safe outputs I->>P: Lockfile-attested deployed files rect rgb(255, 247, 200) P->>P: Resolve prefixes through TargetProfile P-->>R: Shared skills and nested resources R->>A: Check four required nonempty files end rect rgb(255, 247, 200) A->>S: Complete markdown directly in add_comment body end Note over A,S: Hosted receipt is separate from local CLI publicationTrade-offs
Benefits
Validation
Exact final head:
3ca8dd44efb55a66ee30b3345a65574c2b3fd374; incorporated base:e38261c5db4d893d6ddebc3925742e4e3bd2ba74.Local execution receipts and evidence boundaries
The 192 exact-head tests include both format-specific sharing paths, qualified unpack help/warnings, the native legacy branch, fixture roundtrip, unit-level user-root policy consumption, transport/ledger contracts, and the deployment boundary changed by current main. A separate real-process install/Copilot-only pack/unpack run preserved all 12 actual panel package files and recorded SHA-256 values at this head. Reverting only the two unpack messages made both strengthened regressions fail; restoring the committed source passed.
Earlier predecessor evidence includes
477 passed in 231.22s, the native missing-SKILL mutation, and four expected failures after static owner-guard deletion. At predecessorb27c2c09, 15 strict taxonomy/quality tests passed with the unchanged 300-second limit. The subsequent wording-only fold changed no behavioral classification; current assertion-quality and exact-duplicate ratchets passed, and current hosted ratchets remain authoritative. These predecessor receipts are not relabeled as exact-final-head executions or exclusive-host performance evidence.The full lint mirror includes Ruff check/format, YAML I/O, the 2100-line guard, portable relative paths, pylint R0801, auth boundaries, and architecture boundaries.
Current deterministic hosted runs: CI and shared APM compatibility. The published APM 0.30 roundtrip passed and preserved its four required panel files byte-for-byte; that scope is narrower than the local full-package 12-file proof. Live status is authoritative; local success is not a hosted CI success claim.
Scenario Evidence
tests/integration/test_required_lifecycle_state_machine.py::test_required_pack_install_compile_audit_closes_regular_package_state(regression-trap for #2897)tests/integration/test_pack_shared_skill_roundtrip.py::test_install_pack_zip_restore_preserves_shared_skilltests/unit/integration/test_targets_registry_completeness.py;tests/unit/test_lockfile_enrichment.pytests/unit/test_shared_apm_workflow_contract.pytests/unit/commands/test_pack_cli_surface.py::TestRenderBundleResult::test_live_share_line_emittedtests/unit/test_review_panel_transport_contract.py(static instruction/provenance checks, not hosted delivery)tests/unit/compilation/test_user_root_context.pyReview delivery is tracked on the single advisory surface. The baseline compact-input limitation remains historical; the same nine retained reviewers received complete raw inputs plus verified incremental changes and returned current-head reviews. The final CEO synthesis and publication use that same surface. Local publication is not hosted safe-output execution.
How to test
APM_BINARY_PATHpinned to this checkout's executable andAPM_E2E_TESTS=1.tests/integration/test_pack_shared_skill_roundtrip.py, the registry/filtering tests, andtests/unit/test_shared_apm_workflow_contract.py.tests/unit/test_review_panel_transport_contract.pyand confirm source/deployment/ledger parity.Spec conformance
No normative OpenAPM format or target deployment rule changes. This repairs legacy bundle filtering, provisioning, and internal review delivery; no spec ratification is claimed.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com