From 765a412eacd5a164cf9670c8a3e3c8aeb1b4653b Mon Sep 17 00:00:00 2001 From: Kaustubh Arora Date: Sun, 25 Oct 2026 00:48:58 +0530 Subject: [PATCH] fix(policy): judge the declared constraint, accept package release tags require_pinned_constraint judged the wrong string in two ways. The resolve phase rewrites a direct dependency's in-memory reference to the tag a semver range selected, and the install gate runs after that, so a bounded range such as `pkg-a#^1.0.0` was classified as whatever tag it resolved to. The resolve phase now records the manifest-declared refs of the direct dependencies before any rewrite, and the gate and the post-targets re-check classify those. The resolved reference is left in place for download, drift and lockfile work; preflight and audit already hold the manifest view and are unchanged. The literal-tag classifier only knew the `v{version}` layout, while the resolver selects from `v{version}`, `{name}--v{version}` and `{name}-v{version}` and the dependency guide recommends pinning the package-qualified forms. The classifier now accepts those layouts for the dependency's own package name, using the resolver's tag patterns and regex builder, so `pkg-a-v1.1.0` on a `pkg-a` dependency is pinned while `pkg-b-v1.1.0`, `pkg-a-v1.1` or `release-pkg-a-v1.1.0` stay bare branches. Missing refs, moving branches, wildcards and open-upper ranges are classified as before, and a selected tag or SHA never launders an unbounded declaration. Refs #3224. --- .../governance-on-the-consumer-ramp.md | 8 +- .../docs/consumer/manage-dependencies.md | 5 +- .../docs/enterprise/policy-reference.md | 8 +- .../content/docs/reference/policy-schema.md | 6 +- src/apm_cli/install/context.py | 5 + src/apm_cli/install/phases/policy_gate.py | 1 + .../install/phases/policy_target_check.py | 1 + src/apm_cli/install/phases/resolve.py | 8 + src/apm_cli/policy/_constraint_pinning.py | 88 ++++- src/apm_cli/policy/_help_text.py | 4 +- src/apm_cli/policy/policy_checks.py | 30 +- ..._require_pinned_constraint_declared_e2e.py | 321 ++++++++++++++++++ .../test_require_pinned_constraint_e2e.py | 58 ++++ .../policy/test_pinned_constraint_declared.py | 189 +++++++++++ 14 files changed, 712 insertions(+), 20 deletions(-) create mode 100644 tests/integration/policy/test_require_pinned_constraint_declared_e2e.py create mode 100644 tests/unit/policy/test_pinned_constraint_declared.py diff --git a/docs/src/content/docs/consumer/governance-on-the-consumer-ramp.md b/docs/src/content/docs/consumer/governance-on-the-consumer-ramp.md index 123858595e..03f73b8080 100644 --- a/docs/src/content/docs/consumer/governance-on-the-consumer-ramp.md +++ b/docs/src/content/docs/consumer/governance-on-the-consumer-ramp.md @@ -36,9 +36,11 @@ your platform team can set, and what each one does to your install: - **`dependencies.require`** -- packages your `apm.yml` must include. - **`dependencies.max_depth`** -- maximum transitive dependency depth. - **`dependencies.require_pinned_constraint`** -- when `true`, every - APM dep declared in your `apm.yml` must use a bounded constraint - (semver range, literal tag, or 40-char SHA); bare branch names, - wildcards, and open-upper ranges (`>=1.0.0`) are rejected. + direct APM dep declared in your `apm.yml` must use a bounded constraint + (semver range, literal tag such as `v1.2.3` or `pkg-a-v1.2.3`, or + 40-char SHA); bare branch names, wildcards, and open-upper ranges + (`>=1.0.0`) are rejected. The constraint is judged as you declared it, + not as the tag it resolved to. - **`mcp.allow`** / **`mcp.deny`** -- glob patterns over MCP server references. Same semantics as the dependency lists. - **`mcp.transport.allow`** -- restricts MCP transports diff --git a/docs/src/content/docs/consumer/manage-dependencies.md b/docs/src/content/docs/consumer/manage-dependencies.md index 8c0e8b76b9..a62d29c9d6 100644 --- a/docs/src/content/docs/consumer/manage-dependencies.md +++ b/docs/src/content/docs/consumer/manage-dependencies.md @@ -454,7 +454,10 @@ lockfile alongside the resolved tag, so `apm install` on a fresh clone replays the same tag deterministically. A malformed range-like ref is rejected; use a plain range such as `^1.2.0` or pin a literal tag such as `pkg-a-v1.2.0`. Only `apm update` (or legacy `apm install --update`) or a -manifest change re-resolves to a newer tag. +manifest change re-resolves to a newer tag. Under an org policy with +`require_pinned_constraint: true`, both forms count as bounded: the range +is judged as the range you wrote, and `pkg-a-v1.2.0` is recognised as +this package's own release tag. Marketplace object-form ranges use the marketplace package name in the tag pattern: diff --git a/docs/src/content/docs/enterprise/policy-reference.md b/docs/src/content/docs/enterprise/policy-reference.md index 423fe47e1b..596a842be5 100644 --- a/docs/src/content/docs/enterprise/policy-reference.md +++ b/docs/src/content/docs/enterprise/policy-reference.md @@ -186,14 +186,16 @@ dependencies: ### `require_pinned_constraint` -Default: `false`. When `true`, every APM dependency declared in `apm.yml` must use a bounded constraint -- a semver range with an upper bound, a literal version tag (e.g. `v1.5.3`), or a 40-char commit SHA. Empty refs, bare branch names, wildcards (`*`, `1.x`), and open-upper ranges (`>=1.0.0`, `>1.0.0`) all fail the `dependency-pinned-constraint` check. +Default: `false`. When `true`, every direct APM dependency declared in `apm.yml` must use a bounded constraint -- a semver range with an upper bound, a literal version tag (`v1.5.3`, or `pkg-a-v1.5.3` / `pkg-a--v1.5.3` for a package named `pkg-a`, the layouts APM resolves ranges against), or a 40-char commit SHA. Empty refs, bare branch names, wildcards (`*`, `1.x`), and open-upper ranges (`>=1.0.0`, `>1.0.0`) all fail the `dependency-pinned-constraint` check. + +The check judges the constraint as declared. A range such as `pkg-a#^1.0.0` passes as a bounded choice even though the install selects a concrete release tag for it, and a bare branch stays a violation however it resolves. Fresh installs, locked reinstalls, updates, and `apm audit` agree. ```yaml dependencies: require_pinned_constraint: true ``` -Transitive deps are also classified; they pass when their parent manifests pinned them. See the [policy schema reference](../../reference/policy-schema/#require_pinned_constraint-reference) for the full classification table and diagnostic format. +Only direct dependencies are evaluated; a transitive package's own manifest is not something the consumer can rewrite. See the [policy schema reference](../../reference/policy-schema/#require_pinned_constraint-reference) for the full classification table and diagnostic format. --- @@ -430,7 +432,7 @@ There are 21 policy checks. | `required-executable-untrusted` | Required executable packages are present and trusted | | `required-package-version` | Required packages with version pins match per `require_resolution` | | `transitive-depth` | No dependency exceeds `max_depth` | -| `dependency-pinned-constraint` | Every dep uses a bounded constraint (semver range, literal tag, or SHA) when `require_pinned_constraint: true` | +| `dependency-pinned-constraint` | Every direct dep declares a bounded constraint (semver range, literal tag, or SHA) when `require_pinned_constraint: true` | | `registry-source` | Dependencies come from required registries when `registry_source` is configured | **Integrity:** diff --git a/docs/src/content/docs/reference/policy-schema.md b/docs/src/content/docs/reference/policy-schema.md index f125636bcd..d461723fe5 100644 --- a/docs/src/content/docs/reference/policy-schema.md +++ b/docs/src/content/docs/reference/policy-schema.md @@ -108,7 +108,7 @@ When a dependency matches `deny`, `apm install` will **not download or deploy th | `require` | list of refs or null | `null` | `null` = no opinion (transparent during merge). `[]` = explicitly empty. Packages every consumer manifest must include. | | `require_resolution` | enum | `project-wins` | `project-wins` / `policy-wins` / `block` -- how to resolve version conflicts on required packages. | | `max_depth` | integer | `50` | Maximum transitive dependency depth. Must be `> 0`. | -| `require_pinned_constraint` | boolean | `false` | When `true`, every APM dep declared in `apm.yml` must use a bounded constraint (exact, `^`/`~`/bounded range, literal tag, or SHA). Transitive deps are also classified and pass when their parent manifests pinned them. Unbounded refs (missing ref, `*`, bare branch, bare `>=X.Y`) are routed through `policy.enforcement` (`warn` / `block`). **Enabling on existing projects will likely surface violations; roll out with `enforcement: warn` first.** | +| `require_pinned_constraint` | boolean | `false` | When `true`, every direct APM dep declared in `apm.yml` must use a bounded constraint (exact, `^`/`~`/bounded range, literal tag such as `v1.2.3` or `pkg-a-v1.2.3` for a package named `pkg-a`, or SHA). The declared constraint is judged, not the tag a range resolved to; transitive deps are not evaluated. Unbounded refs (missing ref, `*`, bare branch, bare `>=X.Y`) are routed through `policy.enforcement` (`warn` / `block`). **Enabling on existing projects will likely surface violations; roll out with `enforcement: warn` first.** | ### `require_pinned_constraint` reference @@ -128,6 +128,8 @@ dependencies: - sixth_eq/lib#=1.5.3 # OK: exact version (npm/cargo explicit equality) - sixth_pip/lib#==1.5.3 # FAIL: pip-style operator not supported (BARE_BRANCH) - seventh/lib#v1.5.3 # OK: literal tag + - acme/mono/packages/pkg-a#pkg-a-v1.5.3 # OK: this package's own release tag + - acme/mono/packages/pkg-a#pkg-b-v1.5.3 # FAIL: another package's tag (BARE_BRANCH) - eighth/lib#aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa # OK: SHA - ./packages/local # OK: local-path dep (no version surface) ``` @@ -144,6 +146,8 @@ Diagnostic shape (ASCII-only, ``[x]`` for block, ``[!]`` for warn): - acme/lib: bare branch 'main' tracks a moving tip ``` +A range is classified as the author wrote it: `fourth/lib#^1.2.0` stays `OK` after the install selects `v1.4.0` for it, and `acme/lib#main` stays `FAIL` whatever commit it resolves to. The package name used for `{name}-v{version}` and `{name}--v{version}` tags is the final path segment of a subdirectory dependency, otherwise the repository name. + Patterns are matched against `/` (or `//`). Wildcards via shell-style globs, e.g. `contoso/*`. ## mcp diff --git a/src/apm_cli/install/context.py b/src/apm_cli/install/context.py index 001a9362a6..5b27cb265c 100644 --- a/src/apm_cli/install/context.py +++ b/src/apm_cli/install/context.py @@ -133,6 +133,11 @@ class InstallContext: # consumed by install/sources.py to plumb the resolution into the # lockfile via InstalledPackage.git_semver_resolution. git_semver_resolutions: dict[str, Any] = field(default_factory=dict) + # Manifest-declared refs of the direct deps, keyed by unique key. The + # resolve phase fills this before semver resolution rewrites a + # ``dep_ref.reference`` to the selected tag, so the policy gate can judge + # the authored constraint while everything else keeps the resolved ref. + declared_direct_refs: dict[str, str] = field(default_factory=dict) # Run-scoped cache of RefResolver instances keyed by (host, token-fingerprint) # so semver deps sharing an upstream repo reuse one ``git ls-remote`` tag # listing (RefResolver memoizes per instance) instead of one per dep. diff --git a/src/apm_cli/install/phases/policy_gate.py b/src/apm_cli/install/phases/policy_gate.py index 7983223360..8c61df5d4b 100644 --- a/src/apm_cli/install/phases/policy_gate.py +++ b/src/apm_cli/install/phases/policy_gate.py @@ -118,6 +118,7 @@ def run(ctx: InstallContext) -> None: fetch_outcome=fetch_result.outcome, fail_fast=(enforcement == "block"), direct_dep_keys={d.get_unique_key() for d in getattr(ctx, "all_apm_deps", []) or []}, + declared_refs=getattr(ctx, "declared_direct_refs", None) or None, **extra_kwargs, ) diff --git a/src/apm_cli/install/phases/policy_target_check.py b/src/apm_cli/install/phases/policy_target_check.py index 93383cd597..17de95d33c 100644 --- a/src/apm_cli/install/phases/policy_target_check.py +++ b/src/apm_cli/install/phases/policy_target_check.py @@ -88,6 +88,7 @@ def run(ctx: InstallContext) -> None: fail_fast=False, # ensure target check runs even if dep checks re-pass registries=registries_map, direct_dep_keys={d.get_unique_key() for d in getattr(ctx, "all_apm_deps", []) or []}, + declared_refs=getattr(ctx, "declared_direct_refs", None) or None, ) # ------------------------------------------------------------------ diff --git a/src/apm_cli/install/phases/resolve.py b/src/apm_cli/install/phases/resolve.py index bd24364845..bacfeb8928 100644 --- a/src/apm_cli/install/phases/resolve.py +++ b/src/apm_cli/install/phases/resolve.py @@ -411,6 +411,14 @@ def _resolve_dependencies( # 4. Tracking variables (phase-local except where noted) direct_dep_keys = builtins.set(dep.get_unique_key() for dep in ctx.all_apm_deps) + # Keep the manifest-declared refs of the direct deps before resolution + # rewrites ``dep_ref.reference`` to a selected tag further down. The + # policy gate reads these so ``require_pinned_constraint`` judges what + # the author wrote; the resolved ref stays in place for every other + # consumer (download, drift, lockfile). + ctx.declared_direct_refs = { + dep.get_unique_key(): dep.reference for dep in ctx.all_apm_deps if dep.reference is not None + } callback_downloaded: builtins.dict = {} pending_callback_downloads: builtins.dict[Path, tuple[str, str | None]] = {} transitive_failures: builtins.list = [] diff --git a/src/apm_cli/policy/_constraint_pinning.py b/src/apm_cli/policy/_constraint_pinning.py index 432c0d9593..29b9da761a 100644 --- a/src/apm_cli/policy/_constraint_pinning.py +++ b/src/apm_cli/policy/_constraint_pinning.py @@ -9,6 +9,9 @@ * Caret / tilde / bounded semver ranges (``^1.2.3``, ``~1.2.3``, ``>=1.0 <2.0``, ``1.x``). * Literal tag refs (``v1.2.3``, ``v1.2.3-beta.1``). +* Package-qualified literal tags in the layouts the git-semver resolver + selects from (``pkg-a-v1.2.3``, ``pkg-a--v1.2.3``), when ``pkg-a`` is + the dependency's own package name. * Full 40-char SHA refs. A dependency is "unbounded" when any of the following hold: @@ -25,6 +28,11 @@ constraint string only; no remote calls and no subprocess. Used by ``policy.dependencies.require_pinned_constraint`` (see ``policy_checks.py::_check_pinned_constraints``). + +The install pipeline rewrites ``dep.reference`` to the selected tag once +a semver range is resolved, so callers that run after that point pass +the manifest-declared ref through ``declared_ref`` and the classifier +never judges the replacement (#3224). """ from __future__ import annotations @@ -53,6 +61,9 @@ class UnboundedReason(str, Enum): # Literal version tag: ``v1.2.3`` / ``v1.2.3-rc.1`` / ``v1.2.3+meta``. _LITERAL_TAG_RE = re.compile(r"^v\d+\.\d+\.\d+([-+][0-9A-Za-z.\-+]+)?$") +# Placeholder the git-dependency tag layouts use for the package name. +_NAME_PLACEHOLDER = "{name}" + # Wildcard token in any range component. _WILDCARD_TOKEN_RE = re.compile(r"(^|\s)[\*xX](\s|$)") @@ -114,9 +125,60 @@ def _classify_range(spec: str) -> UnboundedReason | None: return None -def classify_unbounded_reason(dep: DependencyReference) -> UnboundedReason | None: +def package_tag_name(dep: DependencyReference) -> str | None: + """Return the ``{name}`` the git-semver resolver matches tags with for *dep*. + + Same rule as ``apm_cli.install.helpers.ref_reuse._git_semver_package_name`` + and OpenAPM 7.3: the final virtual-path component for a selected + subdirectory package, otherwise the final repository path component + without a trailing ``.git``. + """ + virtual_path = getattr(dep, "virtual_path", None) + is_subdirectory = getattr(dep, "is_virtual_subdirectory", None) + if callable(is_subdirectory) and is_subdirectory() and virtual_path: + return virtual_path.rstrip("/").rsplit("/", 1)[-1] or None + repo_url = getattr(dep, "repo_url", None) or "" + name = repo_url.rstrip("/").rsplit("/", 1)[-1] + if name.endswith(".git"): + name = name[: -len(".git")] + return name or None + + +def _is_package_literal_tag(spec: str, dep: DependencyReference) -> bool: + """True when *spec* is a literal tag in a ``{name}`` layout for *dep*'s own name. + + Reuses the resolver's tag layouts and regex builder, so the version + grammar is the one the resolver already selects with. ``{name}`` is + bound to this dependency's package name: ``pkg-b-v1.1.0`` on a + ``pkg-a`` dependency, or ``anything-v1.1.0`` on a repository named + differently, is still a bare branch. + """ + name = package_tag_name(dep) + if not name: + return False + # Lazy imports keep the policy import graph free of resolver modules. + from apm_cli.deps.git_semver_resolver import DEFAULT_TAG_PATTERNS + from apm_cli.marketplace.tag_pattern import build_tag_regex + + for pattern in DEFAULT_TAG_PATTERNS: + if _NAME_PLACEHOLDER not in pattern: + continue + if build_tag_regex(pattern, name=name).match(spec): + return True + return False + + +def classify_unbounded_reason( + dep: DependencyReference, + *, + declared_ref: str | None = None, +) -> UnboundedReason | None: """Return ``None`` if *dep*'s constraint is pinned, otherwise the reason. + ``declared_ref`` is the manifest-declared ref when the caller knows + ``dep.reference`` has since been rewritten to a resolved tag; when + given it is classified in place of ``dep.reference``. + Classification order (first match wins): 1. Local-path dep -> ``None`` (no version surface). @@ -124,6 +186,7 @@ def classify_unbounded_reason(dep: DependencyReference) -> UnboundedReason | Non 3. Empty / missing ref -> ``NO_REF``. 4. 40-char hex SHA -> ``None``. 5. Literal ``v\\d+\\.\\d+\\.\\d+`` tag -> ``None``. + 5a. ``{name}-v…`` / ``{name}--v…`` tag for this dep's own name -> ``None``. 6. Parses as semver range -> range-shape check. 7. Else -> ``BARE_BRANCH``. """ @@ -131,7 +194,7 @@ def classify_unbounded_reason(dep: DependencyReference) -> UnboundedReason | Non if getattr(dep, "is_local", False): return None - ref = getattr(dep, "reference", None) + ref = declared_ref if declared_ref is not None else getattr(dep, "reference", None) source = getattr(dep, "source", None) # Lazy import: avoid pulling marketplace.semver into policy import graph. @@ -162,6 +225,14 @@ def classify_unbounded_reason(dep: DependencyReference) -> UnboundedReason | Non if _LITERAL_TAG_RE.match(spec): return None + # 5a. Package-qualified literal tag for this dependency's own name + # (``pkg-a-v1.1.0``, ``pkg-a--v1.1.0``) -> pinned. The resolver + # selects from exactly these layouts, and the dependency guide + # recommends pinning them. Any other ``...-vX.Y.Z`` string stays + # a bare branch below. + if _is_package_literal_tag(spec, dep): + return None + # 5b. Bare wildcard tokens ('*', 'x', 'X') -- handled before the # semver-range probe because the registry grammar rejects them as # standalone components, but they unambiguously express "any @@ -184,13 +255,22 @@ def is_pinned_constraint(dep: DependencyReference) -> bool: return classify_unbounded_reason(dep) is None -def humanize_reason(reason: UnboundedReason, dep: DependencyReference) -> str: +def humanize_reason( + reason: UnboundedReason, + dep: DependencyReference, + *, + ref: str | None = None, +) -> str: """Render a one-line actionable hint for the given reason. + ``ref`` is the declared ref to name in the hint when the caller + classified something other than ``dep.reference``. + Emitted in ``[x]``/``[!]`` policy diagnostics; ASCII only per ``.github/instructions/encoding.instructions.md``. """ - ref = getattr(dep, "reference", None) or "" + if ref is None: + ref = getattr(dep, "reference", None) or "" if reason is UnboundedReason.NO_REF: return "no ref; resolves to default branch" if reason is UnboundedReason.BARE_BRANCH: diff --git a/src/apm_cli/policy/_help_text.py b/src/apm_cli/policy/_help_text.py index 126f1145ef..99486b2a68 100644 --- a/src/apm_cli/policy/_help_text.py +++ b/src/apm_cli/policy/_help_text.py @@ -25,7 +25,9 @@ # .github/instructions/encoding.instructions.md. REQUIRE_PINNED_CONSTRAINT_HELP = ( "When true, every direct APM dep must declare a bounded constraint " - "(exact version, '^'/'~'/bounded range, literal tag, or SHA). " + "(exact version, '^'/'~'/bounded range, literal tag such as 'v1.2.3' " + "or 'pkg-a-v1.2.3' for a package named pkg-a, or SHA). The declared " + "constraint is judged, not the tag a range resolved to. " "Unbounded refs (missing ref, '*', bare branch, bare '>=X.Y') are " "reported through 'policy.enforcement' (warn | block). " "Default: false. Recommend rolling out with 'enforcement: warn' first." diff --git a/src/apm_cli/policy/policy_checks.py b/src/apm_cli/policy/policy_checks.py index b303a6cf2f..3c1e047416 100644 --- a/src/apm_cli/policy/policy_checks.py +++ b/src/apm_cli/policy/policy_checks.py @@ -1062,15 +1062,20 @@ def _check_pinned_constraints( deps: list[DependencyReference], policy: DependencyPolicy, direct_dep_keys: set[str] | None = None, + declared_refs: dict[str, str] | None = None, ) -> CheckResult: """Check: every direct dep declares a bounded constraint. Skipped (passes vacuously) when ``policy.require_pinned_constraint`` is ``False`` -- the default. - Operates on the **declared** constraint (``dep.reference``), not - the resolved one, so authors learn before the install completes - that a moving ref slipped past review. + Operates on the **declared** constraint, not the resolved one, so + authors learn before the install completes that a moving ref + slipped past review. After the resolve phase ``dep.reference`` + already holds the selected tag, so the install-pipeline callers + pass ``declared_refs`` (unique key -> manifest ref) and the check + classifies those instead (#3224). Callers that still hold the + manifest view (preflight, audit) leave it ``None``. When ``direct_dep_keys`` is provided, the check is restricted to direct dependencies only -- transitives are excluded, since the @@ -1096,13 +1101,15 @@ def _check_pinned_constraints( violations: list[str] = [] for dep in deps: - if direct_dep_keys is not None and dep.get_unique_key() not in direct_dep_keys: + unique_key = dep.get_unique_key() + if direct_dep_keys is not None and unique_key not in direct_dep_keys: continue - reason = classify_unbounded_reason(dep) + declared = declared_refs.get(unique_key) if declared_refs else None + reason = classify_unbounded_reason(dep, declared_ref=declared) if reason is None: continue key = dep.get_canonical_dependency_string() - hint = humanize_reason(reason, dep) + hint = humanize_reason(reason, dep, ref=declared) violations.append(f"{key}: {hint}") if not violations: @@ -1191,6 +1198,7 @@ def run_dependency_policy_checks( manifest_includes=_INCLUDES_NOT_PROVIDED, registries: dict[str, str] | None = None, direct_dep_keys: set[str] | None = None, + declared_refs: dict[str, str] | None = None, ) -> CIAuditResult: """Evaluate :class:`ApmPolicy` against an already-resolved dependency set. @@ -1237,6 +1245,12 @@ def run_dependency_policy_checks( package's own manifest. When ``None`` (legacy dep-only seam and the audit wrapper that already iterates direct-only manifest deps) every dep in ``deps_to_install`` is evaluated. + declared_refs: + Optional map of ``get_unique_key()`` -> manifest-declared ref for + the direct deps. The resolve phase rewrites ``dep.reference`` to + the selected tag, so the install-pipeline callers pass the refs + they captured beforehand and ``require_pinned_constraint`` judges + the authored constraint rather than its replacement (#3224). Returns ------- @@ -1308,7 +1322,9 @@ def _run(check: CheckResult) -> bool: # When direct_dep_keys is supplied, restrict to direct deps -- a # transitive package with an unbounded ref in its own manifest is # not actionable by the consumer (see Copilot review on #1494). - _check_pinned_constraints(deps_list, policy.dependencies, direct_dep_keys), + _check_pinned_constraints( + deps_list, policy.dependencies, direct_dep_keys, declared_refs=declared_refs + ), ] # MCP checks -- when mcp_deps is None (not provided), skip entirely. # When mcp_deps is an empty list (provided but no MCP deps), still diff --git a/tests/integration/policy/test_require_pinned_constraint_declared_e2e.py b/tests/integration/policy/test_require_pinned_constraint_declared_e2e.py new file mode 100644 index 0000000000..8f86b76881 --- /dev/null +++ b/tests/integration/policy/test_require_pinned_constraint_declared_e2e.py @@ -0,0 +1,321 @@ +"""Real-binary contract for #3224: ``require_pinned_constraint`` judges the +declared constraint, and recognises this package's own release tags. + +One local bare monorepo publishes a subdirectory package ``pkg-a``. The +repository root is tagged ``v1.0.0`` at the first commit only; the package +is tagged ``pkg-a-v1.1.0`` and ``pkg-a--v1.1.0`` at the second. A consumer +declaring ``^1.0.0`` therefore resolves to ``pkg-a-v1.1.0``, which is the +tag the old gate classified as a bare branch after resolution rewrote the +in-memory reference. + +Policy reaches ``install`` the way the Govern contract does: the project +carries an ADO-shaped origin, the strict policy lives behind a leaf whose +path doubles as auto-discovery's cache key, and ``apm policy status`` warms +the cache so the offline install, frozen replay, update and audit all +enforce the same policy. Every governed command asserts the cached-policy +diagnostic, so a pass can never come from the policy silently not loading. + +This contract owns one verdict: ``dependency-pinned-constraint``. Audits +are read as JSON and judged by that check, not by the exit code, because +other checks report on what replay records for this package shape and that +belongs to the lock contracts. +""" + +from __future__ import annotations + +import json +import os +import subprocess +from pathlib import Path, PurePosixPath + +import pytest + +from apm_cli.utils.yaml_io import dump_yaml, load_yaml +from tests.utils.apm_lifecycle_runner import ApmLifecycleRunner, CommandResult +from tests.utils.isolated_apm_environment import IsolatedApmEnvironment +from tests.utils.local_git_repository import LocalGitRepositoryFactory +from tests.utils.local_package import LocalPackageFactory + +pytestmark = [ + pytest.mark.e2e, + pytest.mark.requires_e2e_mode, + pytest.mark.requires_apm_binary, +] + +_POLICY_SOURCE = "dev.azure.com/contoso/apm/apm-policy" +_POLICY_REMOTE = "https://dev.azure.com/contoso/project/_git/pinned-declared" +_POLICY_DIAGNOSTIC = f"Policy: org:{_POLICY_SOURCE} (cached" +_GIT_URL = "https://github.com/acme/mono.git" +_SUBDIR_DEP = "acme/mono/packages/pkg-a" +_ROOT_DEP = "acme/mono" +_SKILL = "pkg-a-skill" +_DEPLOYED_SKILL = Path(".agents") / "skills" / _SKILL / "SKILL.md" +_COMMON = ("--target", "copilot", "--parallel-downloads", "0") +_AUDIT = ( + "audit", + "--ci", + "--no-fail-fast", + "--no-drift", + "--format", + "json", + "--policy", + _POLICY_SOURCE, +) + + +def _skill_document(marker: str) -> str: + return ( + f"---\nname: {_SKILL}\ndescription: Declared-constraint contract skill\n---\n\n# {marker}\n" + ) + + +def _write_subpackage(repo_root: Path, version: str, marker: str) -> None: + pkg = repo_root / "packages" / "pkg-a" + (pkg / ".apm" / "skills" / _SKILL).mkdir(parents=True, exist_ok=True) + dump_yaml({"name": "pkg-a", "version": version, "targets": ["copilot"]}, pkg / "apm.yml") + (pkg / ".apm" / "skills" / _SKILL / "SKILL.md").write_text(_skill_document(marker)) + + +def _publish_monorepo(isolated: IsolatedApmEnvironment, env: dict[str, str]) -> dict[str, str]: + packages = LocalPackageFactory(isolated.package_root) + bundle = packages.create("mono", targets=("copilot",)) + packages.add_skill( + bundle, "mono-skill", "---\nname: mono-skill\ndescription: root\n---\n# root\n" + ) + _write_subpackage(bundle.root, "1.0.0", "pkg-a one") + + repositories = LocalGitRepositoryFactory(isolated.repository_root, env=env) + repository = repositories.create("mono", source_tree=bundle.root) + first = repositories.commit(repository, message="pkg-a 1.0.0") + repositories.tag(repository, "v1.0.0", first) + repositories.tag(repository, "pkg-a-v1.0.0", first) + + _write_subpackage(repository.worktree, "1.1.0", "pkg-a two") + second = repositories.commit(repository, message="pkg-a 1.1.0") + repositories.tag(repository, "pkg-a-v1.1.0", second) + repositories.tag(repository, "pkg-a--v1.1.0", second) + + return repositories.url_rewrite_subprocess_env(repository, _GIT_URL) + + +def _git(args: tuple[str, ...], *, cwd: Path, env: dict[str, str]) -> None: + subprocess.run(args, cwd=cwd, env=env, capture_output=True, text=True, check=True, timeout=30) + + +def _consumer(root: Path, name: str, dependency: str, env: dict[str, str]) -> Path: + project = LocalPackageFactory(root).create( + name, dependencies=(dependency,), targets=("copilot",) + ) + _git(("git", "init", "--initial-branch=main"), cwd=project.root, env=env) + _git(("git", "remote", "add", "origin", _POLICY_REMOTE), cwd=project.root, env=env) + # The strict policy is a separate file and the leaf extends it: only a + # leaf with ``extends`` goes through chain resolution, which is what + # persists the merged policy under the leaf's path. That path is also + # the cache key auto-discovery derives from the ADO origin, so the + # offline install/update/frozen runs below enforce it as ``cached``. + strict = project.root / ".apm-policy" / "strict.yml" + strict.parent.mkdir() + dump_yaml( + { + "name": "pinned-declared-contract", + "version": "1", + "enforcement": "block", + "fetch_failure": "block", + "dependencies": {"require_pinned_constraint": True}, + }, + strict, + ) + leaf = project.root.joinpath(*PurePosixPath(_POLICY_SOURCE).parts) + leaf.parent.mkdir(parents=True) + dump_yaml( + {"name": "pinned-declared-leaf", "extends": strict.relative_to(project.root).as_posix()}, + leaf, + ) + return project.root + + +def _run( + runner: ApmLifecycleRunner, + args: tuple[str, ...], + *, + cwd: Path, + env: dict[str, str], + scenario_id: str, + expected: int | None, +) -> CommandResult: + result = runner.run(args, scenario_id=scenario_id, cwd=cwd, env=env) + assert expected is None or result.returncode == expected, ( + f"{scenario_id}: expected exit {expected}, got {result.returncode}\n" + f"stdout={result.stdout!r}\nstderr={result.stderr!r}" + ) + return result + + +def _governed(result: CommandResult) -> CommandResult: + """Fail loudly if the command ran without the cached org policy in force.""" + text = result.stdout + result.stderr + assert _POLICY_DIAGNOSTIC in text, f"policy was not enforced from cache:\n{text}" + return result + + +def _warm_policy(runner: ApmLifecycleRunner, cwd: Path, env: dict[str, str], tag: str) -> None: + _run( + runner, + ("policy", "status", "--policy-source", _POLICY_SOURCE, "--json", "--check", "--no-cache"), + cwd=cwd, + env=env, + scenario_id=f"{tag}-warm-policy", + expected=0, + ) + + +def _pinned_verdict(runner, cwd: Path, env: dict[str, str], tag: str) -> dict[str, object]: + """Audit under the policy and return the ``dependency-pinned-constraint`` check.""" + audit = _run(runner, _AUDIT, cwd=cwd, env=env, scenario_id=f"{tag}-audit", expected=None) + payload = json.loads(audit.stdout) + assert payload["policy"]["enforced"] is True, payload["policy"] + checks = {check["name"]: check for check in payload["checks"]} + assert "dependency-pinned-constraint" in checks, sorted(checks) + return checks["dependency-pinned-constraint"] + + +def _lock_entry(project_root: Path) -> dict[str, object]: + lock = load_yaml(project_root / "apm.lock.yaml") + deps = lock["dependencies"] + assert len(deps) == 1, deps + return deps[0] + + +@pytest.fixture(scope="module") +def scenario(tmp_path_factory, apm_binary_path: Path): + root = tmp_path_factory.mktemp("pinned-declared") + isolated = IsolatedApmEnvironment.create(root / "isolated", base_env=dict(os.environ)) + env = _publish_monorepo(isolated, isolated.subprocess_env()) + runner = ApmLifecycleRunner( + (str(apm_binary_path),), timeout_seconds=120, scenario_timeout_seconds=600 + ) + return isolated, env, runner + + +def test_declared_range_passes_across_install_replay_update_and_audit(scenario): + isolated, env, runner = scenario + project = _consumer(isolated.work_root, "range-consumer", f"{_SUBDIR_DEP}#^1.0.0", env) + _warm_policy(runner, project, env, "range") + + # Fresh install: the range resolves to the package-qualified tag, which + # is exactly what the gate used to reject after the in-memory rewrite. + fresh = _governed( + _run( + runner, + ("install", *_COMMON), + cwd=project, + env=env, + scenario_id="range-fresh", + expected=0, + ) + ) + assert "blocked by org policy" not in (fresh.stdout + fresh.stderr).lower() + entry = _lock_entry(project) + assert entry["constraint"] == "^1.0.0", entry + assert entry["resolved_tag"] == "pkg-a-v1.1.0", entry + assert "pkg-a two" in (project / _DEPLOYED_SKILL).read_text() + assert _pinned_verdict(runner, project, env, "range-fresh")["passed"] is True + + # Frozen replay, a repeat ordinary install and a deliberate update all + # re-run the gate against the manifest range; the verdict must not move. + for tag, args in ( + ("range-frozen", ("install", *_COMMON, "--frozen")), + ("range-repeat-install", ("install", *_COMMON)), + ("range-update", ("update", "--yes", *_COMMON)), + ): + _governed(_run(runner, args, cwd=project, env=env, scenario_id=tag, expected=0)) + assert _pinned_verdict(runner, project, env, tag)["passed"] is True + + manifest = load_yaml(project / "apm.yml") + assert manifest["dependencies"]["apm"] == [f"{_SUBDIR_DEP}#^1.0.0"], manifest + + +@pytest.mark.parametrize( + ("name", "reference"), + [ + ("literal-dash", "pkg-a-v1.1.0"), + ("literal-double-dash", "pkg-a--v1.1.0"), + ], +) +def test_package_qualified_literal_tags_install_and_audit(scenario, name, reference): + isolated, env, runner = scenario + project = _consumer(isolated.work_root, f"{name}-consumer", f"{_SUBDIR_DEP}#{reference}", env) + _warm_policy(runner, project, env, name) + _governed( + _run( + runner, + ("install", *_COMMON), + cwd=project, + env=env, + scenario_id=f"{name}-install", + expected=0, + ) + ) + assert "pkg-a two" in (project / _DEPLOYED_SKILL).read_text() + assert _pinned_verdict(runner, project, env, f"{name}-fresh")["passed"] is True + _governed( + _run( + runner, + ("install", *_COMMON, "--frozen"), + cwd=project, + env=env, + scenario_id=f"{name}-frozen", + expected=0, + ) + ) + assert _pinned_verdict(runner, project, env, f"{name}-frozen")["passed"] is True + + +def test_root_repository_range_still_passes(scenario): + isolated, env, runner = scenario + project = _consumer(isolated.work_root, "root-range-consumer", f"{_ROOT_DEP}#^1.0.0", env) + _warm_policy(runner, project, env, "root-range") + _governed( + _run( + runner, + ("install", *_COMMON), + cwd=project, + env=env, + scenario_id="root-range-install", + expected=0, + ) + ) + entry = _lock_entry(project) + assert entry["constraint"] == "^1.0.0" and entry["resolved_tag"] == "v1.0.0", entry + assert _pinned_verdict(runner, project, env, "root-range")["passed"] is True + + +@pytest.mark.parametrize( + ("name", "reference", "hint"), + [ + ("branch", "main", "bare branch 'main'"), + ("other-package-tag", "pkg-b-v1.1.0", "bare branch 'pkg-b-v1.1.0'"), + ("open-upper", ">=1.0.0", "unbounded upper"), + ], +) +def test_unbounded_declarations_stay_blocked(scenario, name, reference, hint): + isolated, env, runner = scenario + project = _consumer(isolated.work_root, f"{name}-consumer", f"{_SUBDIR_DEP}#{reference}", env) + _warm_policy(runner, project, env, name) + blocked = _governed( + _run( + runner, + ("install", *_COMMON), + cwd=project, + env=env, + scenario_id=f"{name}-install", + expected=1, + ) + ) + text = blocked.stdout + blocked.stderr + assert "blocked by org policy" in text.lower(), text + assert hint in text, text + assert not (project / "apm.lock.yaml").exists() + verdict = _pinned_verdict(runner, project, env, name) + assert verdict["passed"] is False, verdict + assert any(hint in detail for detail in verdict["details"]), verdict diff --git a/tests/integration/policy/test_require_pinned_constraint_e2e.py b/tests/integration/policy/test_require_pinned_constraint_e2e.py index 2df381e48d..81c338fd7a 100644 --- a/tests/integration/policy/test_require_pinned_constraint_e2e.py +++ b/tests/integration/policy/test_require_pinned_constraint_e2e.py @@ -250,6 +250,64 @@ def test_policy_gate_forwards_direct_dep_keys_to_runner( ) +class TestDeclaredRefsWiring: + """#3224: the gate must forward the manifest-declared refs to the runner. + + The resolve phase rewrites ``dep_ref.reference`` to the selected tag + before the gate runs, so without ``declared_refs`` the pinned-constraint + check would judge the tag instead of the range the author wrote. + """ + + @patch(_PATCH_UPDATES, return_value=None) + @patch(_PATCH_DOWNLOADER) + @patch(_PATCH_DISCOVER_PREFLIGHT) + @patch(_PATCH_DISCOVER_GATE) + @patch(_PATCH_RUN_DEP_CHECKS) + def test_policy_gate_forwards_declared_refs_to_runner( + self, + mock_run_checks, + mock_gate, + mock_preflight, + mock_dl, + mock_updates, + project, + ): + project_dir, runner = project + _write_apm_yml( + project_dir / "apm.yml", + deps=["test-org/skills#^1.2.0"], + ) + fetch = _fetch(_load_fixture("apm-policy-block.yml")) + mock_gate.return_value = fetch + mock_preflight.return_value = fetch + + from apm_cli.policy.models import CIAuditResult + + mock_run_checks.return_value = CIAuditResult(checks=[]) + + _invoke_install(runner) + + # The gate is the call that carries ``fetch_outcome`` with no + # ``effective_target`` (preflight passes neither; the post-targets + # re-check passes a target). Pin the assertion to that call so a + # regression in the gate cannot hide behind the other callers. + gate_calls = [ + c + for c in mock_run_checks.call_args_list + if "fetch_outcome" in c.kwargs and c.kwargs.get("effective_target") is None + ] + assert gate_calls, ( + f"policy_gate did not run the dep checks: {mock_run_checks.call_args_list}" + ) + for call in gate_calls: + declared = call.kwargs.get("declared_refs") or {} + matching = [ref for key, ref in declared.items() if "test-org/skills" in str(key)] + assert matching == ["^1.2.0"], ( + "policy_gate did not forward the manifest-declared range for the " + f"direct dep; it would judge the resolved tag instead. kwargs: {call.kwargs!r}" + ) + + # --------------------------------------------------------------------------- # Promise D + G: actionable diagnostic + exit code 1 # --------------------------------------------------------------------------- diff --git a/tests/unit/policy/test_pinned_constraint_declared.py b/tests/unit/policy/test_pinned_constraint_declared.py new file mode 100644 index 0000000000..a29d0bd349 --- /dev/null +++ b/tests/unit/policy/test_pinned_constraint_declared.py @@ -0,0 +1,189 @@ +"""Declared-constraint and package-qualified tag coverage for #3224. + +Two things changed in ``require_pinned_constraint``: + +* Literal tags in the layouts the git-semver resolver selects from + (``{name}-v{version}``, ``{name}--v{version}``) count as pinned when + ``{name}`` is the dependency's own package name. +* The check can be told the manifest-declared ref when ``dep.reference`` + has already been rewritten to the selected tag, so a range is judged as + the range the author wrote and a bare branch stays a violation even + after resolution replaced it with a tag. +""" + +from __future__ import annotations + +import pytest + +from apm_cli.install.helpers.ref_reuse import _git_semver_package_name +from apm_cli.models.apm_package import DependencyReference +from apm_cli.policy._constraint_pinning import ( + UnboundedReason, + classify_unbounded_reason, + humanize_reason, + package_tag_name, +) +from apm_cli.policy.policy_checks import _check_pinned_constraints +from apm_cli.policy.schema import DependencyPolicy + + +def _dep(spec: str) -> DependencyReference: + return DependencyReference.parse(spec) + + +# --------------------------------------------------------------------------- +# Package-qualified literal tags +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "spec", + [ + "acme/mono/packages/pkg-a#pkg-a-v1.1.0", + "acme/mono/packages/pkg-a#pkg-a--v1.1.0", + "acme/mono/packages/pkg-a#pkg-a-v1.1.0-rc.1", + "acme/mono/packages/pkg-a#pkg-a-v1.1.0+build.5", + "acme/widget#widget-v1.2.0", + "acme/widget#widget--v1.2.0", + "https://github.com/acme/widget.git#widget-v1.2.0", + ], +) +def test_package_qualified_literal_tag_for_own_name_is_pinned(spec): + assert classify_unbounded_reason(_dep(spec)) is None + + +@pytest.mark.parametrize( + "spec", + [ + # another package's tag on a subdirectory dep + "acme/mono/packages/pkg-a#pkg-b-v1.1.0", + # the repository's tag layout on a subdirectory dep + "acme/mono/packages/pkg-a#mono-v1.1.0", + # a subdirectory package's tag on the repository dep + "acme/mono#pkg-a-v1.1.0", + # version-looking suffix that is not a full semver + "acme/mono/packages/pkg-a#pkg-a-v1.1", + "acme/mono/packages/pkg-a#pkg-a-v1", + # arbitrary prefix + "acme/mono/packages/pkg-a#release-pkg-a-v1.1.0", + # layouts the resolver does not select from + "acme/mono/packages/pkg-a#pkg-a_v1.1.0", + "acme/mono/packages/pkg-a#pkg-a@1.1.0", + ], +) +def test_other_names_and_near_misses_stay_bare_branch(spec): + assert classify_unbounded_reason(_dep(spec)) is UnboundedReason.BARE_BRANCH + + +@pytest.mark.parametrize( + "spec", + [ + "acme/mono/packages/pkg-a#^1.0.0", + "acme/mono#^1.0.0", + "acme/widget#widget-v1.2.0", + "https://github.com/acme/widget.git#widget-v1.2.0", + ], +) +def test_package_tag_name_matches_the_resolver(spec): + dep = _dep(spec) + assert package_tag_name(dep) == _git_semver_package_name(dep) + + +def test_package_tag_name_handles_missing_fields(): + class Bare: + pass + + assert package_tag_name(Bare()) is None + + +# --------------------------------------------------------------------------- +# Declared ref beats the rewritten reference +# --------------------------------------------------------------------------- + + +def _resolved(spec: str, selected_tag: str) -> DependencyReference: + """A dep whose ``reference`` the resolve phase already replaced.""" + dep = _dep(spec) + dep.reference = selected_tag + return dep + + +def test_declared_range_is_judged_not_the_selected_tag(): + dep = _resolved("acme/mono/packages/pkg-a#^1.0.0", "pkg-a-v1.1.0") + assert dep.reference == "pkg-a-v1.1.0" + assert classify_unbounded_reason(dep, declared_ref="^1.0.0") is None + + +def test_selected_tag_does_not_launder_a_bare_branch(): + dep = _resolved("acme/mono/packages/pkg-a#main", "pkg-a-v1.1.0") + assert classify_unbounded_reason(dep, declared_ref="main") is UnboundedReason.BARE_BRANCH + + +def test_selected_sha_does_not_launder_an_open_upper_range(): + dep = _resolved("acme/widget#>=1.0.0", "a" * 40) + assert classify_unbounded_reason(dep) is None # the SHA alone would pass + assert classify_unbounded_reason(dep, declared_ref=">=1.0.0") is UnboundedReason.OPEN_UPPER + + +def test_declared_ref_none_falls_back_to_reference(): + dep = _dep("acme/widget#main") + assert classify_unbounded_reason(dep, declared_ref=None) is UnboundedReason.BARE_BRANCH + + +def test_humanize_names_the_declared_ref(): + dep = _resolved("acme/widget#main", "widget-v1.2.0") + hint = humanize_reason(UnboundedReason.BARE_BRANCH, dep, ref="main") + assert "'main'" in hint + assert "widget-v1.2.0" not in hint + + +# --------------------------------------------------------------------------- +# The check itself, with and without declared refs +# --------------------------------------------------------------------------- + + +def _policy() -> DependencyPolicy: + return DependencyPolicy(require_pinned_constraint=True) + + +def test_check_passes_resolved_range_when_declared_refs_are_supplied(): + dep = _resolved("acme/mono/packages/pkg-a#^1.0.0", "pkg-a-v1.1.0") + result = _check_pinned_constraints( + [dep], + _policy(), + {dep.get_unique_key()}, + declared_refs={dep.get_unique_key(): "^1.0.0"}, + ) + assert result.passed, result.details + + +def test_check_still_fails_declared_branch_after_resolution(): + dep = _resolved("acme/mono/packages/pkg-a#main", "pkg-a-v1.1.0") + result = _check_pinned_constraints( + [dep], + _policy(), + {dep.get_unique_key()}, + declared_refs={dep.get_unique_key(): "main"}, + ) + assert not result.passed + assert any("bare branch 'main'" in line for line in result.details), result.details + + +def test_check_without_declared_refs_keeps_the_old_contract(): + # Callers that hold the manifest view (preflight, audit) pass nothing + # and the check reads ``dep.reference`` as before. + assert _check_pinned_constraints([_dep("acme/widget#^1.0.0")], _policy()).passed + assert not _check_pinned_constraints([_dep("acme/widget#main")], _policy()).passed + + +def test_declared_refs_only_apply_to_their_own_key(): + good = _resolved("acme/mono/packages/pkg-a#^1.0.0", "pkg-a-v1.1.0") + other = _dep("acme/widget#main") + result = _check_pinned_constraints( + [good, other], + _policy(), + {good.get_unique_key(), other.get_unique_key()}, + declared_refs={good.get_unique_key(): "^1.0.0"}, + ) + assert not result.passed + assert len(result.details) == 1 and "acme/widget" in result.details[0]