Skip to content

fix(deps): reject incompatible immutable requirements - #3061

Merged
Daniel Meppiel (danielmeppiel) merged 3 commits into
mainfrom
danielmeppiel-issue-delivery-3027-f6b
Sep 24, 2026
Merged

Daniel Meppiel (danielmeppiel) merged 3 commits into
mainfrom
danielmeppiel-issue-delivery-3027-f6b

Conversation

@danielmeppiel

@danielmeppiel Daniel Meppiel (danielmeppiel) commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

fix(deps): reject incompatible immutable requirements

TL;DR

Preserve immutable dependency requirements before single-version hoisting and
reject graphs whose required commits disagree. Frozen installs also reject
locked commits that discard an immutable transitive requirement. Equivalent
tag/SHA spellings remain valid; errors identify both dependency paths and refs.

Closes #3027.

Note

Bounded human scope: #3027 (comment)
This PR does not introduce side-by-side versions, a new resolution mode, or a lockfile schema change.

Problem (WHY)

  • On current main, a root pin and a parent's incompatible immutable pin
    produced a valid graph with no resolution errors and only one selected ref.
  • The structural frozen preflight accepted the resulting collapsed lock.
    It did not inspect the immutable requirement in the parent's manifest.
  • [!] Rejecting unequal ref strings would also reject valid tag aliases and
    tag/SHA pairs that identify the same commit.

These are reproduced dependency-contract failures, not an inference from ref
names. The evidence-first workflow follows Agent Skills:
"Even a single pass of execute-then-revise noticeably improves quality, and complex domains often benefit from several."
The tests exercise both the rejected graph and valid equivalent requirements.

Approach (WHAT)

  • Admit every eligible literal requirement before the existing winner filter
    can discard its download work.
  • Compare proven commit identities, using the downloader's existing ref APIs
    for tag classification and short-SHA expansion.
  • Carry the existing frozen option into resolution and compare manifest
    obligations with the lockfile's selected commits.
  • Keep one registered compatibility owner, with behavior and static-boundary
    regressions instead of duplicating policy in the CLI or lockfile reader.

Implementation (HOW)

The production change is a new compatibility owner composed into the existing
resolver; materialization selection and lockfile identity remain unchanged.

File Intent
src/apm_cli/deps/immutable_requirements.py Snapshot declared refs, retain chain provenance, compare immutable commits, and verify frozen pins. Cache ref lookups per resolution/repository identity.
src/apm_cli/deps/apm_resolver.py Admit BFS work items through that owner before selecting materialization winners. Return existing resolution errors on failure.
src/apm_cli/install/context.py Carry frozen state alongside existing install context.
src/apm_cli/install/pipeline.py Pass frozen state into the context without adding a command flag.
src/apm_cli/install/service.py Preserve structural preflight and pass frozen state into the pipeline.
src/apm_cli/install/phases/resolve.py Supply the existing downloader and frozen state to the resolver.
.apm/architecture/owners/install-deployment.json Register the immutable-compatibility decision owner and guard.
scripts/architecture_linter/checks/install_deployment_analyzers.py Register the static rule.
scripts/architecture_linter/checks/install_uninstall_and_resolution.py Detect duplicate ownership and missing canonical/frozen routing.
tests/unit/deps/test_immutable_requirement_conflicts.py Cover incompatible chains, equivalent refs, lookup failure, frozen pins, and unchanged identity/range behavior.
tests/unit/deps/test_apm_resolver_parallel.py Preserve the one-winner/concurrent-download regression using proven equivalent tags.
tests/integration/test_immutable_requirements_install.py Exercise real Click/service/pipeline/resolver flows across normal/frozen and cold/warm installs.
tests/integration/test_architecture_immutable_requirements.py Prove a copied owner is rejected.
tests/integration/test_architecture_owner_rule_mutations.py Prove removal of canonical admission is rejected.
docs/src/content/docs/consumer/manage-dependencies.md Explain conflicts, equivalence, and recovery.
docs/src/content/docs/reference/cli/install.md Document failure behavior and the cold-cache frozen boundary.
docs/src/content/docs/reference/lockfile-spec.md Document transitive immutable verification during frozen replay.
packages/apm-guide/.apm/skills/apm-usage/commands.md Update command guidance for conflict handling.
packages/apm-guide/.apm/skills/apm-usage/dependencies.md Update pinning and equivalence guidance.

Implementation reference:
ImmutableRequirements at feb026d990.
No README, CHANGELOG, authentication, package-identity, or lockfile-schema changes.

Diagrams

Legend: immutable admission runs before winner filtering; failures use the
existing error exit before deployment and lockfile commit.

flowchart TD
    A["InstallService.run: structural frozen preflight"] --> B["run_install_pipeline: frozen in InstallContext"]
    B --> C["APMDependencyResolver.build_dependency_tree"]
    C --> D["ImmutableRequirements.add: each declared literal edge"]
    D --> E{"Distinct refs or frozen pin check?"}
    E -->|yes| F["Compare full SHAs or query existing downloader refs"]
    F --> G{"Verified immutable commits differ?"}
    G -->|yes| H["Record resolution error with chains and refs"]
    F -->|unverifiable| H
    E -->|no| I["_select_dependency_winners"]
    G -->|no| I
    I --> J["Download selected packages and load manifests"]
    J -->|transitive edges| D
    H --> K["_fail_on_resolution_errors: exit 1"]
    J -->|complete valid graph| L["Continue deployment and lockfile commit"]
Loading

Trade-offs

  • Bounded solver change. Branches and semver ranges retain their current
    policy; this is not general range solving or multiversion isolation.
  • Proof can require network access. Distinct named refs use existing
    downloader lookups. Unverifiable refs fail explicitly rather than being
    guessed compatible or reported as a proven conflict.
  • Frozen is not a zero-I/O promise. Structural drift still fails before
    the pipeline. A cold cache may fetch a locked parent before its transitive
    constraint becomes discoverable.
  • Existing lock pins stay authoritative. Same-spelling locked tags retain
    their pinned commit unless refs are being updated; alias comparison does not
    silently substitute a moved upstream tag.
  • One owner, existing error path. A small registered module replaces
    scattered compatibility checks. No new conflict mode, lockfile format, or
    deployment layout is introduced.

Benefits

  1. Direct/transitive and transitive/transitive immutable conflicts exit with
    both root-to-package chains instead of reporting a valid graph.
  2. Frozen replay cannot conceal a mismatching immutable transitive pin.
  3. Equivalent tags, tag/SHA pairs, and short/full SHAs remain acceptable.
  4. Conflicting CLI installs leave the previous lockfile bytes unchanged and
    do not deploy skills.

Validation

Local validation ran against main 0bd8da62d8 plus this commit.
Remote CI has not run at PR preparation time; this is not a green-CI claim.

Relevant regression suite: command and result
uv run --frozen --extra dev pytest -q \
  tests/unit/deps/test_apm_resolver*.py \
  tests/unit/deps/test_immutable_requirement_conflicts.py \
  tests/test_apm_resolver.py \
  tests/unit/install/test_resolve*.py \
  tests/unit/install/phases/test_resolve*.py \
  tests/unit/install/test_frozen.py \
  tests/unit/install/test_service*.py \
  tests/unit/install/test_pipeline*.py \
  tests/integration/test_immutable_requirements_install.py \
  tests/integration/test_architecture_immutable_requirements.py --tb=short
561 passed, 9 subtests passed in 6.17s

The canonical local lint mirror passed: Ruff check/format, Pylint R0801,
YAML I/O safety, the 2100-line limit, portable relative paths, and auth signals.
Architecture-boundary lint passed. The selected architecture mutation tests
passed; the quality suite passed 63 tests, and assertion/duplicate ratchets
passed. Mermaid CLI 11.15.0 rendered the diagram successfully.

TDD evidence: the four SHA-conflict cases failed before the fix. Temporarily
disabling the production commit-inequality check then caused five regression
failures; the guard was restored before the final passing run.

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 #3027: conflicting root/parent or two-parent pins fail and name both paths. DevX tests/unit/deps/test_immutable_requirement_conflicts.py::test_conflicting_commit_requirements_fail_with_both_chains unit
2 Different tag names are accepted only when they identify the same commit. DevX tests/unit/deps/test_immutable_requirement_conflicts.py::test_tag_spelling_is_not_conflict_evidence unit
3 Tag/SHA and abbreviated/full-SHA equivalents still install. DevX tests/unit/deps/test_immutable_requirement_conflicts.py::test_equivalent_tag_or_short_sha_is_accepted unit
4 Frozen replay rejects a collapsed pin but accepts a matching commit. Secure by default tests/unit/deps/test_immutable_requirement_conflicts.py::test_frozen_transitive_requirement_must_match_locked_commit unit
5 Normal/frozen, cold/warm installs reject conflicts without committing the bad lock or deploying skills. Secure by default, DevX tests/integration/test_immutable_requirements_install.py::test_install_checks_every_immutable_requirement integration
6 An unavailable ref lookup is an explicit verification error, not invented conflict evidence. DevX tests/unit/deps/test_immutable_requirement_conflicts.py::test_unverifiable_names_fail_without_claiming_a_proven_conflict unit

How to test

  • With uv available, run the regression command above; expect 561 passing
    tests and nine passing subtests.
  • Run uv run --frozen --extra dev pytest -q tests/integration/test_immutable_requirements_install.py;
    expect all 16 real-CLI matrix cases to pass without remote access.
  • Run PATH="/opt/homebrew/bin:$PATH" UV_FROZEN=1 bash scripts/lint-architecture-boundaries.sh
    (or put your installed Node on PATH); expect exit 0.
  • Run uv run --frozen --extra dev pytest -q tests/integration/test_architecture_owner_rule_mutations.py -k immutable;
    expect the guard mutation to be detected.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Preserve immutable dependency obligations before single-version hoisting and validate frozen replay against locked commits. Keep equivalent tag and SHA references compatible, expose both dependency chains on conflicts, and add CLI regressions and canonical-owner guards.

Refs #3027

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved findings remain in immutable requirement handling, including short-SHA frozen replay and ref lookup behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Adds immutable dependency conflict detection before hoisting and validates transitive requirements during frozen installs.

Changes:

  • Adds commit-based compatibility checks and dependency-chain diagnostics.
  • Propagates frozen state through installation and registers architecture guards.
  • Adds regression coverage and updates documentation.
File Summary
tests/​unit/​deps/​test_immutable_requirement_conflicts.py Covers conflicts, equivalent refs, lookup failures, and frozen pins.
tests/​unit/​deps/​test_apm_resolver_parallel.py Preserves parallel-resolution behavior for equivalent refs.
tests/​integration/​test_immutable_requirements_install.py Covers normal and frozen install flows.
tests/​integration/​test_architecture_owner_rule_mutations.py Verifies canonical admission cannot be removed.
tests/​integration/​test_architecture_immutable_requirements.py Verifies duplicate ownership is rejected.
src/​apm_cli/​install/​service.py Passes frozen state through installation.
src/​apm_cli/​install/​pipeline.py Carries frozen state into the install context.
src/​apm_cli/​install/​phases/​resolve.py Supplies resolver dependencies and frozen state.
src/​apm_cli/​install/​context.py Stores frozen installation state.
src/​apm_cli/​deps/​immutable_requirements.py Tracks and compares immutable requirements; review findings remain around short-SHA validation, deleted locked tags, and mutable refs.
src/​apm_cli/​deps/​apm_resolver.py Admits requirements before winner selection.
scripts/​architecture_linter/​checks/​install_uninstall_and_resolution.py Adds ownership and routing checks.
scripts/​architecture_linter/​checks/​install_deployment_analyzers.py Registers the architecture rule.
packages/​apm-guide/​.apm/​skills/​apm-usage/​dependencies.md Updates pinning and equivalence guidance.
packages/​apm-guide/​.apm/​skills/​apm-usage/​commands.md Updates command guidance.
docs/​src/​content/​docs/​reference/​lockfile-spec.md Documents frozen transitive verification.
docs/​src/​content/​docs/​reference/​cli/​install.md Documents install failure behavior.
docs/​src/​content/​docs/​consumer/​manage-dependencies.md Documents conflicts and recovery.
.apm/​architecture/​owners/​install-deployment.json Registers the immutable-compatibility owner.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/apm_cli/deps/immutable_requirements.py Outdated
@sergio-sisternes-epam

Copy link
Copy Markdown
Collaborator

This pull request is linked to #3027, which already carries
status/accepted and a bounded maintainer scope record. Advisory
classification: ready for human review.

This is recommendation only, not merge or additional scope
approval. CODEOWNERS currently request review from
Sergio Sisternes (@sergio-sisternes-epam); this triage does not add or change review
requests.

The existing Copilot review thread on frozen short-SHA handling
remains unresolved in the conversation. A responsible maintainer
still needs to review the PR before merge.


Generated by autopilot-pr-triage-worker. This comment is AI-generated and may contain errors.

@sergio-sisternes-epam Sergio Sisternes (sergio-sisternes-epam) added triage/recommended Automated advice completed; not human scope approval. type/bug Something does not work as documented. area/lockfile Lockfile schema, per-file provenance, integrity hashes, drift detection. area/cli CLI command surface, flags, help text (cross-cutting). theme/security Secure by default. Content scanning, lockfile integrity, MCP trust boundaries. status/accepted Human scope approval; verify the issue's approval record and review contact before work. labels Sep 23, 2026
@danielmeppiel

Daniel Meppiel (danielmeppiel) commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: needs_rework

Immutable-requirements admission closes silent constraint loss, but frozen short pins and locked replay need corrections at feb026d.

panel-mode=full; personas=python-architect,test-coverage-expert,auth-expert,doc-writer,performance-expert,devx-ux-expert,supply-chain-security-expert,cli-logging-expert,apm-ceo

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The canonical owner and pre-hoisting admission are appropriate for #3027. Two reproduced failures need correction: matching short-SHA spelling skips frozen verification, and the registered architecture rule is missing from the expected inventory. CI is not green. The conformance job also needs executable evidence under existing requirements, not a waiver.

Preserve the established replay and reference-selection contracts. req-rs-015 requires non-update reuse of unchanged locked literals without discovery; the suggestion to always prefer current upstream tag content contradicts it. The existing resolver prefers branches for bare ambiguous names, so the new owner must not independently switch to tag precedence. Add retained-versus-refreshed and explicit-tag coverage instead. Never print raw transport exceptions: keep credential-safe guidance. The commit cache cannot prove branch/tag classification, so avoid broad cache redesign; local ref indexing and lock-first evidence are appropriate.

Align diagnostic chains with req-rs-010, add the concise upgrade/migration note, and retain a qualified CI-install mental model. The existing integration floor is met; an unrelated lifecycle framework expansion is unnecessary. All proven in-scope follow-ups will be folded into this PR under the approved driver plan. Sergio Sisternes (@sergio-sisternes-epam) retains review ownership; advisory findings do not replace human review.

Dissent. The logging reviewer interpreted req-rs-010 as semver-only; its reference to all req-rs-001 empty intersections supports the documentation reviewer's broader reading. The supply-chain suggestions to override locked replay and branch precedence are not adopted. The auth suggestion to print raw exception text is not safe.

Aligned with: Verify short immutable pins and preserve fail-closed constraint admission. Repair the architecture inventory and bind behavior to existing conformance requirements. Preserve locked-content equivalence and deterministic diagnostic chains. Keep actionable recovery and familiar qualified frozen-install guidance.

Panel summary

Persona B R N Takeaway
python-architect 0 1 1 New ImmutableRequirements canonical owner is well-structured with dual guardrails; frozen short-SHA early-return at L116 bypasses commit verification.
test-coverage-expert 1 1 1 Frozen short-SHA lockfile bypass has no regression trap; architecture rule inventory test fails; integration floor met for conflict abort.
auth-expert 0 1 0 Auth chain intact; new remote calls retain per-dependency AuthResolver routing. Wrapping transport errors drops actionable auth context.
doc-writer 0 3 0 Commit equivalence and remedies are clear, but frozen replay conflicts with the spec, diagnostic chains use the wrong format, and release guidance omits the behavior change.
performance-expert 0 1 1 Bounded extra git ls-remote per conflicting-tag repo bypasses TieredRefResolver; acceptable for safety fix, track for follow-up.
devx-ux-expert 0 1 2 Error messages are well-structured and actionable; frozen flag description drops npm-ci anchor while guide pages retain it; two format gaps.
supply-chain-security-expert 0 2 0 Two untested evidence-gap paths weaken the new immutable-requirements gate: lockfile commit substitution and branch-tag name collision.
cli-logging-expert 0 0 2 Conflict diagnostics are actionable and ASCII-safe; verification errors omit the other triggering chain and notation differs from the spec.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

  1. [test-coverage-expert] Verify frozen short pins with unit and install regression coverage. -- The reproduced matching-spelling shortcut accepts an incompatible locked commit.

  2. [test-coverage-expert] Add the immutable-requirements rule to the explicit architecture inventory. -- The exact inventory test currently fails; preserve its equality assertion.

  3. [doc-writer] Use admissible lock evidence before discovery and prove offline tag/SHA equivalence. -- req-rs-015 requires non-update replay of unchanged locked literals.

  4. [doc-writer] Add a concise Unreleased fix and migration note. -- Previously silent incompatible graphs now fail and users need the alignment remedy.

  5. [doc-writer] Align conflict chains with req-rs-010. -- Use ordered owner/repo@constraint entries separated by -> without changing manifest syntax.

Architecture

classDiagram
    direction TB
    class ImmutableRequirements {
        <<CanonicalOwner>>
        +add(node) void
        -_commit(req) str
        -_check_locked(req) void
        -_require_equal() void
    }
    class ReferenceResolver {
        <<Protocol>>
        +list_remote_refs(dep) Iterable
        +resolve_git_reference(dep) ResolvedReference
    }
    class ImmutableRequirementError {
        <<Exception>>
    }
    class Requirement {
        <<ValueObject>>
        +dependency DependencyReference
        +chain str
    }
    class APMDependencyResolver {
        <<Facade>>
        +resolve_dependencies(root) DependencyTree
        -_reference_resolver ReferenceResolver
        -_frozen bool
    }
    class InstallContext {
        <<DataClass>>
        +frozen bool
        +update_refs bool
    }
    class InstallService {
        <<Service>>
        +install(request)
    }
    class DependencyNode {
        +dependency_ref DependencyReference
        +parent DependencyNode
    }
    class DependencyTree {
        +resolution_errors list
        +is_valid() bool
    }
    class LockFile {
        +get_dependency(key) LockedDependency
    }
    ImmutableRequirements *-- ReferenceResolver : delegates
    ImmutableRequirements o-- LockFile : reads
    ImmutableRequirements ..> Requirement : creates
    ImmutableRequirements ..> DependencyNode : reads
    ImmutableRequirements ..> ImmutableRequirementError : raises
    APMDependencyResolver *-- ImmutableRequirements : owns
    APMDependencyResolver ..> DependencyTree : returns
    InstallService ..> APMDependencyResolver : configures
    InstallContext ..> APMDependencyResolver : configures
    note for ImmutableRequirements "Canonical Owner: commit-based edge compat before hoisting"
    note for ReferenceResolver "Strategy: decouples checks from downloader impl"
    class ImmutableRequirements:::touched
    class ReferenceResolver:::touched
    class ImmutableRequirementError:::touched
    class Requirement:::touched
    class APMDependencyResolver:::touched
    class InstallContext:::touched
    class InstallService:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A["apm install --frozen"] --> B["InstallService.install service.py"]
    B --> C["enforce_frozen structural check"]
    C --> D["run_install_pipeline pipeline.py frozen=True"]
    D --> E["InstallContext context.py frozen=True"]
    E --> F["APMDependencyResolver apm_resolver.py frozen=True"]
    F --> G["ImmutableRequirements init immutable_requirements.py"]
    G --> H{{"BFS level loop"}}
    H --> I["collect work_items"]
    I --> J["requirements.add node for each work_item"]
    J --> K{"dep.is_local or registry or semver?"}
    K -->|"skip"| L["_select_dependency_winners"]
    K -->|"immutable"| M{"first.ref != dep.ref?"}
    M -->|"identical"| N{"self._frozen?"}
    M -->|"different"| O["NET _commit list_remote_refs"]
    O -->|"commits match"| N
    O -->|"mismatch"| P["ImmutableRequirementError return tree"]
    N -->|"yes"| Q["_check_locked L116 is_full_revision_pin gate"]
    N -->|"no"| L
    Q -->|"verified"| L
    Q -->|"mismatch"| P
    L --> R["NET Phase B download"]
    R --> H
    style J fill:#fff3b0,stroke:#d47600
    style O fill:#fff3b0,stroke:#d47600
    style Q fill:#fff3b0,stroke:#d47600
    style P fill:#ffcccc,stroke:#cc0000
Loading

Recommendation

Fold the proven in-scope defects, replay coverage, diagnostic and release-note improvements into this PR, then observe exact-head CI. Preserve Sergio's existing review request and the requirement for human review. This advisory is not approval.


Full per-persona findings

python-architect

  • [recommended] _check_locked early-return bypasses commit verification for short SHA pins (7-39 hex chars) at src/apm_cli/deps/immutable_requirements.py:116
    is_full_revision_pin requires exactly 40 hex chars, but parse_git_reference classifies 7-40 hex chars as COMMIT. Matching short spelling takes the named-ref early return and never reaches _commit. A frozen lock can contain a different full commit. The non-frozen equivalent-short-SHA test does not cover this case.

    Suggested: Use the canonical parse_git_reference type check to distinguish commit pins from named refs.

    Proof (missing, unit): Frozen install rejects a short pin that does not match the locked commit.

  • [nit] Design patterns are well-chosen for the scope; no additional patterns needed
    Canonical Owner, ReferenceResolver Protocol strategy and frozen requirement Value Object are pragmatic. No further pattern is needed.

test-coverage-expert

  • [blocking] No regression trap for frozen short-SHA lockfile verification bypass in _check_locked at src/apm_cli/deps/immutable_requirements.py:116
    Existing frozen tests use only full pins; equivalent short pins are tested only without frozen. Probe found no frozen/short combination. Matching short spelling bypasses verification.

    Suggested: Add invalid and valid frozen short-pin cases through resolver and install.

    Proof (missing, unit): Frozen install refuses a short pin whose locked full commit is incompatible.

  • [recommended] Architecture rule inventory frozen set missing install-deployment-immutable-requirements at tests/unit/scripts/test_architecture_runner.py:744
    The registered-rule equality assertion fails on the new rule. Update the explicit inventory without weakening equality.

    Proof (failed, unit): Every architecture rule addition is acknowledged in the frozen inventory.

  • [nit] Install abort path covered at integration-with-fixtures but no lifecycle-state-machine proof at tests/integration/test_immutable_requirements_install.py:
    The real install suite proves lockfile preservation and no deployment after conflicts, meeting the required integration floor. A lifecycle state snapshot would additionally prove complete state preservation.

    Proof (passed, integration-with-fixtures): Conflict abort preserves lockfile and deployment state.

auth-expert

  • [recommended] Auth error context from build_error_context() silently dropped when ImmutableRequirementError is stringified into resolution_errors at src/apm_cli/deps/immutable_requirements.py:147
    The resolver raises rich auth hints. The new owner chains the cause, but the caller appends only str(exc), discarding that context. Users receive generic accessibility guidance.

    Suggested: Include the underlying error context in the public error without bypassing AuthResolver.

    Proof (passed, unit): Transport errors remain verification failures, but the test does not assert actionable auth context.

doc-writer

  • [recommended] Preserve network-free locked replay when checking tag/SHA equivalence at src/apm_cli/deps/immutable_requirements.py:163
    req-rs-015 forbids discovery for unchanged locked literals. A read-only admission probe with a locked release tag and equivalent full SHA failed because remote enumeration preceded lock substitution. Content fetching is distinct from ref discovery.

    Suggested: Use admissible lock evidence before discovery; test offline frozen tag/SHA equivalence and document proof versus content fetching.

    Proof (manual, unit): An already-proven equivalent pair depends on upstream availability.

  • [recommended] Render conflict chains using the normative diagnostic format at src/apm_cli/deps/immutable_requirements.py:83
    req-rs-001 clause (2) applies to incompatible immutable constraints; req-rs-010 specifies owner/repo@constraint separated by ->. A probe emitted #refs and > separators. Integration assertions omit complete ordered chains.

    Suggested: Use a diagnostic formatter separate from manifest syntax and assert full chains including parent constraints.

  • [recommended] Add an upgrade-facing changelog entry for newly rejected graphs at CHANGELOG.md:10
    Previously successful incompatible installs now fail, including frozen replay. Existing five doc edits explain recovery, but Unreleased omits an upgrade-facing note.

    Suggested: Add a concise Fixed entry explaining failure, equivalent-pin preservation and alignment remedy.

performance-expert

  • [recommended] ImmutableRequirements.list_remote_refs bypasses TieredRefResolver, adding one serialized git ls-remote RTT per repo with distinct non-SHA refs at src/apm_cli/deps/immutable_requirements.py:164
    Remote listing goes through existing downloader but not tiered commit cache; each conflicting repo costs one main-thread network listing before parallel downloads. Cache bounds calls to K distinct repositories. The tiered resolver does not preserve branch/tag classification.

    Suggested: Consult existing cache evidence where sufficient, or consider ref-list cache sharing.

  • [nit] Two O(R) linear scans over cached remote refs tuple per _resolve_commit call; a name-keyed dict would give O(1) at src/apm_cli/deps/immutable_requirements.py:169
    Branch any() and tag filtering scan all refs for each distinct symbolic ref. Index once by name/type while preserving ambiguity detection.

    Suggested: Build a name/type index at insertion.

devx-ux-expert

  • [recommended] --frozen flag description drops 'Mirrors npm ci' anchor; guide pages still cite npm-ci equivalence at docs/src/content/docs/reference/cli/install.md:33
    Reference removes the familiar npm-ci anchor while other pages retain approximate analogies. Preserve a qualified analogy rather than implying identical Git integrity behavior.

    Suggested: Describe it as comparable to npm ci with additional immutable-ref integrity checks.

  • [nit] Recovery guidance unconditionally says 'without --frozen' even in non-frozen install mode at src/apm_cli/deps/immutable_requirements.py:106
    Normal install errors include an irrelevant flag qualifier.

    Suggested: Only mention without --frozen when frozen is active.

  • [nit] Diagnostic chain format diverges from OpenAPM req-rs-010 at src/apm_cli/deps/immutable_requirements.py:82
    Current > and # mirror manifest notation but differ from normative -> and @.

    Suggested: Align formatting or explain deviation.

supply-chain-security-expert

  • [recommended] Lockfile substitution in _resolve_commit overrides remote-attested tag commit on default install path, creating circular-trust evidence gap at src/apm_cli/deps/immutable_requirements.py:183
    A moved tag is replaced with its old lock commit when update_refs is false, so it may compare equal to another old-commit tag despite differing upstream values.

    Suggested: Use current remote evidence outside frozen mode; add moved-tag test.

  • [recommended] Attacker-created branch sharing a tag name downgrades that reference to mutable at src/apm_cli/deps/immutable_requirements.py:169
    Bare names prefer a same-named branch before tags and are excluded from immutable comparison; existing collision regression uses full SHA pins rather than symbolic tags.

    Suggested: Prefer tag when both exist, or treat a name as mutable only if no tag exists; add symbolic collision coverage.

cli-logging-expert

  • [nit] Verification-failure diagnostic names only the failing chain, not both sides that triggered the check at src/apm_cli/deps/immutable_requirements.py:147
    add() knows both requirements, but _commit() only reports the one whose lookup fails. The user lacks the comparison context.

    Suggested: Include both chain operands while retaining verification-failure rather than proven-conflict wording.

  • [nit] Chain format > and # diverges from req-rs-010 -> and @ at src/apm_cli/deps/immutable_requirements.py:82
    Both chains are deterministic, but notation is inconsistent. This reviewer interprets req-rs-010 as semver-specific, in dissent with the doc reviewer.

    Suggested: Assess applicability before changing or documenting the format.

This panel is advisory. It does not block merge. The authorized driver uses delta review after fixes; no label or reviewer changes are requested.


Generated by autopilot-pr-review-worker. This comment is AI-generated and may contain errors.

Address Copilot inline 4073286694 and the full panel follow-ups on #3061. Validate short pins against locked commit prefixes without trusting a lock-seeded ref lookup, reuse unchanged lock evidence before discovery, and preserve existing branch precedence. Align conflict chains, bind executable conformance coverage, and acknowledge the new architecture guard in its frozen inventory.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address the delta panel's consumer-level replay evidence, explicit transport-field suppression assertion, and cold-cache documentation clarity. Keep production behavior unchanged and reuse the existing install matrix for normal and frozen replay.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_now

All in-scope follow-ups folded at 990156a; four terminal panelists report no remaining findings.

panel-mode=delta; personas=python-architect,test-coverage-expert,doc-writer,supply-chain-security-expert,apm-ceo

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The final test/docs delta closes the install-consumer replay evidence gap, makes the transport-content assertion unambiguous, and clarifies the cold-cache sentence. All four terminal panelists report no remaining findings. No production code changed in this final delta.

The final commit has 69 passing exact-head owner-consumer tests in 9.42s, clean architecture and Mode B checks, and deterministic functional-evidence verification across all three touched owners. Targeted validation of the committed content also passed 137 tests and 63 quality tests. Removing locked replay causes 15 failures, including install-consumer cases; exposing raw transport text causes one failure. The earlier eight guard mutations remain recorded as separate evidence, not claimed as rerun at the final commit.

The Copilot short-SHA concern is fixed, answered and resolved. The second and final classification round found no new claims. CI evidence is recorded separately below; this advisory is not approval or merge permission. Sergio's existing review ownership is preserved.

Aligned with: Validate every immutable constraint and keep raw transport error content out of diagnostics. Prove unchanged locked-content replay through actual install without ref discovery. Retain executable conformance and exact-head consumer evidence for every detected owner. Keep conflict recovery and cold-cache guidance precise.

Panel summary

Persona B R N Takeaway
python-architect 0 0 0 Offline matrix and conformance wrapper preserve canonical install routing; no new authority.
test-coverage-expert 0 0 0 Sixteen added locked-replay install cases and the real-install req-rs-015 wrapper close the evidence gap; mutations detect removed guards.
doc-writer 0 0 0 Cold-cache clause unambiguously refers to transitive validation, not local prefix proof.
supply-chain-security-expert 0 0 0 The diagnostic test now checks the actual injected field marker; raw-exception mutation fails. No production change.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Recommendation

No implementation follow-ups remain within the accepted scope, and exact-head CI is now observed green. Ready for maintainer review. This advisory does not replace Sergio's review, grant approval, or authorize merging.

Folded in this run

  • (copilot) Validate frozen short pins against full locked commit prefixes without trusting a seeded ref lookup. -- 93da78ad.
  • (panel) Use unchanged lock evidence before discovery; preserve replay versus refresh and bare-branch versus explicit-tag policy. -- 93da78ad.
  • (panel) Format complete conflict chains with @ and ->, include both operands on verification failure and suppress raw transport secrets. -- 93da78ad.
  • (panel) Index remote refs per repository while retaining ambiguous-tag checks. -- 93da78ad.
  • (panel) Repair frozen architecture-rule inventory and add executable conformance coverage with regenerated statements. -- 93da78ad.
  • (panel) Update dependency/frozen documentation, usage resources and the upgrade-facing changelog remedy. -- 93da78ad.
  • (panel) Prove unchanged named-ref replay through normal/frozen install without discovery and bind req-rs-015 to the consumer. -- 990156a8.
  • (panel) Strengthen the transport-field suppression assertion and clarify the cold-cache documentation antecedent. -- 990156a8.

Copilot signals reviewed

  • Copilot item 4073286694 -- LEGIT: Reproduced: matching short-SHA spelling bypasses commit verification in frozen mode; an incompatible lock returns a valid graph with zero short-ref resolutions. (resolved in 93da78ad).
  • Copilot item 5280071409 -- LEGIT: Overview repeats the proven short-SHA defect. Its deleted-tag and mutable-ref remarks are broader hypotheses requiring independent evidence, not additional proven defects. (resolved in 93da78ad).

Regression-trap evidence (mutation-break gate)

Eight original reversible guard mutations failed as expected, including removal of resolver admission (7 failures) and the static owner guard (1). The final replay mutation produced 15 failures including real install; raw transport-text exposure produced 1. All guards restored.

Lint contract

Canonical ruff check and format check, pylint R0801, auth boundary, YAML I/O, 2100-line and portable-relative-path guards all exit 0 with no diagnostics. Architecture entrypoint exit 0 at exact head.

CI

Observed all 20 reported checks successful, neutral or skipped at 990156a. CI; spec conformance. Earlier Shard 2 inventory and Spec conformance failures recovered; no merge performed.

Mergeability status

PR head SHA CEO stance iters folds defers Copilot rounds CI mergeable mergeStateStatus notes
#3061 990156a8 ship_now 2 8 0 2 green MERGEABLE BLOCKED human review still required; not merged

Convergence

Two implementation iterations, one full panel and two changed-surface delta passes; two Copilot rounds. No deferred work. Ready for maintainer review.


Full per-persona findings

python-architect

No findings.

test-coverage-expert

No findings.

doc-writer

No findings.

supply-chain-security-expert

No findings.

This panel is advisory. It does not block merge. No label, assignment or reviewer changes were made; no approval review, merge or auto-merge was performed.


Generated by autopilot-pr-merge-worker. This comment is AI-generated and may contain errors.

@danielmeppiel
Daniel Meppiel (danielmeppiel) merged commit c5fea9e into main Sep 24, 2026
20 checks passed
@danielmeppiel
Daniel Meppiel (danielmeppiel) deleted the danielmeppiel-issue-delivery-3027-f6b branch September 24, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli CLI command surface, flags, help text (cross-cutting). area/lockfile Lockfile schema, per-file provenance, integrity hashes, drift detection. status/accepted Human scope approval; verify the issue's approval record and review contact before work. theme/security Secure by default. Content scanning, lockfile integrity, MCP trust boundaries. triage/recommended Automated advice completed; not human scope approval. type/bug Something does not work as documented.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] incompatible immutable dependency refs are silently collapsed

3 participants