Repository navigation
fix(install): deploy skills through home directory aliases - #2876
Daniel Meppiel (danielmeppiel) merged 20 commits into
Conversation
Normalize the skill source root on a shallow metadata copy while preserving source-plan authorization and descendant symlink rejection. Add global audit and skill layout regression coverage. 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.
Copilot review overview
🟡 Changes recommended
The new CHANGELOG entry references the closed issue number instead of ending with the PR number per the repo’s changelog format contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Lite
Findings: 2
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
CHANGELOG.md — The new changelog bullet ends with #2867, but changelog entries are required to end with the pull… |
|
tests/unit/install/test_security_scan_scope.py — In the MARKETPLACE_PLUGIN branch the test writes .claude-plugin/plugin.json, but the subsequent… |
What changed in this PR
Fixes a global skill-install edge case where deploying skill files could be silently skipped when HOME/APM_HOME is a directory-symlink alias, by canonicalizing the package root spelling used during skill integration while preserving existing authorization and descendant-link protections.
Changes:
- Canonicalize
PackageInfo.install_pathon a shallow copy insideSkillIntegrator.integrate_package_skill()to align withDeployableSourcePlanroot resolution. - Add regression coverage for aliased home paths in the global install/audit integration test and add a multi-layout unit/component test ensuring authorization is preserved under root aliases.
- Document the root-alias behavior in CLI install docs and add an Unreleased changelog entry.
| File | Description |
|---|---|
| src/apm_cli/integration/skill_integrator.py | Shallow-copies package metadata and resolves the package root before skill routing/copy so source-plan authorization matches copy/discovery paths. |
| tests/unit/install/test_security_scan_scope.py | Adds a multi-layout test asserting root-alias normalization does not broaden authorization and still rejects symlink escapes. |
| tests/integration/test_global_audit_deploy_root.py | Adds a parametric regression test for global install + audit when HOME/APM_HOME uses a directory symlink alias. |
| docs/src/content/docs/reference/cli/install.md | Documents that global skill installation supports home directory alias spellings while keeping in-package symlink rejection unchanged. |
| CHANGELOG.md | Records the fix under Unreleased. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 0 | Canonical owners are reused, but frozen semver transport drift escapes the shared predicate. Historical P8 passes do not establish final integration acceptance. |
| Cli Logging Expert | 0 | 0 | 0 | At a42 vs 103, frozen help/refusals retain ASCII and CommandLogger routing; ownership and freshness changes add no output noise. No CLI findings; execution evidence was not refreshed. |
| Devx Ux Expert | 0 | 1 | 0 | Alias ownership, canonical paths, scoped lock replay and removal/reinstall align with user expectations; complete the advertised frozen transport check for Git semver ranges. |
| Supply Chain Security Expert | 0 | 1 | 0 | Root/source authorization remains intact; fix frozen semver transport comparison. Verified historical a42 lifecycle results do not certify a future merged candidate. |
| Oss Growth Hacker | 0 | 1 | 0 | The alias reliability story is grounded; the changelog should also disclose changed frozen, freshness, and dependency-alias behavior. |
| Auth Expert | 0 | 0 | 0 | No auth regression found at a42fe8081: canonical provider/credential identity preserves equivalence and scoped lock replay. Focused checks: 165 passed, 4 skipped; native/CI acceptance pending. |
| Doc Writer | 0 | 1 | 0 | Changed install docs match the candidate's scope; qualify one remaining unconditional lock-replay promise in the packaged dependency guide. |
| Test Coverage Expert | 1 | 1 | 0 | Existing lifecycle assertions and historical a42 native execution are verified; final integrated driver/parent evidence and exact owner-evidence bindings remain pending. |
| Performance Expert | 0 | 1 | 0 | Scoped lock seeds preserve replay without stale sibling reuse; fresh-result coalescing remains intact. Repeated global ownership indexing is a non-blocking scaling follow-up, not speed evidence. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[Test Coverage Expert] (blocking-severity) Mandatory before completion: obtain final integrated native, owner, mutation and independent acceptance evidence. -- After the human prerequisite landings, actual-main repin, stable release-adapted harden(governance): require native lifecycle evidence before shipping #2882 through fix(release): preserve prereleases and qualify exact native candidates #2879, permitted fix(alias): reject path-traversing dependency aliases — fixes #2900 #2901 integration and all committed folds, run scripts/check_lifecycle_evidence.py --base --head --lane full --report from the clean candidate. Until then, combined qualification is restricted to a named unpublished stable-parent diagnostic: no direct fix(alias): reject path-traversing dependency aliases — fixes #2900 #2901 merge or independent runtime-composite push, and no borrowed upstream acceptance. The parent must independently rerun the full gate using --completion and a separate fresh report. Revalidate the executable command map and preserve all eight witness selectors, three scope variants and 25 rules plus the mandatory initializer through Copilot/apmlifecycle phase2 interactions #2813. Reconcile the existing global, prune/uninstall and generated fixtures identified in compatibility-2901.json with validated explicit-alias placement and lock alias round-trip, independently of source identity and HOME symlink spelling. Preserve exact SHA, deployed bytes, source-key uninstall selection, ownership, hooks, audit and unrelated-user-file checks; keep independent reference expectations rather than computing them from the production path helper. Actual stable harden(governance): require native lifecycle evidence before shipping #2882/fix(alias): reject path-traversing dependency aliases — fixes #2900 #2901 runtime integration, current-remote receipt/locked-seed compatibility and native execution remain pending. Bind every detected exact touched_owners[].decision values to executed consumer rows with head_sha, invocation/report and passing run_evidence. Supply mutation-failure/restoration passes, static/lint, final panel/docs-sync, exact hosted CI and real branch approvals. This is required evidence, not a post-merge deferral; historical a42 success, the interim project-only native pass and aggregate counts cannot discharge it.
-
[Python Architect] Fold the semver transport correction into drift.detect_ref_change and extend existing native refusal/recovery coverage. -- Compare declared Git HTTP/HTTPS transport before the unchanged-semver return, preserving update_refs behavior, unchanged-range replay and equivalent providers. Cover both transport directions in drift/plan tests and the existing hermetic command/lifecycle fixture: refuse without durable mutation, restore the declaration and succeed. Add the canonical-owner regression guard and mutation/restoration evidence rather than a frozen-only identity helper. This closes the shared structural omission identified independently by architecture, DevX and supply-chain reviewers without inventing a credential exploit.
-
[Doc Writer] Replace the unconditional locked-SHA sentence in packages/apm-guide/.apm/skills/apm-usage/dependencies.md:672. -- State that plain installs replay matching lock entries while mutable Git refs without a matching entry resolve upstream when materialized. Link commands.md for frozen/refresh details instead of duplicating the policy. Also reconcile the four placement-table rows and concise explanation with published fix(alias): reject path-traversing dependency aliases — fixes #2900 #2901: HOME/APM_HOME symlink spelling is separate from declared dependency alias; get_install_path/build_materialization_path selects validated explicit-alias placement, and LockedDependency.alias records placement rather than source identity. Remove claims that explicit aliases affect only logs/display or that canonical layout necessarily excludes aliases. Preserve safe dotted names and original declaring-source anchoring. These prose edits are committed at6a0a95edd75431bfd22ce6d758d7c4666c3b3a09, not completed combined qualification. This remains an in-PR truthfulness correction, not authorization for new alias-drift, relocation or persistence policy.
-
[Oss Growth Hacker] Expand the existing fix(install): deploy skills through home directory aliases #2876 changelog entry and add concise recovery/migration guidance. -- Disclose changed frozen source-identity/full-commit checks and upstream resolution for unseeded mutable refs, while retaining the HOME-alias repair and Dave Mead (@DaveMeadAdjust)'s credit. Describe canonical materialization as placement selected by get_install_path/build_materialization_path, including a validated explicit dependency alias; distinguish that placement from HOME/APM_HOME symlink resolution and source identity. LockedDependency.alias round-trips placement. For intentional source/ref changes, retain the existing guidance to regenerate and review the lock with apm install --update, and link the install reference. Do not promise automatic relocation when an alias changes, frozen alias-drift detection, global root-local persistence, network freshness under --frozen, historical credential pinning, universal offline operation, or a ratified release.
-
[Performance Expert] Retain FOLD for committed integrate-phase ownership reuse and its single-builder relocation; execute the unchanged qualification obligations when authorized. -- Exact seam, confirmed at dd3ac38bd1c0e0600ceba2bf194d0dcec1a11e3c: src/apm_cli/install/pipeline.py:_run_integration_phase (line 102 there) encloses only _run_phase('integrate', _integrate_phase, ctx), after target initialization/download and before failure handling, cleanup, LockfileBuilder.build_and_save, post_deps_local and audit. The committed contextlib.nullcontext branch for ctx.lockfile_only fixes the earlier unconditional skill lookup. Retain this branch: targets initialization is skipped in lockfile-only mode, and materialization must still feed lock construction without creating integrators or skipping the whole phase.
The relocation at 84f13a102c2396df3809bd0c700b9d5c8684ec09 remains inside this FOLD. Existing skill_support.py:build_skill_ownership_maps is the single executable builder, exposed through SkillIntegrator._build_ownership_maps = staticmethod(build_skill_ownership_maps). Its executable body is unchanged; lazy imports, outputs and consumer class/instance monkeypatch lookup are preserved. Keep the support/pipeline provenance selectors and legacy skill_integrator selector with their respective existing owners, without duplicate guard IDs. The static guard and source-override mutations follow both the alias and support root lookup. The reported executable-body AST identity and integrator 2099/support 123 lines respect the unchanged 2100 cap; they are not runtime results. Native, bundle and standalone-subskill consumers still use the instance _ownership_maps accessor and that class seam. Lazily retain the two returned dictionaries behind MappingProxyType for each resolved metadata root; no full-index copy, alternative builder, path/stat memo, process-global cache or fallback to an earlier ctx lock representation. One first-use snapshot per root is sufficient here; it is not a simultaneous multi-root transaction. Keep uncached standalone entry points fresh. Normal command execution creates a new integrator; finally drops durable maps on every context exit, and nested contexts remain rejected rather than silently sharing another lifetime.
Required qualification guards remain unexecuted: (1) Drive native, bundle and standalone-subskill package integration through this seam with real populated lockfiles and forwarding counters, including global metadata roots different from deployment roots. Require one canonical load/build per used root per phase, zero for an unused root, and at most 15x N-to-10N ownership-record visit growth; check exact owner identities and observable collision/deployment results, not counts alone. (2) Run consecutive commands through fresh InstallContexts, including reinstall/update, replace/delete/create the lock between epochs, and verify new ownership; also exit and re-enter the context on the same integrator to prove durable maps are not retained. Exercise exceptional exit after maps exist, a per-package failure caught inside the phase, retry, and nested-context refusal. Preserve the current-run overlay throughout a batch; a new command receives a fresh integrator, not an inherited overlay. (3) Verify two distinct resolved metadata roots cannot reuse each other's durable maps, while two symlink spellings of one physical metadata root may share a build. These filesystem aliases are not declared dependency placement aliases. Test separate project/user commands with the same skill names and different owners; do not infer that the skill-name-keyed session overlay supports mixing independent deployment roots in one run. (4) Guard the actual integrate phase's write set: after dependency deployments and again after root primitives, durable lockfile bytes or absence must be unchanged while deployed files and ctx package/local records change. Preserve root-local collision precedence through its augmented managed-files set. The project/user consecutive-pipeline test added at dd3ac38bd retains these shared observations; its persisted-ownership expectation is corrected at 5ace7461935a89b4ed2d36327f4c9cde7bea7894 to distinguish project-local persistence from user dependency ownership. Do not require or introduce global root-local persistence: post_deps_local explicitly skips USER scope. Verify later same-run native collisions consult mutable _native_skill_session_owners when durable ownership is absent, retaining durable-map-first precedence. Do not cache target existence, content comparisons, managed_files or session owners. If a permitted on-disk ownership writer appears between package calls, delimit a new epoch before subsequent reads; the guard must fail rather than bless stale maps. Test replacement at valid phase boundaries and freshness afterward, including initially absent locks. (5) Preserve project/global lockfile-only behavior, no-target/no-skill paths and zero new deployment side effects. Verify drift replay creates fresh integrators and leaves live lock/governed roots unchanged; do not wrap replay or uninstall in this context. Preserve applicable deterministic/generated lifecycle coverage and final P8 execution, subject to the VP hold.
Intermediate-mutation basis is unchanged: integrate prepares materializations, sequentially deploys packages and root primitives, and records package/local outcomes in ctx. Root-local integration uses the same skill instance without persisting the lock. Hook/primitive/config writes are not inputs to the durable maps; the approval prompt may save personal executable consent, not lock ownership. The primitive registry does not dispatch MCP/LSP lock writers. Dependency lock persistence and applicable PROJECT-local persistence remain after snapshot exit. Drift replay is a separate scratch operation with fresh integrators and a live-project read-only guard, not an intermediate writer. InstallTransaction and install/locking.py serialize cooperating mutating commands; this is not protection against arbitrary external filesystem edits or permission to broaden concurrency semantics.
Measured limits and scope: ownership-builder-measurement.log establishes identical builder counts at 1030295 and a42fe808128d5866c17d93fe63ed4b57d0426ea0, not a full-install profile or post-relocation result. P counts calls reaching the builder, not every declared package. Accounting for get_all_dependencies sorting, the fuller preexisting comparison-based bound is O(P*(B + D log D + F)), excluding path/identity-string work; one stable root/epoch could reduce that to O(B + D log D + F + P). For K epochs pay K builds. No dominant phase, elapsed-time regression or end-to-end speedup is established. Absence of a new regression alone is not a rubric-compliant deferral reason. scope_boundary_crossed for excluded work: reuse beyond this ordered integration phase into replay/uninstall, persistent cross-command caches or general writer invalidation would add a new cache-lifetime domain to this HOME-alias ownership repair. Frozen alias-drift, automatic alias relocation, graph-wide alias-conflict policy and global root-local persistence remain outside this fold. Published #2901 owns validated placement and source identity; this amendment authorizes only the stated documentation/fixture reconciliation, not an independent compatibility implementation or merge. The committed relocation, corrected expectations and queued tests do not qualify the candidate for shipping.
Architecture
### Before: 103029508
```mermaid
classDiagram
direction LR
class InstallService {
+run(request)
+enforce_frozen(request)
}
class install_plan {
<<Module>>
+lockfile_satisfies_manifest()
}
class DependencyReference {
+get_unique_key()
+get_install_path()
}
class LockFile {
+read(path)
}
class LockedDependency {
+to_dependency_ref()
}
class RefFreshnessPolicy {
<<Strategy>>
REPRODUCIBLE
CURRENT_REMOTE
}
class TieredRefResolver {
+seed(repo_ref, ref, sha)
+resolve(repo_ref)
}
class RefResolutionTier {
<<Protocol>>
+try_resolve(dep_ref, ref)
}
class PerRunRefCache
class host_providers {
<<Module>>
+classify_host_provider()
}
class HostProviderDescriptor {
<<ValueObject>>
kind
credential_purpose
}
class drift {
<<Module>>
+detect_ref_change()
}
class BaseIntegrator {
<<Base>>
}
class SkillIntegrator {
<<Subclass>>
+integrate_package_skill()
-_build_ownership_maps(project_root)
}
class PackageInfo
class DeployableSourcePlan {
<<ValueObject>>
+copy_ignore()
}
class _ModelFixture {
<<ValueObject>>
+create(root, binary)
}
class RuleBasedStateMachine
class _LifecycleReferenceModel
class LifecycleStateSnapshot {
<<ValueObject>>
+capture()
}
InstallService ..> install_plan
install_plan ..> DependencyReference : membership keys only
install_plan ..> LockFile
LockFile *-- LockedDependency
LockedDependency ..> DependencyReference
TieredRefResolver o-- RefResolutionTier
TieredRefResolver *-- PerRunRefCache : seeds and fresh answers share cache
TieredRefResolver ..> RefFreshnessPolicy
TieredRefResolver ..> DependencyReference
host_providers ..> HostProviderDescriptor
BaseIntegrator <|-- SkillIntegrator
SkillIntegrator ..> PackageInfo : lexical install_path
SkillIntegrator ..> DeployableSourcePlan
SkillIntegrator ..> LockFile : deployment-root lookup
RuleBasedStateMachine <|-- _LifecycleReferenceModel
_LifecycleReferenceModel *-- _ModelFixture
_LifecycleReferenceModel ..> LifecycleStateSnapshot
note for TieredRefResolver "Chain of Responsibility: L0PerRunCache -> L1CommitsAPI -> L2BareRevParse -> L3LegacyClone; policy selects eligible tiers"
note for _ModelFixture "Factory: create builds the project fixture"
note for _LifecycleReferenceModel "Project-only generated model; no mandatory initializer spine"
After: a42fe808
classDiagram
direction LR
class InstallService {
+run(request)
+enforce_frozen(request)
}
class install_plan {
<<Module>>
+lockfile_satisfies_manifest()
}
class DependencyReference {
+get_unique_key()
+get_install_path()
}
class LockFile {
+read(path)
}
class LockedDependency {
+to_dependency_ref()
}
class RefFreshnessPolicy {
<<Strategy>>
REPRODUCIBLE
LOCKED_OR_CURRENT
CURRENT_REMOTE
}
class TieredRefResolver {
+seed(repo_ref, ref, sha)
+resolve(repo_ref)
-_lock_seed_key(dep_ref, ref)
-_lock_seeds
}
class RefResolutionTier {
<<Protocol>>
+try_resolve(dep_ref, ref)
}
class PerRunRefCache
class host_providers {
<<Module>>
+classify_host_provider()
+effective_host_provider_identity()
}
class HostProviderDescriptor {
<<ValueObject>>
kind
credential_purpose
}
class drift {
<<Module>>
+detect_ref_change()
}
class BaseIntegrator {
<<Base>>
}
class SkillIntegrator {
<<Subclass>>
+integrate_package_skill()
-_build_ownership_maps(lockfile_root)
}
class PackageInfo
class DeployableSourcePlan {
<<ValueObject>>
+copy_ignore()
}
class _ModelFixture {
<<ValueObject>>
+create(root, binary, variant)
}
class RuleBasedStateMachine
class _LifecycleReferenceModel {
+mandatory_spine()
+durable_state_matches_reference_model()
}
class LifecycleStateSnapshot {
<<ValueObject>>
+capture()
}
InstallService ..> install_plan
install_plan ..> DependencyReference : canonical key and full pin
install_plan ..> drift : declared identity predicate
install_plan ..> LockFile
LockFile *-- LockedDependency
LockedDependency ..> DependencyReference
drift ..> host_providers
TieredRefResolver o-- RefResolutionTier
TieredRefResolver *-- PerRunRefCache : fresh repository answers only
TieredRefResolver ..> RefFreshnessPolicy
TieredRefResolver ..> DependencyReference : scoped seed identity
TieredRefResolver ..> host_providers : scoped seed provider
host_providers ..> HostProviderDescriptor
BaseIntegrator <|-- SkillIntegrator
SkillIntegrator ..> PackageInfo : shallow copy then root resolve
SkillIntegrator ..> DeployableSourcePlan : unchanged descendant authority
SkillIntegrator ..> LockFile : canonical scope metadata root
RuleBasedStateMachine <|-- _LifecycleReferenceModel
_LifecycleReferenceModel *-- _ModelFixture
_LifecycleReferenceModel ..> LifecycleStateSnapshot
note for TieredRefResolver "Chain of Responsibility retained; matching dependency seed precedes fresh L0 and remote tiers; LOCKED_OR_CURRENT excludes L2BareRevParse"
note for _ModelFixture "Factory: create constructs project, global-canonical or global-aliased fixture"
note for _LifecycleReferenceModel "Hypothesis initialize calls _mandatory_replay before bounded generated tails"
note for SkillIntegrator "Base + subclass retained; plugin admission precedes copy; scope.get_apm_dir selects ownership root"
note for DependencyReference "Download and integrate now both delegate to get_install_path -> materialization.build_materialization_path"
class install_plan:::touched
class RefFreshnessPolicy:::touched
class TieredRefResolver:::touched
class host_providers:::touched
class drift:::touched
class SkillIntegrator:::touched
class _ModelFixture:::touched
class _LifecycleReferenceModel:::touched
class LifecycleStateSnapshot:::touched
classDef touched fill:#fff3b0,stroke:#d47600
```mermaid
### Before: 103029508
Paths below are under src/apm_cli/. The flow focuses on Git skill installation; existing policy, executable and MCP gates remain in force.
```mermaid
flowchart TD
A["commands/install.py: install --global, optional --frozen"] --> B{"install/service.py: InstallService.run; frozen?"}
B -->|yes| C["[I/O] InstallService.enforce_frozen: LockFile.read; install/plan.py: lockfile_satisfies_manifest checks direct keys"]
C --> D{"Structural and applicable MCP checks satisfied?"}
D -->|no| X["commands/install.py: FrozenInstallError -> exit 1"]
D -->|yes| E
B -->|no| E["[I/O] install/phases/resolve.py: run -> _load_lockfile -> _setup_downloader"]
E --> F{"deps/tiered_ref_resolver.py: ref_freshness_policy_for_install"}
F -->|ordinary install| G["REPRODUCIBLE: permit lock seeds and persistent L2"]
F -->|update or refresh| H["CURRENT_REMOTE: no lock seeds or L2"]
G --> I["[LOCK] install/helpers/ref_seed.py: seed_ref_resolver_from_lockfile -> TieredRefResolver.seed -> PerRunRefCache.put"]
I --> J{"TieredRefResolver.resolve: ref kind?"}
H --> J
J -->|full SHA| K["TieredRefResolver._build_result: SHA passthrough"]
J -->|omitted or Artifactory| L["[NET] [EXEC] L3LegacyClone.resolve_full"]
J -->|named| M{"[LOCK] PerRunRefCache.get: repository/ref hit?"}
M -->|yes, including lock seed| K
M -->|no| N["[NET] [EXEC] TieredRefResolver._dispatch: L1CommitsAPI, eligible L2BareRevParse, L3LegacyClone"]
N --> K
L --> K
K --> O["[I/O] [FS] install/phases/download.py: run; alias join or get_install_path"]
O --> P["[I/O] [FS] install/phases/integrate.py: run; alias join or get_install_path; install/services.py: integrate_package_primitives"]
P --> Q["[I/O] integration/skill_integrator.py: integrate_package_skill; enforce_agent_plugin_deployment_boundary"]
Q -->|admitted| R["PackageInfo.install_path remains lexical"]
R --> S["[I/O] _build_ownership_maps(project_root): LockFile.read from deployment root"]
S --> T["[I/O] [FS] _integrate_native_skill / _integrate_skill_bundle / _promote_sub_skills_standalone; shutil.copytree with DeployableSourcePlan.copy_ignore"]
T --> U["[FS] [LOCK] install/pipeline.py: cleanup.run -> LockfileBuilder.build_and_save -> audit.run -> finalize.run"]
U --> V["InstallResult: success exit 0 or failure exit 1; filtered-out skill bytes could still leave exit 0"]
After: a42fe808
flowchart TD
A["commands/install.py: install --global, optional --frozen"] --> B{"install/service.py: InstallService.run; frozen?"}
B -->|yes| C["[I/O] InstallService.enforce_frozen: LockFile.read; install/plan.py: lockfile_satisfies_manifest"]
C --> C1{"Direct key present and full revision pin equals resolved_commit?"}
C1 -->|no| X["commands/install.py: FrozenInstallError -> exit 1"]
C1 -->|yes| C2["drift.py: detect_ref_change -> host_providers.effective_host_provider_identity; source/ref/provider/HTTP comparison"]
C2 --> D{"No drift and applicable MCP checks satisfied?"}
D -->|no| X
D -->|yes| E
C2 -.-> W["Review correction: matching Git-semver constraint currently returns before HTTP-transport comparison"]
B -->|no| E["[I/O] install/phases/resolve.py: run -> _load_lockfile -> _setup_downloader"]
E --> F{"deps/tiered_ref_resolver.py: ref_freshness_policy_for_install"}
F -->|lock exists, ordinary install| G["LOCKED_OR_CURRENT: matching seeds allowed; no persistent L2"]
F -->|no lock, update or refresh| H["CURRENT_REMOTE: no lock seeds or L2"]
G --> I["[LOCK] install/helpers/ref_seed.py: seed_ref_resolver_from_lockfile -> TieredRefResolver.seed -> _lock_seeds using _lock_seed_key"]
I --> J{"TieredRefResolver.resolve: ref kind?"}
H --> J
J -->|full SHA| K["TieredRefResolver._build_result: SHA passthrough"]
J -->|omitted or Artifactory| L["[NET] [EXEC] L3LegacyClone.resolve_full"]
J -->|named| M{"[LOCK] _lock_seeds.get: dependency/repository/provider/ref match?"}
M -->|yes| K
M -->|no| M2{"[LOCK] PerRunRefCache.get: fresh repository/ref answer?"}
M2 -->|yes| K
M2 -->|no| N["[NET] [EXEC] TieredRefResolver._dispatch: L1CommitsAPI -> L3LegacyClone; remote failure is not replaced by L2"]
N --> K
L --> K
K --> O["[I/O] [FS] install/phases/download.py: run -> DependencyReference.get_install_path"]
O --> P["[I/O] [FS] install/phases/integrate.py: run -> DependencyReference.get_install_path; install/services.py: integrate_package_primitives"]
P --> Q["[I/O] integration/skill_integrator.py: integrate_package_skill; enforce_agent_plugin_deployment_boundary"]
Q -->|admitted| R["[I/O] copy(package_info); install_path.resolve root only"]
R --> S["[I/O] scope.get_apm_dir(USER) -> _build_ownership_maps(lockfile_root); independent of deployment root"]
S --> T["[I/O] [FS] _integrate_native_skill / _integrate_skill_bundle / _promote_sub_skills_standalone; shutil.copytree with unchanged DeployableSourcePlan.copy_ignore"]
T --> U["[FS] [LOCK] install/pipeline.py: cleanup.run -> LockfileBuilder.build_and_save -> audit.run -> finalize.run"]
U --> V["InstallResult: success exit 0 or failure exit 1; lifecycle assertions check bytes, ownership and unrelated files"]
classDef touched fill:#fff3b0,stroke:#d47600
class C,C1,C2,G,I,M,O,P,R,S,W touched
### Recommendation
Keep all original FOLDs and P8 holds in the same panel/outer iteration. The phase snapshot, nullcontext correction, single-builder relocation and scope-correct USER expectation are committed but runtime qualification remains queued. Fold the approved documentation/fixture reconciliation with #2901's published placement contract: declared alias is placement, not source identity; HOME/APM_HOME symlink spelling is separate. The repaired project semver fixture has a reported interim pass of 1 test/nine real CLI commands in 30.93 seconds under quiet-2876-semver-1, with explicit own-workspace APM_BINARY_PATH and apm_cli.__file__ identity. It ran at dirty HEAD 843386ea1073373f95d5d8bc588fb057cd8747ab plus git-diff SHA256 b86517479bf9afe414fd4e6c317f3a70ecaceffb68a49286f23a6b57a91aabfe with the native fixture and ownership prototype dirty, before helper commit 60a9a5ca85829d0f83989f5aa5fc5b04cb0ae815; it certifies neither that later commit nor final P8. Actual stable #2882/#2901 runtime integration, the two global native variants and full-provider execution remain pending. No direct #2901 merge or independent runtime-composite push is authorized: combined qualification remains a named unpublished stable-parent diagnostic until human prerequisite landings and actual-main repin. Resume execution only in an authorized VP slot and obtain all final-head evidence, including the independent parent --completion rerun. Historical passes remain historical; this amendment is not merge approval, CI certification or a final ship claim.
---
<details>
<summary>Full per-persona findings</summary>
#### Python Architect
- **[recommended]** Apply transport drift before the Git-semver early return in the canonical predicate at `src/apm_cli/install/plan.py:502`
The new frozen identity gate correctly delegates to drift.detect_ref_change, but that existing predicate returns immediately for ref_kind == 'semver' at src/apm_cli/drift.py:190-191, before its is_insecure comparison at lines 199-201. On exact candidate a42fe808, real DependencyReference.parse_from_dict inputs for owner/package with ref '^1.0.0' and a lock with constraint '^1.0.0', resolved_ref 'v1.2.0', and a full commit produce detect_ref_change=False and lockfile_satisfies_manifest=(True, []) for BOTH HTTPS-to-HTTP and HTTP-to-HTTPS changes. Equivalent branch-ref controls produce drift=True and frozen=False. Thus the newly strengthened frozen contract still accepts a changed declared HTTP transport for Git-semver dependencies. This is an inherited predicate limitation exposed by the new caller, not a newly introduced credential bypass; recommended is appropriate because the bounded probe establishes a structural-contract omission, not an end-to-end unauthorized download. Extend the existing decision owner instead of duplicating transport logic inside install/plan.py.
**Design patterns**
- Used in this PR: Base + subclass -- SkillIntegrator retains BaseIntegrator infrastructure while changing root selection and forwarding.
- Used in this PR: Strategy + Chain of Responsibility -- RefFreshnessPolicy selects cache eligibility for the TieredRefResolver tier chain; dependency-scoped lock seeds no longer masquerade as repository-wide fresh answers.
- Used in this PR: Dataclass-as-value-object + Factory -- DeployableSourcePlan preserves immutable source authorization, and the frozen _ModelFixture.create factory constructs explicit project/global lifecycle variants.
- Pragmatic suggestion: none -- the existing owners are the right boundaries; correcting the common drift predicate is simpler than adding another identity abstraction.
*Suggested:* Move the Git HTTP-transport comparison in drift.detect_ref_change ahead of the Git-semver return, preserving existing update_refs and source semantics. Add bidirectional HTTP/HTTPS semver cases to tests/unit/test_drift_detection.py and tests/unit/install/test_plan.py, then prove frozen refusal, unchanged durable state, and recovery through the existing command/lifecycle fixture. Extend the existing architecture guard so the transport decision remains in the shared drift owner rather than acquiring a frozen-only sibling.
*Proof (manual):* -- proves: A frozen Git-semver declaration can currently change between HTTP and HTTPS without being rejected by the structural identity check. [governed-by-policy, portability-by-manifest]
#### Cli Logging Expert
No findings.
#### Devx Ux Expert
- **[recommended]** Apply the frozen transport check to Git semver ranges too at `src/apm_cli/install/plan.py:502`
At a42fe808128d5866c17d93fe63ed4b57d0426ea0 against 103029508a8fbf863c55fdd224be63461e115a2c, the new frozen preflight delegates declared-identity comparison to detect_ref_change(). For a Git dependency with ref_kind='semver' and an unchanged constraint such as '^1.2.0', drift.py:190-191 returns False before the is_insecure comparison at lines 199-201. The dependency key does not distinguish HTTP from HTTPS, so changing only that declared transport can still satisfy this structural check. This conflicts with the updated install help and reference promising refusal of declared transport changes. Recommended rather than blocking: the helper's early return predates this PR, and this review establishes an incomplete newly advertised contract by static tracing, not a reproduced CLI regression or a security judgment. No native execution was performed; the historical eight passing lifecycle witnesses were inspected and are not being called absent or failed.
*Suggested:* Keep detect_ref_change as the shared authority, but compare the existing declared transport identity before the Git-semver early return. Preserve unchanged-range replay and equivalent inferred/explicit providers. Add a focused frozen CLI fixture covering an unchanged Git semver constraint with an HTTP/HTTPS declaration flip, asserting refusal and unchanged durable state. This needs no new flag, lock schema, or persistence of historical host configuration.
*Proof (unknown):* -- proves: The new frozen structural check has a statically visible path that accepts a declared transport change when the Git semver constraint is unchanged; CLI execution of that combination was not verified. [devx, governed-by-policy]
#### Supply Chain Security Expert
- **[recommended]** Check transport drift before the Git semver early return. at `src/apm_cli/install/plan.py:503`
At a42fe808128d5866c17d93fe63ed4b57d0426ea0 against 103029508a8fbf863c55fdd224be63461e115a2c, the new frozen check delegates declared identity validation to detect_ref_change. For a Git dependency with reference '^1.2.0' and a lock with constraint '^1.2.0', drift.py:190-191 returns False before comparing is_insecure at lines 199-201. Changing the declaration between HTTP and HTTPS therefore passes this structural check when host, provider and package key otherwise match; the key deliberately excludes transport. InstallService.enforce_frozen adds MCP checks but no independent transport comparison. The early return predates this PR, but its newly expanded frozen contract now inherits the gap, contrary to the transport-refusal promise in reference/cli/install.md. Recommended rather than blocking: explicit HTTP consent and commit-integrity controls remain separate safeguards, and this review has not demonstrated a network/authentication bypass. This is static candidate-bound evidence, not an executed CLI failure.
*Suggested:* Have the existing detect_ref_change authority compare Git transport before returning for an unchanged semver constraint. Add a frozen regression for both HTTP-to-HTTPS and HTTPS-to-HTTP with identical constraints, asserting refusal before mutation and acceptance after restoring the declaration. Coordinate any shared identity edits with the exclusive #2901 owner; no new identity helper or lockfile schema is needed.
*Proof (manual):* -- proves: The frozen structural check can accept changed Git transport when the declared semver constraint is unchanged. [secure-by-default, governed-by-policy]
#### Oss Growth Hacker
- **[recommended]** Disclose the broader install behavior changes in the release-facing entry at `CHANGELOG.md:12`
CHANGELOG.md:12 describes only global home-directory aliases, but this exact candidate also changes behavior for users without home aliases. install/plan.py now refuses mismatched manifest-declared identity and full commit pins under --frozen; tiered_ref_resolver.py distinguishes matching dependency-scoped lock seeds from unseeded mutable refs; the changed registry and packaged dependency guides replace the documented alias-controlled directory with canonical materialization. These changes affect CI refusal, upstream access on unlocked materialization, and expectations about installed paths. The detailed install reference explains frozen and freshness behavior, but an upgrader reading the changelog would not learn that this PR affects them. This is recommended change communication, not a correctness finding, a demand for new functionality, or a semver verdict.
*Suggested:* Extend the existing #2876 entry with concise user-facing clauses: frozen installs reject changed declared Git identity or mismatched full commit pins; plain installs replay matching lock entries but resolve unseeded mutable refs upstream; dependency aliases retain their selection/display role while materialization uses canonical paths. Link the install reference for recovery guidance and retain @DaveMeadAdjust's credit. Do not describe frozen as network freshness, historical credential-configuration pinning, or universal offline installation.
#### Auth Expert
No findings.
#### Doc Writer
- **[recommended]** Qualify the packaged dependency guide's unconditional locked-SHA promise at `packages/apm-guide/.apm/skills/apm-usage/dependencies.md:672`
At a42fe808, dependencies.md still says install without --update 'always uses the locked SHA'. This contradicts the revised commands.md and install reference: ordinary installs replay only matching lock entries, changed declarations can require resolution, and unseeded mutable refs resolve upstream even with unrelated or sibling lock entries. The next sentence also lists --refresh as an upstream operation, although --refresh does not require --update. The candidate's ref_freshness_policy_for_install and dependency-scoped TieredRefResolver seeds make this distinction explicit. This is recommended documentation drift, not a demonstrated runtime regression or a missing-test claim.
*Suggested:* Replace the unconditional sentence in place: 'Plain installs replay matching lock entries; mutable Git refs without a matching entry resolve upstream when materialized. See commands.md for frozen and refresh behavior.' Keep the detailed command policy in commands.md rather than maintaining a second exhaustive list here.
*Proof (manual):* `tests/unit/deps/test_tiered_ref_resolver.py::test_install_lock_presence_cannot_authorize_unseeded_bare_refs` -- proves: The inspected candidate distinguishes matching locked commits from upstream resolution for dependencies without matching lock entries; this review did not execute the test. [devx, governed-by-policy]
`assert result.resolved_commit == (SHA_A if matching_lock else SHA_B) stale_l2.assert_not_called()`
#### Test Coverage Expert
- **[blocking]** P8 completion evidence remains pending, not missing lifecycle tests. at `tests/integration/test_required_lifecycle_state_machine.py:2180`
The reviewed a42 tree exactly matches the historical native report, whose digest, eight passing witnesses, command inventory and ordered runtime trajectories were verified. This is genuine source-Python execution, not static proof, and must retain its historical passed status. However, recovery-state.json explicitly records zero recovery native campaigns and pending corrected-provider integration. No final integrated driver report or independent parent --completion execution is available. P8 requires both at the final candidate; the historical report and fresh 186-test aggregate cannot certify a future merged tree. This finding concerns unavailable completion evidence, not an observed runtime failure or absent scenarios. Native execution was prohibited for this reviewer.
*Suggested:* Reuse the adjusted required and generated tests; do not add a duplicate module. After the authorized provider integration produces a clean committed candidate, the driver must execute scripts/check_lifecycle_evidence.py --base <actual-comparison-base> --head <final-candidate> --lane full --report <fresh-external-report>. The parent must independently execute the full gate with --completion <driver-return-json> and a separate fresh report. Preserve all eight witnesses, the three scope variants and the 26 model entrypoints through the planned framework handoff. Keep completion blocked until required execution and mutation/restoration evidence are current.
*Proof (unknown):* `tests/integration/test_required_lifecycle_state_machine.py::test_required_global_audit_rule_matrix_for_external_roots[True]` -- proves: Install, update, compile, repair and remove through home aliases must remain safe on the final integrated candidate. [devx, governed-by-policy, secure-by-default]
`assert_snapshot_set_unchanged(before, ArtifactSnapshotSet.capture(artifact_roots))`
- **[recommended]** Bind the five reported owner decisions to exact executed consumer evidence. at `tests/integration/test_generated_lifecycle_state_machine.py:687`
recovered-owner-report.json identifies five decisions at a42 versus 103. The supplied recovery JSON artifacts contain no functional evidence rows carrying owner_decisions and head_sha together with passing run_evidence. recovered-functional.log records 186 passed in 41.10s but contains neither an invocation nor node identifiers. Historical native witnesses exercise canonical materialization, user scope, provider refusal/recovery and sibling-seed freshness; the four-layout authorization test also exists and uses real files. These are useful tests, not absent coverage. Nevertheless, their exact decision-to-execution bindings are not established by the supplied owner report or aggregate log. This is advisory evidence traceability; shepherd-driver remains the enforcement owner.
*Suggested:* Attach executed consumer rows for each exact touched_owners[].decision from recovered-owner-report.json, retaining its reviewed head_sha and a verifiable invocation/report reference. Reuse existing lifecycle, authorization and frozen tests rather than introducing owner-only tests. Mark static architecture guards as static and mocked resolver tests as unit; neither replaces functional consumer evidence. Regenerate bindings at the final integrated head.
*Proof (unknown):* `tests/integration/test_generated_lifecycle_state_machine.py::test_generated_lifecycle_sequences_preserve_reference_model[global-aliased]` -- proves: Frozen installs refuse a changed provider without altering installed files and recover after the declaration is restored. [governed-by-policy, secure-by-default]
`assert_snapshot_set_unchanged(before, self._capture())`
#### Performance Expert
- **[recommended]** Reuse the ownership index across packages within one immutable lockfile snapshot. at `src/apm_cli/integration/skill_integrator.py:1106`
At skill_integrator.py:878-891, each ownership-map construction reads/parses the complete lockfile and visits every package and deployed path. Native skills call it at line 1106, bundles at 1313, and standalone sub-skills through lines 953 and 897. services.py:602 calls skill integration per package. For P such package integrations, B lockfile bytes, D locked packages, and F deployed-path records, this performs P lockfile reads and O(P*(B+D+F)) processing, ignoring path-string lengths. With one deployed record per package, increasing P and F together from N to 10*N produces 100 times the ownership-record visits. The traversal already existed at base 1030295; this PR correctly makes global calls read the real user lockfile rather than an unrelated deployment-root lockfile. This is an exposed existing scaling cost, not grounds to revert the ownership-root correction or a measured wall-time regression. It conflicts with the integrator rule to partition once and pay only for the current package.
*Suggested:* As a bounded follow-up, reuse the existing canonical ownership-map builder's result for a single install's immutable lockfile snapshot, with explicit replacement/invalidation when that snapshot changes. Keep project and user scopes separate; do not introduce a process-global path-only cache or a second ownership authority. This reduces ownership work to O(B+D+F+P). Add deterministic N/10*N integration guards counting LockFile.read calls and ownership-record visits, requiring at most 15x visit growth, plus update/reinstall and separate-root cases to catch stale indexes. Coordinate any shared identity changes with #2901 rather than taking over its implementation.
</details>
<sub>This panel is advisory. It does not block merge. Re-apply the
`panel-review` label after addressing feedback to re-run.</sub>
Addresses the panel lifecycle follow-up and Copilot plugin fixture comment. Run global install and audit through the installed CLI with aliased HOME/APM_HOME while preserving physical-root snapshots and user-owned sentinels; pass the written plugin manifest explicitly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Shepherd driver: converged for maintainer reviewThe final full panel recommends Reservations carried from strategic-alignment
Folded in this run
Copilot signals reviewed
Regression-trap evidence (mutation-break gate)
Canonical-owner evidenceExact base/head detection reports the legacy plugin membership/placeholder owner through the integrator selector. Classification: Lint contractThe complete current canonical mirror passed before push: Ruff check/format on runtime, tests and architecture scripts; YAML I/O, 2100-line and portable-path guards; pylint R0801; auth signals and architecture boundaries. CIFinal-head CI passed, including both Linux shards, Windows Compatibility, lifecycle, binary smoke, architecture ratchets and lint. Merge Gate also passed. Full rollup: 17 successful checks and one intentional docs-deploy skip; zero CI recovery iterations. Mergeability status
ConvergenceOne outer iteration; two Copilot rounds; full initial and final specialist panels. No deferrals. This is a landing-ready advisory, not an approval or merge. No issue closure or merge was performed. |
Exercise connected global canonical and aliased command traces and deterministic spines inside generated models. Preserve global skill ownership and canonical module paths across update, validate frozen identity, and refresh mutable refs when no lock exists. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…imits Exercise deps info, cache info/prune, project target inspection, and global absolute-file find refusal without inventing unsupported flags or weakening durable-state checks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Docs sync advisoryVerdict: no residual docs changes * Pages affected: 2 already updated * LLM calls: 3/15 At |
Bind frozen full-SHA validation to req-lk-003 and extend canonical/aliased trajectories with real lock exports and cache/source maintenance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extend the existing identity owner selectors and provenance consumer checks for canonical module paths, frozen drift validation, and user-scope lockfile roots, with focused bypass mutations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…e-alias-lifecycle
Regenerate the conformance statements after adding the req-lk-003 frozen full-SHA equality/refusal test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR triage recommendationready-for-review Recommendation only, not merge or scope approval. A responsible Linked issue#2867 (open). Live labels include Record on that issue: automated triage applied Proposed classification
Suggested next actionWait for a human CODEOWNERS review. Do not merge from this CODEOWNERS for Note: base branch is Suggested PR commentThank you for this pull request and for closing #2867.
This is advisory classification only. A responsible human
maintainer still has to review. Linked #2867 is labelled
status/accepted, so this PR is not auto-deferred.
CODEOWNERS owners are @danielmeppiel and @sergio-sisternes-epam.
A review is already requested of @sergio-sisternes-epam; this
comment does not change review requests or assignment. |
Sergio Sisternes (sergio-sisternes-epam)
added
triage/recommended
Preserve original HOME-alias repair history and current destination-scoped ownership, alias placement and lifecycle coverage. Adapt documented conflicts without merging obsolete prerequisite branches. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adapt matching-lock freshness and provider-scoped seeds from de82061b7f66fe149ef240575d16165ca3ab631d and e30e4e59ef90f679309334ba87c4173ecbcbe09b; transport-before-semver from 843386ea1073373f95d5d8bc588fb057cd8747ab; connected real-tag trajectory from 60a9a5ca85829d0f83989f5aa5fc5b04cb0ae815. Adapt integration-phase ownership snapshots from 40c53a8ae1cdd0ca8ae675e5ea7c7770ff123461 while retaining current-main destination collision authority. Reuse narrowly scoped epoch, alias and receipt tests with provenance in the approved standalone plan. Native evidence mechanics adapted from be73ed139f3a73c19437a895dfd6d68e41df8bfc without restoring the closed governance stack. Fix the alias reader defect exposed by retained global lifecycle witnesses through existing placement authority. Original published repair and authorship remain in merge ancestry. No historical qualification results are reused. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Preserve landed Claude discovery and selected-store authority from #2952, OpenCode enabled semantics from #3102, and package-remote marketplace tags from #3044. Union existing dependency-identity selectors with the standalone reader/frozen routes; regenerate combined conformance. No unmerged prerequisite branch is imported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise same-cardinality witness substitution, skipped calls behind successful pytest exits, realistic foreign launchers, and manifest-only alias escape. Admit only canonical Python exec trampolines while preserving exact environment and source checks, including foreign virtualenvs sharing a base interpreter. Remove unrelated prototype ratchet metadata; ordinary required unit coverage remains unchanged. Align inherited scoped-lock fixture with actual bare-ref serialization without relaxing frozen admission. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Require actual APM Popen argv and isolated cwd/environment rather than counting identity probes. Reject executable-prefix lookalikes alongside foreign virtualenvs. Correct the external-global ownership test to independently expect its explicit alias path, lock alias and absence of the non-alias cache while retaining deployed bytes and compile assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|
| python-architect | 0 | 2 | 0 | Ownership snapshots preserve the canonical builder and phase lifetime. Two recommended fixes remain: stale-provider seed recovery and validation of driver witness contents. |
| test-coverage-expert | 0 | 1 | 0 | Full-pin mismatch has unit coverage but lacks a native frozen refusal/recovery trap. Alias lifecycle coverage is present; exact-head native completion remains pending. |
| auth-expert | 0 | 1 | 0 | Scoped seeds preserve provider and transport boundaries; unclassifiable historical provider hints need a nonfatal seed-rejection path. Native execution remains pending. |
| supply-chain-security-expert | 0 | 0 | 0 | No introduced containment, ownership, or identity bypass found at 25299d vs d3c5cd9. Scoped tests: 344 passed, 2 subtests passed. Native9 and independent completion remain pending. |
| performance-expert | 0 | 0 | 0 | Original performance FOLD satisfied: 101 scoped tests pass; one build per used root, zero unused, 10x ownership visits at 10N. Native9 remains pending; no wall-time speedup claimed. |
| devx-ux-expert | 0 | 0 | 0 | No actionable UX regression found in the scoped 25299d diff. Frozen recovery, selected-store readers, and freshness guidance align. Review was static; disclosed native validation remains pending. |
| doc-writer | 0 | 2 | 0 | Scoped behavior claims align with the changed runtime. Clarify lifecycle-run prerequisites and consolidate duplicate install guidance; no blocking docs finding. |
| oss-growth-hacker | 0 | 0 | 0 | The original growth fold is addressed: contributor credit, concise recovery, distinct alias concepts, and bounded freshness claims. No new growth findings. |
| cli-logging-expert | 0 | 2 | 0 | Install help distinguishes frozen identity from freshness. Two native-provider failure paths obscure actionable diagnostics; review was static and no native suite was rerun. |
B = blocking-severity findings, R = recommended, N = nits. Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[test-coverage-expert] Extend the existing global external-root trajectory with real-CLI full-pin-only lock corruption, frozen refusal and recovery for both HOME variants. -- This missing secure-by-default/governed-by-policy regression ranks above opinion-only recommendations. Keep manifest.ref and lock.resolved_ref at real commit A, change only lock.resolved_commit to real commit B, assert exit 1 and the commit-mismatch diagnostic with unchanged durable snapshots, then restore the lock and prove frozen replay preserves pinned bytes. Retain all nine selectors and the initializer-backed 25-rule model; passing comparison units and today's native nine do not cover this transition.
-
[python-architect] Validate driver witness contents and required source/interpreter/launcher identity through the existing execution-validation owner at completion verification and emission. -- The manual probe accepted nine uncollected/skipped witnesses with no events/models or execution identity after digest binding. Reuse validate_execution and the current candidate contract, explicitly allowing appropriate checkout-relative differences; add digest-recomputed invalid-witness/profile regressions and extend the existing static guard. The fresh nine-pass run does not refute this independent verification defect.
-
[python-architect, auth-expert] Discard unclassifiable historical provider seeds without aborting resolution of a valid current manifest. -- These duplicate findings describe one recovery regression: obsolete GitLab hints on a now-GHES host can raise during seeding, including for lock-only entries, before valid declarations resolve. Return False without recording the unusable seed, keep current-manifest validation strict, and add matching-entry and unrelated stale-entry recovery regressions plus the existing freshness-boundary check.
-
[cli-logging-expert] Preserve the independent run's failure report and original diagnostic; compare postflight source identity only against a captured baseline. -- Two distinct error paths obscure the actionable cause: completion status comparison can prevent persistence of a failed fresh run's report, and early contract-validation failure can be overwritten by a false source-change message. Use the existing exclusive report writer before completion comparison on execution failure, print the original reason and report path, and preserve initial errors separately from genuine postflight errors. Add focused regressions for both paths without emitting completion success.
-
[doc-writer] Clarify lifecycle qualification prerequisites and consolidate duplicate install freshness/HOME-alias explanations. -- Define BASE_SHA as a full, locally present, distinct ancestor of HEAD; identify distinct new external report/sidecar paths; explicitly replace --completion-output with --completion for independent execution on the same base/head and keep the driver's report accessible at its recorded path. State replay/freshness and HOME-alias behavior once in install.md, retaining recovery links, frozen refusal details, the separate user lock root and containment qualifications.
Architecture
Class Diagram
Before
classDiagram
direction LR
class InstallPipeline {
<<Module>>
+run_install_pipeline()
}
class BaseIntegrator {
+check_collision()
}
class SkillIntegrator {
+integrate_package_skill()
+_build_ownership_maps(project_root)
}
class LockFile {
+read(path)
+get_package_dependencies()
}
class LockedDependency {
+get_unique_key()
+to_dependency_ref()
}
class DependencyReference {
+get_unique_key()
+get_install_path()
}
class TieredRefResolver {
<<ChainOfResponsibility>>
+seed()
+resolve()
}
class RefResolutionTier {
<<Protocol>>
+try_resolve()
}
class PerRunRefCache {
+get()
+put()
}
class RefFreshnessPolicy {
<<Enumeration>>
REPRODUCIBLE
CURRENT_REMOTE
}
InstallPipeline ..> SkillIntegrator : sequential integration
BaseIntegrator <|-- SkillIntegrator
SkillIntegrator ..> LockFile : rebuild per consumer
LockFile o-- LockedDependency
LockedDependency ..> DependencyReference : reconstructs
TieredRefResolver *-- RefResolutionTier
TieredRefResolver *-- PerRunRefCache : lock seeds and fresh results
TieredRefResolver ..> RefFreshnessPolicy
TieredRefResolver ..> DependencyReference
note for TieredRefResolver "Chain of Responsibility: L0 -> L1 -> L2 -> L3"
After
classDiagram
direction LR
class InstallPipeline {
<<Module>>
+run_install_pipeline()
+_run_integration_phase()
}
class BaseIntegrator {
+check_collision()
}
class SkillIntegrator {
<<ContextManager>>
+ownership_snapshot()
+_ownership_maps(lockfile_root)
+integrate_package_skill()
}
class SkillSupport {
<<Module>>
+build_skill_ownership_maps(lockfile_root)
}
class LockFile {
+read(path)
+get_package_dependencies()
}
class LockedDependency {
+get_unique_key()
+to_dependency_ref()
}
class DependencyReference {
+get_unique_key()
+get_install_path()
}
class TieredRefResolver {
<<ChainOfResponsibility>>
+seed()
+_lock_seed_key()
+resolve()
}
class RefResolutionTier {
<<Protocol>>
+try_resolve()
}
class PerRunRefCache {
+get()
+put()
}
class RefFreshnessPolicy {
<<Enumeration>>
REPRODUCIBLE
LOCKED_OR_CURRENT
CURRENT_REMOTE
}
class HostProviders {
<<Module>>
+classify_host_provider()
+effective_host_provider_identity()
}
class LifecycleEvidencePlugin {
<<Observer>>
+pytest_sessionstart()
+pytest_runtest_logreport()
}
class LifecycleContracts {
<<Module>>
+candidate_contract()
+validate_execution()
}
class CheckLifecycleEvidence {
<<Module>>
+execute()
+validate_completion()
}
InstallPipeline ..> SkillIntegrator : integration-only context
BaseIntegrator <|-- SkillIntegrator
SkillIntegrator ..> SkillSupport : canonical builder seam
SkillSupport ..> LockFile : once per used physical root
LockFile o-- LockedDependency
LockedDependency ..> DependencyReference : reconstructs
TieredRefResolver *-- RefResolutionTier
TieredRefResolver *-- PerRunRefCache : fresh repository results
TieredRefResolver ..> RefFreshnessPolicy
TieredRefResolver ..> DependencyReference : scoped lock-seed identity
TieredRefResolver ..> HostProviders
CheckLifecycleEvidence ..> LifecycleEvidencePlugin : fresh pytest observations
CheckLifecycleEvidence ..> LifecycleContracts : execution validation
note for SkillIntegrator "Context manager: lazy MappingProxyType maps; finally clears the epoch; live collision overlay remains separate"
note for TieredRefResolver "Chain of Responsibility: matching lock seed first; unseeded install refs use L0 -> L1 -> L3"
class InstallPipeline:::touched
class SkillIntegrator:::touched
class SkillSupport:::touched
class TieredRefResolver:::touched
class RefFreshnessPolicy:::touched
class HostProviders:::touched
class LifecycleEvidencePlugin:::touched
class LifecycleContracts:::touched
class CheckLifecycleEvidence:::touched
classDef touched fill:#fff3b0,stroke:#d47600
Component
Before
flowchart TD
A["apm install: install/pipeline.py run_install_pipeline"] --> B["install/phases/resolve.py run"]
B --> C["install/helpers/ref_seed.py seed_ref_resolver_from_lockfile"]
C --> D["deps/tiered_ref_resolver.py seed: PerRunRefCache.put repository/ref"]
D --> E["resolve: L0 repository/ref hit?"]
E -->|yes| G["[FS] install/phases/download.py run"]
E -->|no| F["[NET] _dispatch: commits API; [I/O] bare cache; [EXEC] legacy clone"]
F --> G
G --> H["[FS] _run_phase integrate: install/phases/integrate.py run"]
H --> I{"ctx.lockfile_only?"}
I -->|no| J["integration/skill_integrator.py integrate_package_skill"]
J --> K["[I/O] _build_ownership_maps(project_root): LockFile.read per consumer"]
K --> L["[FS] _integrate_native_skill / _integrate_skill_bundle / _promote_sub_skills_standalone"]
L --> M{"pipeline result_from_install_context: FAILED?"}
I -->|yes| M
M -->|yes| N["Return failed InstallResult: exit_code 1"]
M -->|no| O["[LOCK] install/phases/lockfile.py LockfileBuilder.build_and_save after cleanup"]
After
flowchart TD
A["apm install: install/pipeline.py run_install_pipeline"] --> B["install/phases/resolve.py run"]
B --> C["install/helpers/ref_seed.py seed_ref_resolver_from_lockfile"]
C --> D["deps/tiered_ref_resolver.py seed: _lock_seed_key"]
D --> P{"core/host_providers.py effective_host_provider_identity succeeds?"}
P -->|no| Q["ValueError escapes before _resolve_dependencies"]
P -->|yes| E{"resolve: matching dependency/provider/transport/ref lock seed?"}
E -->|yes| G["[FS] install/phases/download.py run"]
E -->|no| F["[NET] _dispatch: current upstream for unseeded install refs; fresh L0 reuse allowed"]
F --> G
G --> H["install/pipeline.py _run_integration_phase"]
H --> I{"ctx.lockfile_only?"}
I -->|yes| J["contextlib.nullcontext: integrate.run without skill lookup"]
I -->|no| K["SkillIntegrator.ownership_snapshot: empty lazy epoch"]
K --> L["[FS] integrate.run: integrate_package_skill; plugin boundary before root resolution"]
L --> R["[I/O] _ownership_maps: USER get_apm_dir or project_root; resolve physical root"]
R --> S{"Physical root already cached?"}
S -->|no| T["[I/O] skill_support.build_skill_ownership_maps: LockFile.read; freeze maps"]
S -->|yes| U["Reuse immutable maps; retain live managed set and session owners"]
T --> U
U --> V["[FS] _integrate_native_skill / _integrate_skill_bundle / _promote_sub_skills_standalone"]
V --> W["ownership_snapshot finally: clear maps on success or exception"]
L -->|uncaught exception| W
W --> X{"Integration raised?"}
X -->|yes| Y["Propagate exception without lockfile persistence"]
X -->|no| M{"pipeline result_from_install_context: FAILED?"}
J --> M
M -->|yes| N["Return failed InstallResult: exit_code 1"]
M -->|no| O["[LOCK] install/phases/lockfile.py LockfileBuilder.build_and_save after cleanup"]
AA["scripts/check_lifecycle_evidence.py main --lane full"] --> AB["[I/O] execute: candidate and source_profile"]
AB --> AC["[EXEC] pytest.main fixed nine nodes; LifecycleEvidencePlugin observes runner calls"]
AC --> AD["scripts/lifecycle_contracts.py validate_execution"]
AD --> AE["[I/O] execute finally: candidate and source_profile recheck"]
AE --> AF{"Passed and --completion supplied?"}
AF -->|yes| AG["[I/O] validate_completion: digest, headers, witness keys and CLI digest"]
AF -->|passed without completion| AH["[FS] _write_new report; optional completion_summary sidecar"]
AG --> AH
AF -->|failed| AI["main returns 1"]
AG -->|EvidenceError| AI
AH --> AJ["main returns 0"]
Recommendation
Fold the concrete in-scope repairs and coverage/doc improvements, import the parent-approved exact upstream matrix test commit, and qualify the resulting head before seeking a fresh ship recommendation. Highest-signal work is the real-CLI frozen full-pin regression and driver-witness completion validation. The nine fresh passes are meaningful evidence for 25299d/d3, not proof for a later head or completed independent parent acceptance. This is advisory input to Sergio's responsible-human review; it does not authorize a merge or substitute for human review.
Full per-persona findings
python-architect
- [recommended] Reject an unclassifiable lock seed without aborting valid manifest resolution at
src/apm_cli/deps/tiered_ref_resolver.py:466
The new _lock_seed_key calls effective_host_provider_identity on every seed, but seed does not handle that owner's ValueError/RuntimeError. A previously valid lock for code.example.com with host_type='gitlab' becomes unclassifiable when GITHUB_HOST identifies that host as GHES and the manifest removes the hint. detect_ref_change correctly returns True, but resolve.run calls seed_ref_resolver_from_lockfile before _resolve_dependencies, so the old lock aborts the command before the valid manifest can recover. The in-memory probe confirmed that old reconstruction and _repository_cache_identity succeed while the new seed key raises ValueError. Seeding also visits lock-only dependencies, so a stale unused entry can affect otherwise unrelated work. This is a bounded error-path regression, not evidence that ordinary matching installs fail.
Design patterns
-
Used in this PR: Context manager -- SkillIntegrator.ownership_snapshot bounds lazy immutable ownership maps to integration and clears them in finally.
-
Used in this PR: Chain of Responsibility -- TieredRefResolver retains the tier waterfall while separating dependency-scoped lock seeds from fresh repository results.
-
Pragmatic suggestion: none -- the existing owners are sufficient; a narrow seed-rejection path is simpler than another policy abstraction.
Suggested: Treat canonical provider-classification failure for a locked seed as seed rejection, returning False without recording it. Keep invalid current-manifest resolution fail-closed. Add matching and unrelated stale-lock recovery regressions, and extend the registered freshness boundary check to retain that distinction. -
[recommended] Validate driver witness records, not only their names and report digest at
scripts/check_lifecycle_evidence.py:204
validate_completion checks the driver's header, witness key set, profile kind and CLI digest, but never validates the witness values or the remaining source/interpreter/launcher profile. A read-only probe supplied a correctly digest-bound driver with all nine witnesses collected=False, setup='skipped', events=[], models=0 and no source/interpreter/launcher identity; validate_completion accepted it. The current _completion test fixture similarly treats empty witness objects as an acceptable driver. This does not bypass execute() or the parent's fresh run. It means successful independent verification cannot also substantiate the claimed driver execution, which is a separate obligation in the brief. A hash establishes which bytes were supplied, not whether those bytes contain valid execution evidence.
Suggested: Route loaded driver records through the existing validate_execution owner using the current candidate contract, and validate required source-profile fields with checkout-relative differences explicitly allowed. Reuse that validation at completion emission. Add a digest-recomputed negative case for skipped/empty driver witnesses and extend the native-evidence static guard to require the owner call; do not duplicate trajectory logic.
test-coverage-expert
- [recommended] Exercise the new full-pin mismatch rejection through a real frozen install at
tests/integration/test_required_lifecycle_state_machine.py:
Scenario Evidence row 7 claims that frozen mode rejects mismatched full commit pins. The new branch in install/plan.py compares a manifest full pin against resolved_commit independently of resolved_ref. Its eight-case unit test passes in this review, and the conformance test also contains the mismatch assertion, but neither invokes the CLI. I searched tests/integration for full-pin, manifest-commit, mismatch and frozen/provider cases and read the matching tests. The required/global generated trajectories reject a changed declaration or provider; they do not keep manifest.ref == lock.resolved_ref == A while changing only lock.resolved_commit to B. test_hermetic_lifecycle_foundation.py tests that corruption through audit, not install --frozen. Thus existing native trajectories would not detect a command-path regression that bypasses this particular preflight rejection. This is a tier-floor gap, not evidence of a failing implementation or a claim that the pending native nine are absent.
Suggested: Extend test_required_global_audit_rule_matrix_for_external_roots with a short full-pin subtrajectory for both existing home variants: install a real full commit A; change only the locked resolved_commit to another real commit B while retaining resolved_ref=A and manifest ref=A; snapshot the durable roots; run install --global --frozen and require exit 1 plus the manifest-commit/locked-commit diagnostic and unchanged snapshots; restore the matching lock and require frozen replay with the original deployed bytes. Extend the existing trajectory instead of adding another module or witness selector, preserving all nine selectors and the 25-rule initializer-backed model.
auth-expert
- [recommended] Reject unclassifiable lock seeds without aborting ordinary resolution at
src/apm_cli/deps/tiered_ref_resolver.py:464
The new seed identity calculation can raise ValueError for historical provider metadata that drift detection explicitly treats as requiring re-resolution. For example, a locked dependency with host='code.example.com' and host_type='gitlab' becomes unclassifiable when GITHUB_HOST is subsequently set to code.example.com. classify_host_provider rejects that hint, and seed() propagates the exception. seed_ref_resolver_from_lockfile seeds every locked entry without handling this failure, including entries no longer declared. Thus an obsolete seed can prevent ordinary installation instead of being discarded so the valid current declaration resolves upstream. This is a static call-path finding; no tests were executed.
Suggested: Handle unclassifiable historical provider identity during seed admission by returning False, consistently with rejected seed inputs and detect_ref_change's treatment of unclassifiable locks. Keep current manifest validation strict. Add a regression using a formerly GitLab-hinted lock entry and a currently GHES-classified host, asserting that the seed is rejected and resolution of the valid current declaration can proceed.
supply-chain-security-expert
No findings.
performance-expert
No findings.
devx-ux-expert
No findings.
doc-writer
-
[recommended] Specify the lifecycle driver's base revision and completion-mode prerequisites. at
docs/src/content/docs/contributing/integration-testing.md:79
The new example uses BASE_SHA without defining its required form or relationship to HEAD. scripts/check_lifecycle_evidence.py:94-111 requires a full commit SHA, rejects an abbreviated SHA or branch name, and requires a distinct ancestor of the checked-out head. The independent-run paragraph also does not explicitly say to replace --completion-output with --completion, although argparse makes them mutually exclusive. These omitted prerequisites can stop a contributor before the documented qualification runs.
Suggested: Define BASE_SHA as the full SHA of the intended comparison base, which must be a distinct ancestor present locally; identify REPORT_PATH and COMPLETION_PATH as distinct new external paths. In the independent-run paragraph, explicitly replace --completion-output with --completion, retain the same base/head, and keep the driver's report available at the path recorded by its sidecar. -
[recommended] Consolidate the newly duplicated install behavior explanations. at
docs/src/content/docs/reference/cli/install.md:125
The new 'Lock replay and freshness' bullet at lines 125-131 repeats the existing, also-expanded 'Lockfile replay and Git ref freshness' bullet at line 180. 'Home aliases' at lines 132-135 repeats the new 'User-scope skill paths' bullet at line 185. This creates multiple maintenance locations for the same promises within one page, contrary to state-once documentation composition. The five scoped documentation files grow by 641 whitespace-delimited words, including 194 in install.md; consolidating these bullets offsets growth without removing user guidance.
Suggested: Keep one replay/freshness bullet containing matching-lock versus unseeded-ref behavior and the existing update/lockfile links; put structural refusal details in the existing Frozen mode bullet. Merge the two home-alias bullets, preserving the separate user lock root and package/destination containment qualifications.
oss-growth-hacker
No findings.
cli-logging-expert
-
[recommended] Preserve the independent run's failure report before comparing completion claims at
scripts/check_lifecycle_evidence.py:334
In --completion mode, a fresh failed execution reaches validate_completion before its report is written. A successful driver sidecar then disagrees on status, replacing the actual execution error with 'Completion disagrees with fresh execution: status'. The command exits without saving the requested independent report, losing its structured witness diagnostics precisely when the independent run fails.
Suggested: Handle a failed fresh execution before completion comparison: persist its failed report through the existing exclusive-write path, print the report location and original error, and return nonzero without emitting completion success. Add a --completion regression case asserting that the fresh failure reason and report survive. -
[recommended] Do not replace contract-validation errors with a false source-change diagnostic at
scripts/check_lifecycle_evidence.py:291
If candidate_contract rejects the ledger before line 241, report['profile'] still contains only the initial kind field. The finally block compares a complete source profile against that placeholder and overwrites the original actionable error with 'Source executable identity changed during execution', even when no source changed and execution never started.
Suggested: Track whether an initial source profile was successfully captured and compare profiles only when that baseline exists. Preserve the original failure, recording any genuine postflight failure separately. Add a case where candidate_contract raises an EvidenceError and assert that its diagnostic remains in the failed report.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 0aad2c1)
Validate driver witness contents and execution identity, preserve failure reports and first causes, reject unclassifiable historical provider seeds, and exercise real frozen full-pin refusal/recovery in both global trajectories. Preserve initialized integrator contracts in pipeline unit fixtures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Compose 6db0c11 (#3103). Preserve every existing provider/seed identity guard and add the incoming MCP AuthResolver/provenance guards as a faithful union. Incoming production auth files are unchanged from main; the already-imported marketplace mutation case remains single. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deep isolated paths can make Rich wrap the existing schema warning between words. Preserve the exact warning phrase, successful install, and deployed-byte assertions while comparing whitespace-normalized output. The unchanged b6 test fails on real wrapped output; the corrected ownership suite passes in the same deep environment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|
| python-architect | 0 | 0 | 0 | Delta folds close the prior architecture defects; canonical validators and incoming guard union are preserved. No remaining substantive finding. Independent acceptance and hosted CI remain pending. |
| test-coverage-expert | 0 | 1 | 0 | Both full-pin lifecycle trajectories are strengthened; stale-provider seed recovery still lacks fixture-backed install coverage. |
| auth-expert | 0 | 0 | 0 | Historical provider refusals are discarded without cache or receipt admission; unexpected TypeError propagates. Current classification remains strict, and auth guards compose without regression. |
| supply-chain-security-expert | 0 | 0 | 0 | No concrete in-scope regression found in historical seed rejection, strict current identity, full-pin refusal, or fresh-report binding. Read-only assessment; independent acceptance and CI remain pending. |
| performance-expert | 0 | 0 | 0 | Static delta: seed refusal (467-474) adds no I/O or loops; shared fresh-answer caching/coalescing (526-566) remains unchanged. No broader per-dependency work. Tests not run. |
| cli-logging-expert | 0 | 0 | 0 | Scoped delta preserves failed reports and original errors, records postflight failures separately, and keeps seed debug and whitespace assertions truthful. Parent acceptance and CI remain pending. |
| doc-writer | 0 | 0 | 0 | No new substantive docs defects in 25299d..541e583. Lifecycle instructions match the verifier; install consolidation preserves root, containment, freshness and recovery boundaries. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 2 follow-ups
-
[test-coverage-expert] Extend the existing ref_freshness real-CLI fixture with matching and unrelated unclassifiable historical provider hints. -- The narrower inspection confirmed the original integration-floor gap. Reuse test_reinstall_uses_current_ref_without_a_matching_lock_entry to verify current installed and deployed bytes, rewritten lock identity, preserved sentinels and stable replay. Keep current-declaration rejection strict; this adds a missing portability safeguard rather than repairing a proven runtime failure.
-
[orchestrator] Profile Lifecycle Smoke (Linux) and reduce unnecessary execution cost within the unchanged six-minute job budget. -- Both 541 CI runs exceeded the job cap despite passing test assertions, and the dependent check failed. Preserve all witnesses, contracts and required selection; do not raise the timeout or rely on unchanged reruns. Obtain successful CI and fresh successor-bound acceptance evidence after any source fold.
Recommendation
Use the existing fold-by-default authorization for the bounded recovery fixture and contract-preserving CI cost reduction. Independent acceptance has now passed for 541; obtain fresh successor proof for any source changes before presenting the updated evidence to Sergio and the maintainer. This is advisory guidance, not a waiver or authorization to merge, change reviewers or alter PR state.
Full per-persona findings
python-architect
No findings.
test-coverage-expert
- [recommended] Exercise stale-provider seed recovery through a real install at
tests/unit/install/phases/test_resolve_seed_from_lockfile.py:82
The new matching and unrelated historical-seed cases exercise real classification and resolver admission, but construct the lockfile in memory and mock the downloader. They defend the function-level fix, not the install promise that an obsolete provider hint cannot prevent a valid current dependency from being installed. Targeted searches of the owned integration tests for historical, unclassifiable, seed, host_type, GITHUB_HOST and provider found no corresponding recovery transition. The generated frozen_refusal rule instead changes the current manifest's provider, expects refusal, and restores the manifest; it does not exercise rejecting stale lock metadata while continuing a non-frozen install. This is a tier-floor follow-up, not evidence of a correctness regression or a reason to discount the recorded passing unit controls.
Suggested: Extend test_required_global_audit_rule_matrix_for_external_roots rather than adding another expensive lifecycle module or changing the nine selectors. Reuse its local remote and sentinels: install A, publish B, arrange an obsolete historical provider hint with a valid current declaration, run ordinary install, verify B's installed and deployed bytes plus the rewritten lock, then replay and verify unchanged state. Cover matching and unrelated historical entries within the existing fixture. Keep current-declaration rejection strict.
Proof (missing at integration-with-fixtures): tests/integration/test_required_lifecycle_state_machine.py::test_required_global_audit_rule_matrix_for_external_roots -- A stale provider hint in my lockfile does not prevent installing my valid current dependency or preserve obsolete package bytes.
Proposed assertion: assert installed_skill.read_bytes() == revision_b_bytes assert locked_commit == commit_b.sha assert sentinel.read_bytes() == original_sentinel_bytes
auth-expert
No findings.
supply-chain-security-expert
No findings.
performance-expert
No findings.
cli-logging-expert
No findings.
doc-writer
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
Exercise matching and unrelated stale provider hints through ordinary CLI installs. Use four bounded smoke workers without changing selection, grouping or the six-minute cap. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| python-architect | 0 | 0 | 0 | No substantive architecture defect in the four-file delta. Existing fixture authority and isolation are preserved; four-worker loadgroup retains selection. Independent full9 remains pending. |
| test-coverage-expert | 0 | 0 | 0 | Adjusted ApmLifecycle cases close both historical-provider recovery gaps in recorded real-CLI evidence; exact-four-worker topology has mutation-backed negative protection. No new findings. |
| performance-expert | 0 | 0 | 0 | Measured 2e hosted smoke completes in 210s under the unchanged 360s cap; four workers retain loadgroup and selection. No substantive performance finding. |
| doc-writer | 0 | 0 | 0 | No substantive docs drift in this delta. Four bounded workers, public Ubuntu, loadgroup grouping and the unchanged six-minute limit match the workflow and worker guard. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Recommendation
Recommend shipping this exact frozen revision through the repository's normal human review and merge-queue process: the two previously demonstrated deficiencies now have affirmative exact-head evidence, and the delta panel identifies no further technical changes. Complete the pending parent process/postflight receipt handoff without inventing its identifier or conflating it with the already-passing report. This stance follows the evidence, not exhausted iteration caps; it is neither human approval nor permission to merge or enter the queue.
Full per-persona findings
python-architect
No findings.
test-coverage-expert
No findings.
performance-expert
No findings.
doc-writer
No findings.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.
fix(install): complete HOME-alias lifecycle repair standalone against main
TL;DR
Global skills deploy through
HOME/APM_HOMEdirectory aliases without authorizing linked descendants. This standalone repair preserves the original history and fixes the directly coupled ownership, matching-lock freshness, frozen identity and alias-reader defects exposed by connected lifecycle execution. Fresh exact-head proof covers the retained nine-witness contract; source-Python proof remains distinct from hosted platform checks and independent acceptance.Closes #2867. Thanks to Dave Mead (@DaveMeadAdjust) for the reproduction and proposed fix.
Important
Candidate
2e819da58835436be1c936fbfe6e5c38856f7baf, treec71d8a9cfac6126c40698bf6a2fbdc5aab846b43, targetsmain@6db0c11dfe4aa603889e7216babf0c0fd2b7e036. Driver and independent parent acceptance pass; all 22 hosted checks conclude successfully or with expected neutral/skipped status. Final delta recommendation: ship_now, no technical follow-ups. GitHub reports MERGEABLE / BLOCKED: required code-owner and last-push approval plus merge-queue handling remain human-controlled. CLA passes; no merge or auto-merge occurred.Problem (WHY)
Qualification follows the Agent Skills guidance "Read agent execution traces, not just final outputs.". This anchors the validation method; the failure descriptions above come from observed regressions, not hypothetical policy claims.
Approach (WHAT)
loadgroup, print actual runner CPUs, and reject scheduling drift.Implementation (HOW)
src/apm_cli/integration/skill_integrator.py,integration/skill_support.py,install/pipeline.pysrc/apm_cli/deps/tiered_ref_resolver.py,install/helpers/ref_seed.py,core/host_providers.pysrc/apm_cli/drift.py,install/plan.py,commands/install.pysrc/apm_cli/commands/deps/cli.pyget_install_path()authority; refuse escape and stay in the selected store. No parallel discovery map.scripts/check_lifecycle_evidence.py,scripts/lifecycle_contracts.py,tests/utils/lifecycle_evidence.pytests/fixtures/lifecycle_bug_ledger.json,lifecycle_completion.schema.jsontests/spec_conformance/test_lockfile_reqs.py,CONFORMANCE.json,CONFORMANCE.mdreq-lk-003; regenerate inventory without a new normative requirement..github/workflows/ci.yml,tests/quality/test_ci_topology.pyloadgroupand budget. Strict positive/negative topology coverage, including physical guard removal. No new jobs, permissions or selector changes.Standalone scope, provenance and review folds
Direct human authorization made this same PR standalone against main, replacing only necessary obsolete prerequisites. Original published
171a3ecbeand selected main6db0c11are ancestors; the protected original local branch remains at70e52a03.0aad2c16supplied only the marketplace MutationCase, preserved through provenance-bearing260d5dce; the exhaustive equality assertion remains.541e5832exceeded the six-minute lifecycle budget. Clean same-selection profiling showed 231 passes/one skip at both two and four workers, with identical case identities; local times were 374.57s versus 218.09s. Those measurements remain 541-only, not successor hosted proof.GitHub required removing stack grouping 2883 before retargeting. Explicitly authorized metadata-only removal preserved PRs, refs, content and reviewers; #2882 remains closed/unmerged. Only #2876 was retargeted and normally fast-forwarded. No force-push, replacement PR, reviewer request or merge occurred.
Historical successful reports remain bound to their actual heads. Final driver and parent qualification each require their own clean checkout and interpreter.
Diagrams
Global trajectories advance installed bytes, commit identity and ownership together; generated global variants additionally require fresh C despite a sibling-only seed.
stateDiagram-v2 direction LR [*] --> InstalledA: install A InstalledA --> UpstreamB: publish B UpstreamB --> InstalledB: update / compile B note right of InstalledB NEW assertions: bytes, commit and ownership advance together Replay and refusal leave the installed revision unchanged end note InstalledB --> DamagedB: tamper or remove sources DamagedB --> InstalledB: install repairs / audit passes InstalledB --> Removed: uninstall / audit empty Removed --> Reinstalled: restore declaration / install note right of Reinstalled Required globals retain B Global models publish C with only a sibling lock seed NEW assertion: unseeded dependency installs C end note Reinstalled --> Removed: audit / uninstall / audit empty Removed --> [*]Trade-offs
apm install --update.Benefits
Validation
Exact 2e819d proof, commands and qualification boundaries
Full functional/owner-matrix/quality/composed integration invocation, including
tests/integration/test_ref_freshness_lifecycle.py(python -m pytest -p no:cacheprovider -o addopts= -q, exact argv/environment retained in the driver artifact):python scripts/check_lifecycle_evidence.py --base 6db0c11dfe4aa603889e7216babf0c0fd2b7e036 --head 2e819da58835436be1c936fbfe6e5c38856f7baf --lane full --report <external-driver.json> --completion-output <external-completion.json>:Driver report SHA256:
14a3929876a6d7ca4b2cc9d42e7319408635275fe616b4123df1beddb4a7b837; sidecar SHA256:53604cdff9ff0489999867bee9e7cf0c4a6a9b2633c8e0f3a7017040d8b3a088. Canonical schema/execution validation and clean postflight identity pass.75 physical mutation recipes killed and restored: 44 component, 30 provider and one fixed-worker topology guard. Both new real-CLI historical variants fail at ordinary reinstall when the historical-admission guard is removed. Full-pin removal loses the early diagnostic while a downstream immutable-conflict guard still refuses; this is not a claim that the mutant succeeds.
Fresh detection finds seven canonical owners; nine actual consumer nodes cover all seven. Version-2 schema and strict semantic preflight pass. Complete owner matrix equality, all newly registered guards, source safety/2100-line guards, Ruff/format, pylint R0801, auth/architecture boundaries and assertion/duplicate ratchets pass.
Conformance: 274 passed with two existing waivers, neither a P8 skip.
npm --prefix docs run check:links:Independent parent fresh
--completion, in its own checkout/venv/disposable roots:Parent report SHA256:
057e631e4fbccc4c74400e5f31b3cb45fe6ea7e12397dabd0d0009a8244d38f9. Actual process exit 0 and parent postflight revalidate all nine witnesses/734 events, every setup/call/teardown, clean exact source and unchanged profile. The driver separately revalidated the completion and distinct source/venv bindings.CI run 36740776748 and required gate 36740776727 succeed. Lifecycle Smoke reports actual four CPUs,
231 passed, 1 skipped in 184.01s; whole-job duration is 210s within the unchanged 360s budget. Linux shards/binary smoke, Windows checks, architecture ratchets, spec, docs, CodeQL and CLA succeed. Native nine have no skips.Four final GPT-6 Astra delta specialists report no findings; the final advisory recommends
ship_now. Cumulative counters remain outer 4, Copilot 2/2 and CI recovery 2. Earlier 541 acceptance and 541 scheduling measurements are not rebound to this head. Human review/last-push approval and queue handling remain required.The unchanged lifecycle diagram was freshly validated with
mmdc 11.15.0; GitHub visual rendering is not independently verified.Scenario Evidence
tests/integration/test_required_lifecycle_state_machine.py::test_required_global_audit_rule_matrix_for_external_roots(both variants; regression-trap for #2867)tests/integration/test_generated_lifecycle_state_machine.pymandatory replay and Hypothesis sequences, all three variantstests/integration/test_ownership_invariant_lifecycle.py::test_global_update_preserves_owned_external_skill_targetstests/integration/test_ref_freshness_lifecycle.pyincluding[historical-matching]and[historical-unrelated]tests/integration/test_required_lifecycle_state_machine.pyseparate real-tag witness and both required globalstests/unit/install/test_security_scan_scope.py,tests/unit/commands/test_deps_alias_readers.pytests/unit/integration/test_skill_ownership_snapshot.py,tests/unit/install/test_pipeline_ownership_snapshot.pytests/unit/deps/test_github_downloader_phase3.py,tests/unit/cache/test_git_cache.pyreceipt casestests/unit/scripts/test_lifecycle_evidence.py,tests/integration/test_architecture_native_lifecycle_evidence.pyHow to test
apm_clisource.--completion /absolute/driver/completion.json --report /absolute/new/parent.json; require fresh execution, not reuse of the driver result.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com