Skip to content

feat(deps): update revision pins#1738

Merged
danielmeppiel merged 9 commits into
mainfrom
danielmeppiel/1209-update-revision-pins
Jun 11, 2026
Merged

feat(deps): update revision pins#1738
danielmeppiel merged 9 commits into
mainfrom
danielmeppiel/1209-update-revision-pins

Conversation

@danielmeppiel

@danielmeppiel danielmeppiel commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

feat(deps): update revision pins

TL;DR

apm update now has an additive SHA-pin path: it resolves full 40-character revision pins to the latest annotated semver tag SHA, rewrites apm.yml with a # <tag> annotation, and then runs the existing install/update pipeline. apm outdated now reports SHA-pinned deps against that same annotated-tag target instead of treating them as unknown branch/commit refs. The capability-surface behavior-diff idea from the issue discussion is deliberately out of scope for this PR.

Note

Closes #1209. The board-approved fences are preserved: authoritative upstream ref fetch, branch/lightweight-tag rejection, atomic manifest writes, and no behavior-diff tooling.

Problem (WHY)

Approach (WHAT)

# Fix Guardrail
1 Add a revision-pin helper module that finds the highest stable annotated semver tag from git ls-remote refs. Reject branch refs and lightweight tags; skip prereleases by default.
2 Run that helper before the normal apm update install path only when direct manifest deps contain full-SHA refs. Strictly additive: non-SHA update flows continue through the existing plan callback.
3 Rewrite apm.yml through a sibling temp file and os.replace, then reload the manifest before install. Old pin survives failed writes; --dry-run never writes.
4 Record the resolved tag in apm.lock.yaml after the install pipeline writes the new SHA. The lockfile can explain which tag the SHA represents.
5 Teach apm outdated to compare full-SHA pins against the same annotated-tag candidate. Read-only reporting path; local, registry, and Artifactory skips stay unchanged.

Implementation (HOW)

File Intent
src/apm_cli/deps/revision_pins.py New pure helper surface for detecting full-SHA pins, selecting the latest annotated stable semver tag from tag-only remote ref fetches, rendering the revision-pin plan, and atomically rewriting manifest lines with # <tag>.
src/apm_cli/commands/update.py Adds the pre-update revision-pin pass, dry-run preview, single consent gate, manifest rewrite after consent, and post-install lockfile tag annotation. Existing update plan/install behavior remains the path for non-SHA deps.
src/apm_cli/commands/outdated.py Adds a read-only full-SHA branch that reports latest annotated-tag SHA candidates, while preserving existing tag, branch, marketplace, and registry checks.
src/apm_cli/deps/git_remote_ops.py + src/apm_cli/models/dependency/types.py Carries an annotated bit on remote tag refs by detecting peeled refs/tags/<tag>^{} lines from git ls-remote.
src/apm_cli/utils/yaml_io.py Adds a shared sibling-file atomic text replace helper for YAML-adjacent writes.
src/apm_cli/install/phases/lockfile.py Preserves existing resolved_tag metadata for unchanged SHA pins during normal lockfile regeneration.
tests/unit/deps/test_revision_pin_resolver.py Covers annotated-tag detection, branch/lightweight rejection, stable-over-prerelease selection, atomic manifest rewrite, and failed-write rollback.
tests/unit/commands/test_update_command.py Covers dry-run no-write behavior, --yes manifest rewrite, non-TTY abort, and lockfile tag annotation failure checks.
tests/unit/test_outdated_command.py Covers SHA pins reporting against annotated tags and refusing branch-only replacements.
Docs + packages/apm-guide/.apm/skills/apm-usage/* + CHANGELOG.md Documents the new update/outdated SHA-pin behavior and security model without editing README.

Diagrams

Legend: the dashed nodes are the new revision-pin path layered ahead of the existing update pipeline.

flowchart LR
    subgraph Detect[Detect]
        A["apm.yml full SHA pin"] --> B["git ls-remote upstream refs"]
        B --> C{"stable annotated semver tag?"}
    end
    subgraph Apply[Apply]
        C -->|"yes"| D["atomic apm.yml rewrite with tag comment"]
        C -->|"no"| E["fail closed"]
        D --> F["existing install update pipeline"]
        F --> G["record resolved_tag in lockfile"]
    end
    classDef new stroke-dasharray: 5 5;
    class B,C,D,G new;
Loading

Trade-offs

  • Annotated tags only. Chose fail-closed annotated-tag verification; rejected branch or lightweight-tag targets because they do not satisfy the board's anti-spoofing fence.
  • Stable releases only. Chose to skip prerelease tags by default to match existing git-semver resolver behavior; rejected implicit rc upgrades for SHA pins.
  • Line-preserving manifest rewrite. Chose a surgical text rewrite over re-serializing YAML so user formatting survives; rejected broad YAML dumping because it would churn apm.yml diffs.
  • No capability-surface diff. Left the behavior-diff review tool out of scope exactly as the board decision required.

Benefits

  1. SHA-pinned direct deps now produce an actionable apm update --dry-run plan instead of staying stale silently.
  2. apm.yml diffs show both the immutable SHA and the human-readable tag (# vX.Y.Z) for review.
  3. apm outdated reports full-SHA pins using the same annotated-tag rule as update, so preview and apply agree.
  4. Failed manifest writes leave the old SHA pin untouched; dry-run and non-TTY aborts do not mutate files.
  5. Existing non-SHA update, registry, marketplace, local, and Artifactory flows remain unchanged.

Validation

uv run --extra dev pytest tests/unit/deps/test_revision_pin_resolver.py tests/unit/test_outdated_command.py tests/unit/commands/test_update_command.py tests/unit/install/phases/test_lockfile_revision_pin_carry.py -q:

........................................................................................                         [100%]
88 passed in 1.52s
Full validation commands

uv run --extra dev pytest tests/unit tests/test_console.py -q:

completed with exit code 0

uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/ && ... && bash scripts/lint-auth-signals.sh:

All checks passed!
1239 files already formatted

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

[*] Rule A: get_bearer_provider boundary (any reference)
[*] Rule B: git ls-remote auth-delegated annotation
[+] auth-signal lint clean

Scenario Evidence

# Scenario (user promise) Principle(s) Test(s) proving it Type
1 apm update --dry-run previews a SHA/tag rewrite without changing apm.yml. Secure by default, DevX tests/unit/commands/test_update_command.py::TestUpdateDryRun::test_dry_run_renders_revision_pin_plan_without_writing_manifest unit
2 apm update --yes rewrites the SHA pin, annotates it with the tag, and records the tag in the lockfile. Governed by policy, DevX tests/unit/commands/test_update_command.py::TestUpdateAssumeYes::test_yes_updates_revision_pin_manifest_before_install
tests/unit/commands/test_update_command.py::TestRevisionPinLockfileAnnotation::test_annotates_lockfile_with_resolved_tag
tests/unit/install/phases/test_lockfile_revision_pin_carry.py::test_carry_forward_resolved_tag_for_unchanged_sha
unit
3 Branches, lightweight tags, and prerelease tags cannot replace a SHA pin. Secure by default, Governed by policy tests/unit/deps/test_revision_pin_resolver.py::test_latest_revision_pin_tag_ignores_branches_and_lightweight_tags
tests/unit/deps/test_revision_pin_resolver.py::test_latest_revision_pin_tag_ignores_prereleases_by_default
unit
4 A failed manifest write preserves the old SHA pin. Secure by default tests/unit/deps/test_revision_pin_resolver.py::test_apply_revision_pin_updates_keeps_old_pin_when_replace_fails unit
5 apm outdated reports SHA-pinned deps against the latest annotated release tag. DevX, Governed by policy tests/unit/test_outdated_command.py::TestOutdatedCommand::test_commit_ref_reports_latest_annotated_tag unit

How to test

  • Create an apm.yml dependency pinned to a full 40-character SHA from an older annotated release tag; run apm update --dry-run; expect a revision-pin plan and no file changes.
  • Re-run with apm update --yes; expect the manifest entry to change to the latest annotated tag's SHA with # <tag> appended.
  • Inspect apm.lock.yaml; expect resolved_commit to match the new SHA and resolved_tag to match the annotation.
  • Run apm outdated; expect the same latest tag/SHA target in the table.
  • Try a repo whose only newer ref is a branch or lightweight tag; expect the SHA pin path to fail closed instead of updating.

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

danielmeppiel and others added 2 commits June 11, 2026 00:29
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 10, 2026 22:37

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.

Pull request overview

Adds first-class handling for full 40-character SHA revision pins in the dependency workflow so that apm update / apm outdated can treat them as updateable (by comparing against the commit behind the latest annotated stable semver tag) while preserving the “no branch / no lightweight tag” fence.

Changes:

  • Introduces a revision-pin resolver + manifest rewriter that maps full SHA pins to the latest annotated semver tag’s commit and annotates apm.yml lines with # <tag>.
  • Extends git remote ref parsing to distinguish annotated vs lightweight tags (via peeled ^{} refs) and uses that in both update and outdated.
  • Updates lockfile regeneration to preserve existing resolved_tag metadata for unchanged SHA pins, plus adds unit coverage and docs updates.
Show a summary per file
File Description
src/apm_cli/deps/revision_pins.py New helper module for detecting full-SHA pins, selecting latest annotated stable semver tag, rendering a plan, and rewriting apm.yml atomically.
src/apm_cli/commands/update.py Adds pre-update revision-pin resolution / consent flow, applies manifest rewrite, and annotates lockfile with resolved_tag.
src/apm_cli/commands/outdated.py Adds SHA-pin check path to compare pinned SHA against latest annotated semver tag commit.
src/apm_cli/deps/git_remote_ops.py Detects annotated tags by tracking peeled refs/tags/<tag>^{} output and sets RemoteRef.annotated.
src/apm_cli/models/dependency/types.py Extends RemoteRef with an annotated: bool = False field.
src/apm_cli/utils/yaml_io.py Adds a sibling-temp-file + os.replace helper for atomic text replacement.
src/apm_cli/install/phases/lockfile.py Preserves resolved_tag across installs when SHA pins are unchanged.
tests/unit/deps/test_revision_pin_resolver.py New unit tests for peeled-tag detection, annotated-only selection, prerelease skipping, and atomic manifest rewrite rollback.
tests/unit/commands/test_update_command.py Adds unit tests for revision-pin dry-run behavior, --yes apply path, non-TTY abort, and lockfile tag annotation.
tests/unit/test_outdated_command.py Adds unit coverage for SHA pins reporting against latest annotated tag candidates.
packages/apm-guide/.apm/skills/apm-usage/dependencies.md Documents new SHA-pin update behavior and tag annotation.
packages/apm-guide/.apm/skills/apm-usage/commands.md Updates apm update / apm outdated command docs to mention SHA-pin semantics.
docs/src/content/docs/reference/cli/update.md Documents revision-pin behavior and --dry-run semantics.
docs/src/content/docs/reference/cli/outdated.md Documents SHA-pin comparison against annotated tags.
docs/src/content/docs/enterprise/security.md Extends security model docs to describe revision-pin update fences + lockfile tagging.
docs/src/content/docs/consumer/manage-dependencies.md Adds user-facing explanation + example of SHA pin rewrite and # <tag> annotation.
CHANGELOG.md Adds an Unreleased entry for the revision-pin support.

Copilot's findings

  • Files reviewed: 17/17 changed files
  • Comments generated: 8

Comment thread src/apm_cli/commands/update.py Outdated
Comment on lines +456 to +457
if dry_run and revision_pin_updates:
return
Comment thread src/apm_cli/commands/update.py Outdated
Comment on lines +470 to +495
@@ -351,6 +489,10 @@ def _plan_callback(plan: UpdatePlan) -> bool:
)
return False

if revision_pin_updates:
plan_state["proceeded"] = True
return True

Comment thread src/apm_cli/commands/update.py Outdated

def _resolve_and_maybe_apply_revision_pin_updates(
*,
all_declared_deps,
Comment thread src/apm_cli/commands/outdated.py Outdated
current_ref: str,
package_name: str,
package_basename: str,
remote_refs,
Comment thread src/apm_cli/deps/revision_pins.py Outdated

def resolve_revision_pin_updates(
dependencies: Iterable[DependencyReference],
downloader,
Comment thread src/apm_cli/deps/revision_pins.py Outdated
Comment on lines +200 to +202
def dependency_ref_from_locked(locked) -> DependencyReference:
"""Rebuild a DependencyReference suitable for authoritative upstream checks."""
return locked.to_dependency_ref()
Comment thread src/apm_cli/deps/revision_pins.py Outdated
Comment on lines +133 to +136
lines = ["[i] Revision pin updates for apm.yml", ""]
for update in ordered:
lines.append(f" [~] {update.display_name}")
lines.append(f" ref: {update.old_sha[:7]} -> {update.new_sha[:7]} (# {update.tag})")
Comment thread CHANGELOG.md Outdated

### Added

- `apm update` and `apm outdated` now understand full-SHA revision pins, resolving the latest annotated semver tag SHA and annotating updated manifest pins with the tag. (#1209)
Folds copilot-pull-request-reviewer follow-ups: --dry-run prints full plan, preserve consent gate, type hints, STATUS_SYMBOLS, changelog PR number.

Refs #1209

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

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_with_followups

Additive SHA-pin update path: resolves full revision pins to the latest ANNOTATED stable semver tag, rewrites apm.yml atomically with a # <tag> annotation, and teaches apm outdated the same target -- fail-closed on branches/lightweight tags.

cc @danielmeppiel @sergio-sisternes-epam -- a fresh advisory pass is ready for your review.

This PR closes a real security-toil gap (#1209): revision-pinned deps were invisible to the updater, widening the vuln-patch window. The panel signals converge strongly. The core supply-chain threat model is honored on every fence: annotated-tag-only selection (find_latest_annotated_tag accepts a ref only when the peeled refs/tags/<tag>^{} line set annotated=True, so a branch or lightweight tag named like a release can never masquerade as the target and the resolver raises RevisionPinResolutionError fail-closed); authoritative upstream (the outdated path now rebuilds the ref via dependency_ref_from_locked -> to_dependency_ref, preserving host/port/virtual-path/insecure metadata, instead of the lossy DependencyReference.parse(full_url) it replaced); and write-before-remove (write_yaml_text_atomic writes a sibling temp then os.replace, removing the temp and re-raising on failure so the old pin survives). Test coverage maps 1:1 onto each fence. CI is fully green across all 13 checks.

No blocking findings. The follow-ups below are non-blocking polish and cross-PR coordination.

Aligned with: Secure by default -- SHA pins only move to verified annotated tags, atomic writes never strand a half-edited manifest; Pragmatic as npm -- brings Dependabot-style "your pin is stale, here is the reviewed release" ergonomics to APM's git-first model.

Growth signal. "APM now keeps your SHA-pinned deps patchable without sacrificing pin immutability" is a crisp release-note and adoption story: it removes the one ergonomic penalty of choosing the most secure pin format.

Panel summary

Persona B R N Takeaway
Supply Chain Security 0 1 0 All three fences honored and tested; annotated-detection rests entirely on the peeled ^{} line (fail-closed, no downgrade risk).
Performance 0 1 0 No N+1 beyond existing per-dep model; update resolve + install pipeline each do one ls-remote, a redundant round-trip per pinned dep.
Python Architect 0 0 1 Clean pure-helper module with a Protocol downloader seam; _confirm_plan_application extraction removes prior duplication.
CLI Logging 0 0 0 ASCII status symbols, clear dry-run/plan rendering, consistent with conventions.
DevX UX 0 0 1 --dry-run/--yes/non-TTY gates correct and independent; two sequential confirm prompts in one run is a mild surprise.
Test Coverage 0 0 0 Every security fence has a regression trap: annotated-vs-lightweight/branch, prerelease skip, dry-run no-write, rollback, sibling-temp, lockfile annotation.
Doc Writer 0 0 1 update/outdated/security docs + apm-usage skills updated; no command-doc drift.
OSS Growth Hacker 0 0 0 Strong adoption narrative; amplify in release notes.
Auth Expert 0 0 0 Re-resolution reuses the existing AuthResolver + auth-delegated git ls-remote; no new host/token/credential selection.

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

Top 3 follow-ups

  1. [Supply Chain Security] Document/assert that annotated-tag classification depends solely on the peeled ^{} ref line from git ls-remote. -- A transport that omitted peeled refs would misclassify a genuine annotated tag as lightweight; this is fail-closed (rejects, never downgrades), so it is safe to defer, but worth an inline comment so the invariant is not lost.
  2. [Performance] Consider threading the already-resolved annotated-tag SHA from the update resolve pass into the install pipeline. -- Today the pinned dep is resolved once for the plan and re-resolved (another ls-remote) during install; harmless but a redundant authoritative round-trip per pinned dep.
  3. [Python Architect] Coordinate the lockfile.py merge with PR fix(deps): support multi-host dependency identity #1735 (Multi-host dependency support: lockfile identity, token resolution consistency, error clarity #773). -- Both touch install/phases/lockfile.py; whichever lands second should rebase and re-run the lockfile preservation tests to confirm _preserve_existing_revision_pin_tags still composes with the other change.

Architecture

classDiagram
    class RevisionPinUpdate {
        +str dep_key
        +str old_sha
        +str new_sha
        +str tag
        +str display_name
    }
    class AnnotatedTagCandidate {
        +str tag
        +str commit_sha
    }
    class RemoteRef {
        +str name
        +GitReferenceType ref_type
        +str commit_sha
        +bool annotated
    }
    class _RemoteRefDownloader {
        <<Protocol>>
        +list_remote_refs(dep_ref) Iterable~RemoteRef~
    }
    class revision_pins {
        <<module>>
        +is_full_revision_pin(ref) bool
        +find_latest_annotated_tag(refs, package_name) AnnotatedTagCandidate
        +resolve_revision_pin_updates(deps, downloader) list~RevisionPinUpdate~
        +render_revision_pin_update_plan(updates) str
        +apply_revision_pin_updates(path, updates) None
    }
    revision_pins ..> RemoteRef : reads annotated bit
    revision_pins ..> _RemoteRefDownloader : authoritative fetch
    revision_pins ..> AnnotatedTagCandidate : selects
    revision_pins ..> RevisionPinUpdate : produces
    class GitHubPackageDownloader
    GitHubPackageDownloader ..|> _RemoteRefDownloader
    class revision_pins:::new
    class RevisionPinUpdate:::new
    class AnnotatedTagCandidate:::new
    classDef new stroke-dasharray: 5 5;
Loading
flowchart TD
    A["apm update / apm outdated"] --> B{"direct dep is full 40-char SHA pin?"}
    B -->|"no"| Z["existing tag/branch/registry/marketplace path"]
    B -->|"yes"| C["dependency_ref_from_locked -> authoritative upstream"]
    C --> D["git ls-remote (auth-delegated)"]
    D --> E{"highest stable annotated semver tag?"}
    E -->|"none (branch / lightweight / prerelease only)"| F["fail closed: RevisionPinResolutionError"]
    E -->|"found"| G{"command?"}
    G -->|"outdated"| H["report tag (sha[:8]) + status"]
    G -->|"update --dry-run"| I["render plan, no write"]
    G -->|"update --yes / consent"| J["write-before-remove: sibling temp -> os.replace"]
    J --> K["reload manifest -> existing install pipeline"]
    K --> L["annotate resolved_tag in apm.lock.yaml"]
    class C,D,E,F,J,L new;
    classDef new stroke-dasharray: 5 5;
Loading
sequenceDiagram
    participant U as apm update
    participant R as revision_pins
    participant D as Downloader (authoritative)
    participant M as apm.yml
    participant L as apm.lock.yaml
    U->>R: resolve_revision_pin_updates(pins)
    R->>D: list_remote_refs(dep_ref)
    D-->>R: RemoteRef[] (annotated bit)
    R->>R: find_latest_annotated_tag (reject branch/lightweight/prerelease)
    R-->>U: RevisionPinUpdate[] or fail closed
    U->>M: apply (sibling temp -> os.replace)
    U->>L: annotate resolved_tag (after install writes SHA)
Loading

Recommendation

Ship with follow-ups. There are no blocking findings: the board-approved security fences are implemented exactly as specified and each has a dedicated regression test, CI is green, and docs are in sync. Recommended next action is to merge once the maintainer is satisfied, tracking the lockfile.py rebase coordination with PR #1735 as the highest-signal item so the second-to-land PR re-runs the lockfile preservation tests.


Full per-persona findings

Supply Chain Security

  • [recommended] Annotated-tag classification depends entirely on the peeled refs/tags/<tag>^{} line in git ls-remote output at src/apm_cli/deps/git_remote_ops.py.
    Only annotated tags emit a peeled ref, so annotated=True is a sound signal and the core anti-spoofing fence is correct. The residual risk is purely theoretical: a transport that suppressed peeled refs would misclassify a real annotated tag as lightweight. Because find_latest_annotated_tag then raises rather than accepting a lower-assurance ref, the failure mode is fail-closed (no SHA downgrade), so this is a documentation/invariant note, not a correctness bug.
    Suggested: Add an inline comment at the peeled-ref branch stating that annotated classification is load-bearing for the security fence.
    Proof (present): tests/unit/deps/test_revision_pin_resolver.py::test_parse_ls_remote_marks_only_peeled_tags_as_annotated -- proves: peeled refs mark annotated, lightweight do not [secure_by_default]
  • Authoritative-upstream fence verified: outdated now uses dependency_ref_from_locked (-> LockedDependency.to_dependency_ref) which carries host/port/virtual_path/is_insecure, replacing the lossy DependencyReference.parse(full_url). Branch/lightweight/prerelease rejection and write-before-remove are all covered.
    Proof (present): tests/unit/deps/test_revision_pin_resolver.py::test_latest_revision_pin_tag_ignores_branches_and_lightweight_tags -- proves: SHA pin never replaced by branch/lightweight [secure_by_default]

Performance

  • [recommended] Redundant authoritative round-trip per pinned dep during apm update.
    resolve_revision_pin_updates performs one list_remote_refs (one git ls-remote) per SHA-pinned dep to build the plan; the subsequent install pipeline re-resolves the same ref. For outdated the cost is one ls-remote per dep, identical to the existing tag/branch model -- no new N+1 and no tag-enumeration blowup (candidate selection is O(n log n) over already-fetched refs).
    Suggested: Optionally pass the resolved SHA forward to avoid the second ls-remote; low priority.

Python Architect

  • [nit] revision_pins.py is a clean, pure module: frozen dataclasses, a Protocol (_RemoteRefDownloader) decoupling it from GitHubPackageDownloader, and lazy imports to avoid cycles. The extraction of _confirm_plan_application in update.py removes the duplication that would otherwise exist between the revision-pin gate and the main plan gate. update.py grows ~150 lines but stays well under the 2450-line guardrail and the new helpers are cohesive and single-purpose.

CLI Logging

  • No findings. Plan output uses [i] and STATUS_SYMBOLS['update'], dry-run emits an explicit "no changes applied" line, and the outdated row renders tag (sha[:8]) -- all ASCII, all consistent with the console conventions.

DevX UX

  • [nit] A single apm update run can now prompt twice (once to apply the manifest pin rewrite, once for the main update plan).
    This is correct -- they are distinct mutations (manifest rewrite vs install) and test_revision_pin_updates_do_not_auto_confirm_main_plan proves the gates are independent -- but two sequential confirms may mildly surprise users. --yes and --dry-run collapse this cleanly; non-TTY aborts with an actionable message. No change required.

Test Coverage

  • No findings. Every security fence has a regression trap: annotated-vs-lightweight/branch rejection, prerelease skip, highest-semver selection, --dry-run no-write (manifest byte-identical), failed-write rollback + stale-temp cleanup, sibling-temp path assertion, lockfile tag annotation, and outdated reporting against the annotated tag. The dry-run test also asserts the standard plan still renders alongside the revision-pin plan.

Doc Writer

  • [nit] Docs are in sync: docs/.../reference/cli/update.md, cli/outdated.md, consumer/manage-dependencies.md, enterprise/security.md, plus packages/apm-guide/.apm/skills/apm-usage/{commands,dependencies}.md and a CHANGELOG entry. No README change (correctly gated). No command-doc drift detected.

Auth Expert

  • No findings. The pre-update pass instantiates AuthResolver() and GitHubPackageDownloader and re-resolves the remote, but reuses the existing auth-delegated git ls-remote path with no new host/token/credential/fallback selection logic. The auth-signal lint (Rule A/Rule B) passes. Token reuse is via the existing resolver; no leakage surface introduced.

This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.

Folds the two non-blocking apm-review-panel recommended follow-ups for
the additive revision-pin update path, both comment-only (no behavior
change; existing regression traps already cover the invariants).

[Supply Chain] Document that annotated-tag classification rests solely
on the peeled refs/tags/<tag>^{} ls-remote line: only annotated tags
emit a peeled ref, so branch and lightweight tags are rejected
fail-closed and can never masquerade as a SHA-pin update target.
Comments added at parse_ls_remote_output and find_latest_annotated_tag.

[Performance] Justify the redundant authoritative ls-remote round-trip
per pinned dep: the install pipeline must independently re-verify the
freshly-written pin against upstream rather than trust a SHA computed
earlier in the process. The duplicate fetch is a binding
authoritative-upstream security requirement, not an oversight.

Refs #1209

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

Copy link
Copy Markdown
Collaborator Author

Shepherd-driver: terminal advisory (PR #1738)

Drive-to-mergeable pass on the additive apm update / apm outdated
revision-pin resolver path. The full apm-review-panel already ran
(ship_with_followups, no blocking findings); this pass folds its
remaining non-blocking recommendations and confirms green CI. No merge
performed -- mergeable + green CI + folded follow-ups is terminal.

Folded in this run

  • (panel) [Supply Chain] Documented the annotated-tag classification
    security invariant: classification rests solely on the peeled
    refs/tags/<tag>^{} ls-remote line; only annotated tags emit it, so
    branches and lightweight tags are rejected fail-closed and can never
    masquerade as a SHA-pin update target. Inline comments added at
    parse_ls_remote_output and find_latest_annotated_tag -- resolved
    in 3cb028f.
  • (panel) [Performance] Justified the redundant authoritative
    ls-remote round-trip per pinned dep rather than dropping it: the
    install pipeline must independently re-verify the freshly-written
    pin against upstream instead of trusting a SHA computed earlier in
    the same process. Documented as a binding authoritative-upstream
    requirement at the resolve site in commands/update.py -- resolved
    in 3cb028f. The authoritative fetch is preserved.

Both items are comment-only; existing regression traps
(test_parse_ls_remote_marks_only_peeled_tags_as_annotated,
test_latest_revision_pin_tag_ignores_branches_and_lightweight_tags)
already cover the invariants, so no new test was added and no
mutation-break gate was required.

Copilot signals reviewed

The 8 copilot-pull-request-reviewer[bot] findings (dry-run plan
rendering, consent-gate preservation, parameter type hints x4,
STATUS_SYMBOLS reuse, CHANGELOG PR-number) were all classified LEGIT
and folded in the prior commit d23fdd3. No new Copilot inline
comments arrived on the latest diff.

Deferred (out-of-scope follow-ups)

The capability-surface behavior-diff idea from the issue discussion
remains deliberately out of scope (separable downstream tooling).

Lint contract

uv run --extra dev ruff check src/ tests/ and
uv run --extra dev ruff format --check src/ tests/ both silent;
pylint R0801 10.00/10; auth-signal lint clean.

CI

All 13 required checks green on 3cb028f (Lint, Build & Test Shard 1/2,
Coverage Combine, Analyze actions/python, Spec conformance, gate,
build, PR Binary Smoke, NOTICE Drift, APM Self-Check, CodeQL).
0 CI fix iterations.

Mergeability status

PR head SHA CEO stance iters folds defers Copilot rounds CI mergeable mergeStateStatus notes
#1738 3cb028f ship_with_followups 1 2 1 1 green MERGEABLE BLOCKED pending required review

Ready for maintainer review.

@danielmeppiel

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_with_followups

Adds auditable annotated-tag-only SHA-pin update resolver to apm update/outdated, closing the last gap between APM and Dependabot-class dependency hygiene for AI agent packages.

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

All eight panelists converge on a positive signal: the security contract (annotated-tag-only fence, fail-closed, authoritative upstream fetch) is sound, the architecture is cleanly modular (Protocol-based DI, frozen dataclasses), and the UX ergonomics (dry-run, outdated table, non-TTY guard) are consistent with existing CLI paths. No panelist raised a highest-severity finding.

The two substantive tensions are (1) the devx-ux concern about a two-prompt interactive flow creating a half-applied state versus the security visibility of the separate revision-pin gate, and (2) the performance note on sequential ls-remote RTTs versus the need to preserve per-dep auth isolation. I side with DevX on the first: a single unified plan with one confirmation is the stronger UX and does not weaken security. I side with security on the second: serial resolution is acceptable at current dep counts, and parallelism can be follow-up work.

Dissent. DevX and supply-chain/security implicitly disagree on whether the two-prompt flow is a feature or a footgun. Prefer a unified plan confirmation in follow-up: same visibility, less partial-state risk.

Aligned with: secure by default: annotated-tag-only fence with fail-closed rejection of branches and lightweight tags; governed by policy: resolved_tag records which annotated tag justified the SHA update; portable by manifest: updates flow through apm.yml; pragmatic as npm: dry-run/outdated/yes flag match package-manager expectations.

Growth signal. This unlocks a "Dependabot for agent dependencies" story: APM can keep SHA-pinned deps current against annotated tags with an auditable trail.

Panel summary

Persona B R N Takeaway
Python Architect 0 1 2 Clean module shape; consider a clearer downloader injection seam.
CLI Logging Expert 0 1 2 Plan output is consistent; fix plain-text SHA column alignment later.
DevX UX Expert 0 2 1 Dry-run/non-TTY are solid; two-prompt interactive flow is the main UX concern.
Supply Chain Security Expert 0 1 2 Annotated-tag-only fence is sound; temp-file permissions can be hardened.
OSS Growth Hacker 0 1 2 Strong SHA-pin hygiene story; lead release notes with user benefit.
Auth Expert 0 0 1 AuthResolver path is correct; no token bypass or direct env access.
Doc Writer 0 3 1 Docs are mostly accurate; lockfile resolved_tag reference needs broadening.
Test Coverage Expert 0 1 0 New surfaces are covered; add a carry-forward test for resolved_tag.
Performance Expert 0 2 1 Serial ls-remote cost is acceptable under the security contract.

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] Add a unit test for _preserve_existing_revision_pin_tags carry-forward -- missing coverage on a governed-by-policy surface could let a future refactor silently drop tag carry-forward and degrade apm outdated.
  2. [DevX UX Expert] Merge revision-pin consent into the existing single-plan confirmation prompt -- one plan and one confirmation avoids a half-applied manifest rewrite if the user declines the later install prompt.
  3. [Doc Writer] Broaden lockfile-spec.md resolved_tag description to cover SHA-pin usage -- the field is now written for full-SHA pins, not only semver constraints.
  4. [Supply Chain Security Expert] Harden write_yaml_text_atomic temp-file permissions to 0o600 before os.replace -- defense-in-depth for shared systems.
  5. [CLI Logging Expert] Truncate the current SHA in the apm outdated plain-text fallback -- the current full 40-char value overflows the 13-char column in pipe/CI output.

Architecture

classDiagram
    direction LR
    class _RemoteRefDownloader {
        <<Protocol>>
        +list_remote_refs(dep_ref) Iterable~RemoteRef~
    }
    class GitHubPackageDownloader {
        <<ConcreteStrategy>>
        +list_remote_refs(dep_ref) Iterable~RemoteRef~
    }
    class AuthResolver {
        <<Strategy>>
        +resolve_for(host) AuthContext
    }
    class RevisionPinUpdate {
        <<ValueObject>>
        +dep_key str
        +old_sha str
        +new_sha str
        +tag str
        +display_name str
    }
    class AnnotatedTagCandidate {
        <<ValueObject>>
        +tag str
        +commit_sha str
    }
    class RemoteRef {
        <<ValueObject>>
        +name str
        +ref_type GitReferenceType
        +commit_sha str
        +annotated bool
    }
    GitHubPackageDownloader ..|> _RemoteRefDownloader : satisfies
    GitHubPackageDownloader *-- AuthResolver : delegates auth
    revision_pins_module ..> _RemoteRefDownloader : accepts
    revision_pins_module ..> RemoteRef : filters
    revision_pins_module ..> AnnotatedTagCandidate : returns
    revision_pins_module ..> RevisionPinUpdate : returns
Loading
flowchart TD
    A[apm update CLI entry] --> B[_run_dep_update]
    B --> C[_resolve_and_maybe_apply_revision_pin_updates]
    C --> D{Any SHA-pinned deps?}
    D -- No --> E[Existing install pipeline]
    D -- Yes --> F[GitHubPackageDownloader.list_remote_refs]
    F --> G[find_latest_annotated_tag]
    G --> H{Annotated tag with newer SHA?}
    H -- No --> E
    H -- Yes --> I[render revision-pin plan]
    I --> J{--dry-run?}
    J -- Yes --> K[Exit no mutations]
    J -- No --> L[apply_revision_pin_updates]
    L --> M[write_yaml_text_atomic os.replace]
    M --> N[Reload manifest]
    N --> E
    E --> O[Install pipeline re-resolves]
    O --> P[Lockfile resolved_tag annotation]
Loading

Recommendation

The core security contract, architecture, and UX are sound across the panel, with no highest-severity findings. Ship this PR and immediately track follow-up issues for the resolved_tag carry-forward test, the two-prompt UX consolidation, and the lockfile-spec doc drift. These are real gaps, but they do not invalidate the feature or create user-facing breakage on merge.


Full per-persona findings

Python Architect

  • [recommended] Inline AuthResolver + downloader construction in _resolve_and_maybe_apply_revision_pin_updates couples command layer to auth/download internals at src/apm_cli/commands/update.py:121.
    The revision-pin module already defines a downloader Protocol; an optional downloader/factory parameter would keep runtime behavior while improving test seams.
    Suggested: Add an optional downloader or factory kwarg and construct only when absent.
  • [nit] getattr(ref, "annotated", False) bypasses type checking at src/apm_cli/deps/revision_pins.py:90.
    RemoteRef declares annotated; direct access is clearer.
    Suggested: Use if not ref.annotated:.
  • [nit] dependency_ref_from_locked is a one-line passthrough at src/apm_cli/deps/revision_pins.py:219.
    Either document the future seam or call locked.to_dependency_ref() directly.

CLI Logging Expert

  • [recommended] apm outdated plain-text fallback: full 40-char SHA overflows the 13-char Current column at src/apm_cli/commands/outdated.py:234.
    Pipe/CI output can lose table alignment.
    Suggested: Truncate current SHA to 8 chars like latest_display, or widen the fallback column.
  • [nit] render_revision_pin_update_plan uses hardcoded [i] instead of STATUS_SYMBOLS['info'] at src/apm_cli/deps/revision_pins.py:152.
    Symbol constants prevent drift.
  • [nit] Non-TTY revision-pin prompt constraint uses _rich_error at src/apm_cli/commands/update.py:163.
    This is arguably warning-level, though consistent with existing update behavior.

DevX UX Expert

  • [recommended] Interactive mode prompts twice in one invocation at src/apm_cli/commands/update.py:155.
    One plan and one confirmation better match package-manager expectations and avoid half-applied state.
    Suggested: Render revision-pin updates inside the existing plan callback and gate once.
  • [recommended] Plan output uses # v2.0.0 notation in display text at src/apm_cli/deps/revision_pins.py:163.
    The YAML comment marker is an implementation detail.
    Suggested: Render as (v2.0.0) in the plan.
  • [nit] outdated.md sample output uses ambiguous ellipsis for latest SHA at docs/src/content/docs/reference/cli/outdated.md:58.
    Use a concrete 8-char SHA to match the code path.

Supply Chain Security Expert

  • [recommended] write_yaml_text_atomic temp file uses default umask at src/apm_cli/utils/yaml_io.py:75.
    Restrictive temp-file permissions are better defense-in-depth for shared systems.
    Suggested: Use os.open(..., 0o600) plus os.fdopen, then os.replace.
  • [nit] SHA comparison normalization is inconsistent at src/apm_cli/deps/revision_pins.py:130.
    Normalize both old and new SHAs at the value boundary.
  • [nit] apply_revision_pin_updates accepts arbitrary manifest_path without a path guard at src/apm_cli/deps/revision_pins.py:155.
    Current call site is safe; this is future-proofing.

OSS Growth Hacker

  • [recommended] CHANGELOG entry buries the user-facing hook.
    Lead with the solved problem: keeping SHA-pinned dependencies current with annotated-tag auditability.
  • [nit] Manage-dependencies example truncates a SHA with ellipsis at docs/src/content/docs/consumer/manage-dependencies.md.
    A full dummy SHA reinforces the 40-character requirement.
  • [nit] Consider cross-linking the outdated reference to the security model at docs/src/content/docs/reference/cli/outdated.md.
    Enterprise readers will want the why behind annotated tags.

Auth Expert

  • [nit] Inline import of AuthResolver hides the auth dependency from static analysis at src/apm_cli/commands/update.py:103.
    Runtime usage is correct and no token bypass was found.

Doc Writer

  • [recommended] Lockfile reference still scopes resolved_tag to git-source semver only at docs/src/content/docs/reference/lockfile-spec.md:147.
    This PR writes it for full-SHA pins too.
    Suggested: Broaden the field description and track the OpenAPM spec note separately.
  • [recommended] security.md says branches, caches, and lightweight tags are rejected at docs/src/content/docs/enterprise/security.md:58.
    caches is not a resolver guardrail term.
    Suggested: Drop caches.
  • [recommended] outdated.md sample shows current as 8-char SHA, but code prints full current ref at docs/src/content/docs/reference/cli/outdated.md:58.
    Literal CLI examples should match code or mark abbreviations.
  • [nit] Illustrative SHAs use ellipsis where full-40-char examples would be clearer at docs/src/content/docs/consumer/manage-dependencies.md:192.

Test Coverage Expert

  • [recommended] No test exercises LockfileBuilder._preserve_existing_revision_pin_tags carry-forward at src/apm_cli/install/phases/lockfile.py:251.
    A future refactor could silently drop resolved_tag carry-forward for unchanged SHA pins.
    Suggested: Add a unit test that starts with an existing lockfile entry with resolved_tag, builds a same-SHA new entry, and asserts the tag survives.
    Proof (missing at): tests/unit/install/phases/test_lockfile_revision_pin_carry.py::test_carry_forward_resolved_tag_for_unchanged_sha -- proves: Plain apm install preserves the annotated-tag label on unchanged SHA-pinned deps so apm outdated reports accurate version info. [governed-by-policy,devx]
    assert dep.resolved_tag == 'v1.2.0'

Performance Expert

  • [recommended] Revision-pin resolution issues one serial ls-remote per SHA-pinned dep at src/apm_cli/deps/revision_pins.py:131.
    Typical dep counts make this acceptable, but it can dominate at scale.
    Suggested: Consider a bounded ThreadPoolExecutor follow-up keyed to existing parallel-download settings.
  • [recommended] Revision-pin resolver bypasses TieredRefResolver / PerRunRefCache, causing duplicate RTT on install re-resolve at src/apm_cli/commands/update.py:121.
    This is documented as a security property; the cost is worth recording.
  • [nit] list_remote_refs fetches branches even though the revision-pin path only needs tags at src/apm_cli/deps/revision_pins.py:131.
    A future list_remote_tags() path could reduce bandwidth.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

danielmeppiel and others added 2 commits June 11, 2026 15:54
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ce pin filter

Fold remaining apm-review-panel follow-ups for the revision-pin update path:
replace the stringly-typed plan_state dict with a typed _UpdateRunState
dataclass, extract a shared abbreviate_sha() helper (one SHA-display source
across outdated/update/revision_pins), move the revision-pin eligibility
filter into resolve_revision_pin_updates (drop the now-dead duplicate +
unused list_remote_refs protocol method), and report a pins-only success
line when no packages reinstall. Tests cover the new state object, the
helper, and the pins-only summary.

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

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_with_followups

Adds an additive, transport-minimal revision-pin update path that resolves latest annotated semver tag SHA -- 'Dependabot for AI packages' in one command.

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

Nine panelists converge on a clean additive feature with no security bypass, no auth regression, and solid test coverage (88 unit tests). The architectural concern from python-architect (in-place mutation of shared DependencyReference) is contained: the mutation happens after the consent gate on a path that terminates in manifest write, so downstream aliasing risk is bounded to a single call-site. The supply-chain-security finding (validate 40-char hex before manifest write) is the highest-signal hardening item -- it costs one line and closes a theoretical injection surface if an upstream ever returns malformed ls-remote output. The performance-expert's note on outdated fetching heads+tags when only tags are needed is a real but non-urgent inefficiency; it does not affect correctness or security.

The test-coverage-expert flags a missing integration-with-fixtures test for the full update flow on a revision-pinned dep. Because this touches the secure_by_default surface (SHA integrity of installed packages), the absence of an end-to-end integration test is a meaningful regression-trap gap. However, the 88 unit tests already exercise every branch individually, and the feature is additive (no existing behavior regressed). I weigh this as a strong follow-up, not a ship-stopper.

No panelist raised a finding at blocking severity. The feature is story-shaped, the CHANGELOG entry is sound, and the positioning value ('pin to SHA, update like npm') is immediate. Ship with follow-ups tracked.

Dissent. Minor tension between performance-expert (recommending heads+tags fetch narrowing in outdated) and supply-chain-security-expert (who did not flag the broader fetch as a risk). Both are right in their lanes; the narrowing is a perf optimization, not a security gap.

Aligned with: Portable by manifest: Revision pins and their annotated-tag comments live in apm.yml; the manifest remains the single source of truth for pinned state. Secure by default: Annotated-tag-only fence and authoritative-upstream fetch preserve supply-chain integrity; hex validation follow-up closes the last theoretical gap. Pragmatic as npm: One-command update with dry-run preview and consent gate matches the ergonomic bar users expect from mature package managers.

Growth signal. The oss-growth-hacker's 'Dependabot for AI packages' framing is high-signal. This is the first CLI feature that directly maps to a pain point enterprise teams already articulate in npm/pip terms. A release-beat mention positioning apm update as the answer to 'how do I keep my agent deps current without breaking prod?' will resonate in the enterprise-eval funnel and on social.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 2 Well-structured additive module with Protocol seam, frozen dataclasses, and atomic write. One in-place mutation is the main concern.
CLI Logging Expert 0 1 2 Solid output discipline; dry-run plan rendering and symbols correct. One summary-message gap when installs and revision pins both change.
DevX UX Expert 0 2 2 Solid dry-run/consent/non-TTY design; single prompt covers SHA rewrites and lockfile correctly. Two plan-clarity improvements recommended.
Supply Chain Security Expert 0 1 2 Annotated-tag-only fence and authoritative-upstream invariants are sound; recommend validating new_sha format before manifest write.
OSS Growth Hacker 0 1 2 Strong story-shaped feature; CHANGELOG is solid; docs example is discoverable. Recommend a release-beat mention and one docs tweak.
Auth Expert 0 0 2 No auth bypass or credential-leak risk; token resolution flows through AuthResolver correctly. Two minor polish nits only.
Doc Writer 0 2 1 Docs accurately track the new SHA-pin update path; minor single-source-of-truth and sample-fidelity polish only.
Test Coverage Expert 0 1 1 88 unit tests cover all new surfaces; recommend integration-with-fixtures for the full update flow in follow-up.
Performance Expert 0 1 2 Transport-minimal design; tags-only update path saves branch enumeration; no blocking issues.

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

Top 5 follow-ups

  1. [Supply Chain Security Expert] Validate new_sha is 40-char lowercase hex before writing to manifest -- one-line guard closes a theoretical injection surface from malformed ls-remote output on the secure-by-default surface.
  2. [Test Coverage Expert] Add integration-with-fixtures test for full apm update on a revision-pinned dep -- unit tests cover branches individually but no single test proves the end-to-end SHA-update promise survives refactoring.
  3. [Python Architect] Replace in-place DependencyReference mutation with a new frozen instance returned from the resolver -- eliminates aliasing risk if DependencyReference is ever shared across concurrent paths in future parallel-install work.
  4. [Performance Expert] Narrow outdated ls-remote to tags-only for SHA-pinned deps -- saves branch enumeration per pinned dep; pure efficiency gain with no correctness risk.
  5. [CLI Logging Expert] Include revision-pin count in success summary when installed_count > 0 -- users who update both deps and pins currently see only the install count.

Architecture

classDiagram
    direction LR
    class RemoteRefDownloader {
        <<Protocol>>
        +list_remote_tag_refs(dep_ref) Iterable~RemoteRef~
    }
    class GitHubPackageDownloader {
        <<ConcreteStrategy>>
        +list_remote_tag_refs(dep_ref) list~RemoteRef~
        +list_remote_refs(dep_ref) list~RemoteRef~
    }
    class GitReferenceResolver {
        <<Strategy>>
        +list_remote_tag_refs(dep_ref) list~RemoteRef~
        -_list_remote_refs(dep_ref, include_heads) list~RemoteRef~
    }
    class RevisionPinUpdate {
        <<ValueObject>>
        +dep_key str
        +old_sha str
        +new_sha str
        +tag str
    }
    class AnnotatedTagCandidate {
        <<ValueObject>>
        +tag str
        +commit_sha str
    }
    class RemoteRef {
        <<ValueObject>>
        +name str
        +ref_type GitReferenceType
        +commit_sha str
        +annotated bool
    }
    class DependencyReference {
        <<Entity>>
        +reference str
        +get_unique_key() str
    }
    class _UpdateRunState {
        <<MutableState>>
        +plan UpdatePlan
        +proceeded bool
        +revision_pins_applied bool
    }
    note for RemoteRefDownloader "Protocol seam: update/outdated\ncoupled to tag resolution only"
    note for RemoteRef "annotated field is the\nanti-spoofing security fence"
    GitHubPackageDownloader ..|> RemoteRefDownloader : structural subtype
    GitHubPackageDownloader *-- GitReferenceResolver : delegates
    GitReferenceResolver ..> RemoteRef : returns
    class RevisionPinUpdate:::touched
    class AnnotatedTagCandidate:::touched
    class RemoteRefDownloader:::touched
    class _UpdateRunState:::touched
    class RemoteRef:::touched
    class GitReferenceResolver:::touched
    class GitHubPackageDownloader:::touched
    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A["apm update [PACKAGES]"] --> B["resolve_requested_packages\nsrc/apm_cli/commands/update.py"]
    B --> C["_resolve_and_maybe_apply_revision_pin_updates"]
    C --> D["[NET] resolve_revision_pin_updates\nThreadPoolExecutor fan-out"]
    D --> E["[NET] GitHubPackageDownloader.list_remote_tag_refs\nls-remote --tags"]
    E --> F["parse_ls_remote_output\ngit_remote_ops.py\nannotated detection via ^{}"]
    F --> G["find_latest_annotated_tag\nrejects non-annotated fail-closed"]
    G --> H{"SHA changed?"}
    H -- no --> I["skip dep"]
    H -- yes --> J["RevisionPinUpdate built"]
    J --> K["dep_ref.reference = new_sha\nMUTATION in-place"]
    K --> L["_plan_callback renders plan + revision_pin plan"]
    L --> M{"dry_run?"}
    M -- yes --> N["print plan, exit 0"]
    M -- no --> O["_confirm_plan_application"]
    O --> P["[FS] apply_revision_pin_updates\nwrite_yaml_text_atomic\nos.replace apm.yml"]
    P --> Q["clear_apm_yml_cache"]
    Q --> R["[NET] _install_apm_dependencies\nfull install pipeline"]
    R --> S["[LOCK] _annotate_lockfile_revision_tags\nlockfile.save()"]
    S --> T["_rich_success"]
Loading
sequenceDiagram
    participant User
    participant UpdateCmd as update.py
    participant Resolver as revision_pins.py
    participant Downloader as GitHubPackageDownloader
    participant GitRemote as git ls-remote
    participant Manifest as apm.yml
    participant Pipeline as install pipeline
    participant Lockfile as apm.lock.yaml
    User->>UpdateCmd: apm update
    UpdateCmd->>Resolver: resolve_revision_pin_updates(deps)
    Resolver->>Downloader: list_remote_tag_refs(dep)
    Downloader->>GitRemote: ls-remote --tags
    GitRemote-->>Downloader: refs + peeled ^{}
    Downloader-->>Resolver: RemoteRef[annotated=true/false]
    Resolver->>Resolver: find_latest_annotated_tag (reject non-annotated)
    Resolver-->>UpdateCmd: RevisionPinUpdate[]
    UpdateCmd->>UpdateCmd: mutate dep_ref.reference
    UpdateCmd->>User: render plan + confirm?
    User-->>UpdateCmd: yes
    UpdateCmd->>Manifest: write_yaml_text_atomic(new SHA + # tag)
    UpdateCmd->>Pipeline: _install_apm_dependencies
    Pipeline-->>UpdateCmd: result
    UpdateCmd->>Lockfile: _annotate_lockfile_revision_tags
    UpdateCmd->>User: Updated N revision pins
Loading

Recommendation

All nine panelists signal ship-ready with no blocking findings. The highest-signal post-merge item is the one-line SHA hex validation in revision_pins.py (supply-chain hardening); track as a fast-follow issue. The integration test gap is the second priority. Maintainer can merge on green CI with confidence.


Full per-persona findings

Python Architect

  • [recommended] In-place mutation of shared DependencyReference objects couples resolution phase to install pipeline implicitly at src/apm_cli/commands/update.py:148
    In _resolve_and_maybe_apply_revision_pin_updates, dep_ref.reference = update.new_sha mutates objects returned from apm_package.get_apm_dependencies(). If cached or reused elsewhere, this hidden side-effect becomes a fault surface. Prefer building copies or documenting the mutation contract.
    Suggested: Clone the dep_ref before mutation, or add a comment documenting that the install pipeline reads .reference from these objects and the cache is cleared below.
  • [recommended] RevisionPinResolutionError is swallowed differently in update vs outdated at src/apm_cli/commands/update.py:137
    Update exits while outdated reports unknown, which is reasonable UX. The generic except Exception in update catches network errors and programming bugs identically; catching specific operational exceptions would preserve diagnostics.
  • [nit] _UpdateRunState is a mutable dataclass but has no slots at src/apm_cli/commands/update.py:86
    Minor consistency note because other PR value objects are frozen.
  • [nit] write_yaml_text_atomic could live in a more general file_ops module at src/apm_cli/utils/yaml_io.py:59
    The helper writes pre-rendered text atomically and is not YAML-specific, but current placement is acceptable as the only user.

CLI Logging Expert

  • [recommended] Success summary swallows revision-pin count when installed_count > 0 at src/apm_cli/commands/update.py:556
    When both normal installs and revision pins change, only Updated N APM dependencies. prints. The user never sees that revision pins also moved.
    Suggested: Emit a combined success message when both counts are non-zero.
  • [nit] render_revision_pin_update_plan terminology differs from the success message at src/apm_cli/deps/revision_pins.py:213
    The plan uses update/updates while success uses pin/pins. Consistent terminology would improve coherence.
  • [nit] RevisionPinResolutionError path should offer verbose diagnostics at src/apm_cli/commands/update.py:141
    Other command error paths suggest --verbose; this one could too.

DevX UX Expert

  • [recommended] dry-run still hits the network for revision-pin resolution without telling the user at src/apm_cli/commands/update.py:122
    The resolver runs before the dry-run branch. For proxies or air-gapped CI this latency is surprising; at minimum make the upstream check visible.
    Suggested: Emit a user-visible info line under dry-run such as Checking upstream for revision-pin freshness....
  • [recommended] Plan output mixes two plan blocks without a combined summary line at src/apm_cli/commands/update.py:496
    When both revision-pin and standard dependency changes exist, the user sees two plan blocks followed by one prompt but no total scope line.
    Suggested: Emit a totals line when both are non-empty.
  • [nit] Source label git annotated tags is inconsistent with existing git tags label at src/apm_cli/commands/outdated.py:230
    Consider carrying the annotated distinction in prose or latest display instead of widening the source label.
  • [nit] render_revision_pin_update_plan accesses STATUS_SYMBOLS keys directly at src/apm_cli/deps/revision_pins.py:182
    Defensive .get() fallback would avoid a future KeyError if symbols change.

Supply Chain Security Expert

  • [recommended] new_sha written to manifest is not validated as a 40-char hex SHA before write at src/apm_cli/deps/revision_pins.py:146
    The old SHA is validated but the replacement SHA from ls-remote is written directly. Git normally emits valid SHAs, but a fail-closed defense-in-depth guard would protect future refactors or malformed remotes.
    Suggested: Validate latest.commit_sha with the same full-SHA predicate before constructing RevisionPinUpdate.
  • [nit] Tag name written to YAML comment is not sanitized for control chars at src/apm_cli/deps/revision_pins.py:241
    Git refname rules make this low risk, but a one-line sanitizer would be defense-in-depth.
  • [nit] Atomic temp file permissions do not preserve original file permissions at src/apm_cli/utils/yaml_io.py:79
    Replacing with a 0600 temp file can narrow permissions on shared manifests; low risk for user-owned apm.yml.

OSS Growth Hacker

  • [recommended] manage-dependencies.md example uses a fabricated SHA with no surrounding narrative hook at docs/src/content/docs/consumer/manage-dependencies.md
    A single sentence comparing the behavior to Dependabot/Renovate for SHA-pinned deps would give readers a familiar mental model.
  • [nit] CHANGELOG entry could front-load the user benefit over the mechanism at CHANGELOG.md
    Users scan for outcome first; the entry can be release-note ready with a benefit-led sentence.
  • [nit] update.md docs do not surface --dry-run in the first example block at docs/src/content/docs/reference/cli/update.md
    Dry-run is the zero-risk entry point and could appear before live update.

Auth Expert

  • [nit] GitHubDownloader update path does not accept an explicit AuthResolver via DI at src/apm_cli/deps/github_downloader.py
    The base-class wiring correctly goes through AuthResolver today; explicit constructor injection would make the seam easier to test.
  • [nit] No inline comment explaining why the SHA-pin comparison uses full 40-char hex at src/apm_cli/deps/github_downloader.py
    A short comment would help future maintainers avoid short-prefix matching.

Doc Writer

  • [recommended] manage-dependencies.md restates the SHA-pin rewrite mechanic without cross-linking the authoritative apm update page at docs/src/content/docs/consumer/manage-dependencies.md:197
    The consumer page duplicates update.md mechanics and can drift. Keep the example but link to the update reference as the source of truth.
  • [recommended] outdated.md points readers to enterprise/security for annotated-tag update rules rather than the authoritative update page at docs/src/content/docs/reference/cli/outdated.md:73
    The reader wants rewrite mechanics; linking directly to ../update/ removes an extra hop.
  • [nit] outdated.md sample table is slightly misaligned after widening the Latest column at docs/src/content/docs/reference/cli/outdated.md:92
    Cosmetic sample-fidelity issue only.

Test Coverage Expert

  • [recommended] No integration-with-fixtures test exercises apm update with a revision-pinned dep end-to-end at tests/integration/
    The CLI command surface and install pipeline floors call for fixture integration coverage. Unit tests prove branches individually, but no single test proves resolve -> plan -> manifest rewrite -> install -> lockfile annotate together.
    Suggested: Add a fixture-based integration test using a local bare git repo with annotated tags.
    Proof (missing at): tests/integration/test_update_revision_pins.py::test_update_rewrites_sha_pin_to_latest_annotated_tag_e2e -- proves: apm update replaces a SHA pin with the latest annotated tag and records the tag in the lockfile end-to-end [secure-by-default,devx]
    assert f'#{new_sha} # v2.0.0' in manifest.read_text(); assert lockfile.get_dependency(key).resolved_tag == 'v2.0.0'
  • [nit] Existing test_list_remote_refs.py does not assert the new annotated field on RemoteRef at tests/unit/test_list_remote_refs.py
    The new revision-pin resolver test already covers this invariant, but adding the assertion to the older file would improve local test discoverability.
    Proof (passed): tests/unit/deps/test_revision_pin_resolver.py::test_parse_ls_remote_marks_only_peeled_tags_as_annotated -- proves: Only peeled ^{} refs produce annotated=True; branches and lightweight tags are rejected by the revision-pin resolver [secure-by-default]
    assert annotated.annotated is True; assert lightweight.annotated is False

Performance Expert

  • [recommended] outdated fetches heads+tags for SHA-pinned deps that only need tags at src/apm_cli/commands/outdated.py:279
    _check_revision_pin_ref returns early for full-SHA pins and never consumes branch heads. Use tags-only fetch for pinned deps.
    Suggested: Choose downloader.list_remote_tag_refs(dep_ref) when is_full_revision_pin(current_ref).
  • [nit] ThreadPoolExecutor max_workers default could inherit CLI-level parallel_downloads in future users at src/apm_cli/deps/revision_pins.py:120
    Current update path wires parallel_downloads correctly; this is future-proofing only.
  • [nit] find_latest_annotated_tag materializes and sorts candidates instead of max() at src/apm_cli/deps/revision_pins.py:110
    O(n log n) is fine for normal tag counts, but max() would be simpler.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

danielmeppiel and others added 3 commits June 11, 2026 16:44
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel

Copy link
Copy Markdown
Collaborator Author

APM Review Panel: ship_now

Current head folds the revision-pin follow-ups into a tight additive update/outdated path: annotated-tag-only SHA resolution, atomic manifest rewrite, lockfile tag annotation, dry-run visibility, and regression tests.

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

The final pass finds the PR ship-ready. The prior security, performance, logging, DevX, architecture, docs, and coverage follow-ups were folded: remote SHAs are validated before manifest write, tag comments reject control characters, apm outdated uses tags-only remote fetches for SHA pins, dry-run and success output name the actual work, update resolution stages changes on an owned APMPackage copy, and the docs now point to apm update as the single source of truth. Local targeted tests and the CI-mirror lint chain are green.

Aligned with: Secure by default: SHA pins only move to annotated semver tags from authoritative upstream and malformed refs fail closed. Portable by manifest: apm.yml remains the human-reviewed source of truth with the tag annotation beside the immutable SHA. Pragmatic as npm: dry-run, one consent gate, and apm outdated make the workflow familiar.

Growth signal. This is a strong 'Dependabot-style review for AI packages' release beat: users keep SHA integrity while getting normal package-manager freshness checks.

Panel summary

Persona B R N Takeaway
Python Architect 0 0 0 Clean additive module with Protocol seam, frozen update records, staged package copy, and atomic manifest write.
CLI Logging Expert 0 0 0 Output now surfaces upstream checking, combined totals, and accurate success/no-op outcomes.
DevX UX Expert 0 0 0 Dry-run, consent, non-TTY, and outdated/reporting surfaces match package-manager expectations.
Supply Chain Security Expert 0 0 0 Annotated-tag-only, authoritative-upstream, full-SHA validation, and control-char rejection preserve the security fence.
OSS Growth Hacker 0 0 0 Docs and changelog now carry the adoption hook without over-expanding scope.
Auth Expert 0 0 0 Remote tag checks continue through AuthResolver-backed downloader paths; no token leakage concern.
Doc Writer 0 0 0 Docs are accurate, concise, cross-linked, and README remains intentionally untouched.
Test Coverage Expert 0 0 0 Targeted tests cover resolver, manifest safety, CLI dry-run/consent/output, outdated tags-only behavior, and lockfile annotation.
Performance Expert 0 0 0 Tags-only ref discovery and bounded parallelism keep the additive network cost appropriate.

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

Architecture

flowchart TD
    A["apm update"] --> B["resolve SHA-pinned deps"]
    B --> C["ls-remote --tags via AuthResolver-backed downloader"]
    C --> D{"latest stable annotated semver tag?"}
    D -- no --> E["fail closed"]
    D -- yes --> F["validate 40-char SHA"]
    F --> G["stage refs on copied APMPackage"]
    G --> H["render dry-run / consent plan"]
    H --> I{"apply?"}
    I -- no --> J["no manifest write"]
    I -- yes --> K["atomic apm.yml rewrite with tag comment"]
    K --> L["install pipeline re-resolves authoritative upstream"]
    L --> M["record resolved_tag in lockfile"]
Loading

Recommendation

Ship now. No in-scope follow-ups remain from the final panel pass; remaining queue state is CI/protection only and should stay a human merge decision.


Full per-persona findings

No findings.

This panel is advisory. It does not block merge. Re-apply the panel-review label after addressing feedback to re-run.

@danielmeppiel danielmeppiel merged commit 3ee0055 into main Jun 11, 2026
15 checks passed
@danielmeppiel danielmeppiel deleted the danielmeppiel/1209-update-revision-pins branch June 11, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Update revision-pins for external dependencies

2 participants