[Ching Wei Kang (WilliamK112)]

Summary

• Extract the registered marketplace ref fallback into one co-located pure helper in marketplace/resolver.py.
• Reuse that helper in the existing structured-reference and canonical-string routes.
• Preserve all current behavior: explicit install and plugin refs keep precedence, main/HEAD stay implicit, dict sources are not overwritten, and later raw-ref/semver processing remains with its current owner.

Closes #1917

Human scope approval and review contact: #1917 (comment)

Scope alignment

This PR implements only the approved behavior-preserving extraction. It does not change default-branch semantics, add master, add network lookups, alter ref/semver precedence, add source support, or restructure the wider resolver.

Current-main reconciliation

• Rebased the single refactor commit onto current main at 29200432.
• Preserved the newer explicit-path decision and dotted bundle-directory fallback added on main.
• Kept the PR limited to src/apm_cli/marketplace/resolver.py.

Validation

• uv run --frozen --extra dev pytest tests/unit/marketplace/test_marketplace_resolver.py -q -n0 — 143 passed.
• Related marketplace and ref-resolution suites — 75 passed.
• uv run --frozen --extra dev ruff check src/apm_cli/marketplace/resolver.py — passed.
• uv run --frozen --extra dev ruff format --check src/apm_cli/marketplace/resolver.py — passed.
• BASE_REF=upstream/main bash tests/spec_conformance/mode_b_detector.sh — passed.
• git diff --check upstream/main...HEAD — passed.

[Copilot]

Pull request overview

This PR refactors marketplace ref propagation by extracting the registered-ref selection logic into a single helper in marketplace/resolver.py, then reusing it across both the GitLab structured DependencyReference path and the GitHub-family #ref suffix path while preserving the existing precedence/guard rules from #1917.

Changes:

• Introduces _select_marketplace_plugin_ref(...) to centralize ref selection precedence (version_spec > plugin ref > marketplace registered ref, excluding implicit main/HEAD).
• Replaces duplicated inline ref-selection logic in the GitLab in-marketplace dependency reference path with the new helper.
• Replaces the GitHub-family canonical #ref propagation block to use the helper and preserve non-overwrite behavior.

[Ching Wei Kang (WilliamK112)]

Rebased the single refactor commit onto the rewritten current main history and preserved the newer auth-scheme handling while resolving the helper conflict. Validation is green: 200 marketplace/ref tests pass; Ruff, the Mode B gate, and git diff --check are clean.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

[Sergio Sisternes (sergio-sisternes-epam)]

APM Review Panel: ship_now

Clean internal refactor with full panel consensus -- ship as-is, nits are optional polish.

cc Ching Wei Kang (@WilliamK112) Daniel Meppiel (@danielmeppiel) Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

All eight panelists converge: this is a textbook extract-function refactor with no user-visible behavior change, no security regression, and no performance impact. The priority chain (version_spec > plugin_ref > source.ref excluding implicit refs > empty) is preserved verbatim from the inlined logic it replaces. No disagreements to arbitrate.

The four nits cluster into two themes: (a) sentinel ergonomics (empty-string vs None, docstring clarity) and (b) coverage completeness (master in the implicit set, HEAD parametrize for GitLab path). Neither theme touches a user contract or a security boundary today. The test-coverage finding carries passed evidence confirming the HEAD-exclusion invariant holds on the GitHub path; the GitLab path shares the same helper, so the gap is symmetric-paranoia rather than a live regression risk.

Strategically this PR is low-risk, high-hygiene: it makes the ref-selection contract a named, testable unit rather than duplicated inline logic, which improves APM maintainability for the next contributor who touches marketplace resolution.

Aligned with: Secure by default (implicit-ref exclusion invariant preserved; test evidence confirms HEAD not propagated); Portability by manifest (ref-selection logic is manifest-derived; no regression).

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	1	Clean extraction that consolidates duplicated ref-selection logic into a single canonical owner. The priority chain is clear and the security-adjacent invariants are preserved. No architectural concerns.
CLI Logging Expert	0	0	0	No CLI output or logging surface touched; pure internal refactor. Ship.
DevX UX Expert	0	0	1	No user-visible behavior change; internal refactor preserves all ref-selection contracts. Ship.
Supply Chain Security Expert	0	0	1	Ref-selection precedence is preserved and security-adjacent invariants hold. GitHub-family path condition change is semantically equivalent. Consider adding master to implicit refs set.
OSS Growth Hacker	0	0	1	Pure internal refactor -- no conversion-surface impact. Docstring link to #1917 would help drive-by contributors.
Test Coverage Expert	0	0	1	All priority branches of the extracted helper are covered through public-API tests; no blocking gaps.
Performance Expert	0	0	0	Pure extract-function refactor with zero performance impact. O(1) set lookup on 2-element set. No I/O, no loops. Ship.

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

Top 3 follow-ups

1. [Test Coverage Expert] Add HEAD parametrize for GitLab path to mirror GitHub coverage -- _IMPLICIT_MARKETPLACE_REFS contains both main and HEAD, but the GitLab path only has a main-not-propagated test. A symmetric parametrize entry at tests/unit/marketplace/test_marketplace_resolver.py guards against future divergence if paths ever separate from the shared helper.
2. [Supply Chain Security Expert] Add "master" to _IMPLICIT_MARKETPLACE_REFS -- repos still using master as default branch would get a misleading #master suffix appended. Low urgency since the previous code also omitted it, but the named constant is now the canonical place to document the complete implicit-ref set.
3. [OSS Growth Hacker] Reference #1917 in the helper docstring for contributor context -- the extraction removed 14 lines of inline commentary explaining why implicit refs are excluded; a one-liner docstring link to #1917 preserves discoverability for drive-by contributors.

Architecture

classDiagram
    direction LR
    class MarketplaceSource {
      <<ValueObject>>
      +url str
      +ref str
      +org str
    }
    class resolve_marketplace_plugin {
      <<Function>>
      +plugin Plugin
      +source MarketplaceSource
      +version_spec str
    }
    class _select_marketplace_plugin_ref {
      <<Pure>>
      +source MarketplaceSource
      +version_spec str
      +plugin_ref str
      returns str
    }
    resolve_marketplace_plugin ..> _select_marketplace_plugin_ref : delegates ref choice
    _select_marketplace_plugin_ref ..> MarketplaceSource : reads .ref
    class _select_marketplace_plugin_ref:::touched
    class resolve_marketplace_plugin:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A[resolve_marketplace_plugin] --> B{in_repo_path found?}
    B -->|Yes| C["_select_marketplace_plugin_ref(source, version_spec, path_ref)"]
    C --> D[_gitlab_in_marketplace_dependency_reference]
    B -->|No| E[resolve_plugin_source builds canonical]
    E --> F{dep_ref is None AND str source AND no hash}
    F -->|Yes| G["_select_marketplace_plugin_ref(source, None, None)"]
    G --> H{canonical_ref truthy?}
    H -->|Yes| I["canonical = canonical + hash + ref"]
    H -->|No| J[canonical unchanged]
    F -->|No| J

Loading

Recommendation

Ship. All nits are optional polish. The three follow-ups above are trivial and can be addressed in a follow-up commit or at the author's discretion -- none of them represent a live regression risk or a user-contract gap.

---

Full per-persona findings

Python Architect

• [nit] Empty-string return as sentinel -- None would be more idiomatic for no ref selected at src/apm_cli/marketplace/resolver.py:779
  Returning "" as a falsy sentinel works but None would be more idiomatic for no ref selected and avoids accidental f"#{empty}" if a caller forgets the truthiness check. Low risk given both call sites already guard with if canonical_ref:.
  Suggested: Return None instead of "" and update call sites to if canonical_ref is not None

CLI Logging Expert

No findings.

DevX UX Expert

• [nit] Helper docstring could name the empty-string return semantic at src/apm_cli/marketplace/resolver.py
  Returning "" as a falsy sentinel is fine internally, but a one-line note like "Returns empty string when no explicit ref applies (caller treats as implicit/HEAD)" would save the next reader a round-trip to the call-sites.

Supply Chain Security Expert

• [nit] Consider adding master to _IMPLICIT_MARKETPLACE_REFS at src/apm_cli/marketplace/resolver.py:52
  Some repos still use master as default branch. Appending #master as a registered ref carries the same no-op/misleading risk as main/HEAD. The previous inline guard also only excluded main/HEAD so this is consistent behavior, but the named constant is now the right place to document the canonical list.

OSS Growth Hacker

• [nit] Link docstring to issue refactor(marketplace): extract ref-propagation helpers from resolve_marketplace_plugin #1917 for implicit-ref exclusion context at src/apm_cli/marketplace/resolver.py
  The removed 14 lines of inline commentary explained why implicit refs are excluded. A one-liner docstring reference to refactor(marketplace): extract ref-propagation helpers from resolve_marketplace_plugin #1917 preserves discoverability for drive-by contributors without inline noise.

Auth Expert -- inactive

Only src/apm_cli/marketplace/resolver.py was changed -- a pure refactor of ref selection priority logic with no touch to auth tokens, credential resolution, AuthResolver, host classification, or authorization headers.

Doc Writer -- inactive

The only changed file is src/apm_cli/marketplace/resolver.py (internal refactor); no documentation files, README, CHANGELOG, or skill/agent prose were modified, and the removed inline comments were never reflected in the public docs corpus.

Test Coverage Expert

• [nit] GitLab path missing HEAD-implicit exclusion test (only main is tested for GitLab path) at tests/unit/marketplace/test_marketplace_resolver.py
_IMPLICIT_MARKETPLACE_REFS contains both main and HEAD. GitLab path tests main-not-propagated but not HEAD. GitHub path covers both. Risk is extremely low since both paths share the same helper, but symmetric parametrize guards against future divergence.
  Suggested: Add test_gitlab_string_source_head_ref_not_propagated mirroring the existing main test.
  Proof (passed): tests/unit/marketplace/test_marketplace_resolver.py::test_github_string_source_head_ref_not_appended -- proves: HEAD is treated as implicit and not propagated (GitHub path)
  assert "#" not in result.canonical

Performance Expert

No findings.

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

[Sergio Sisternes (sergio-sisternes-epam)]

PR triage recommendation

needs-issue

Recommendation only, not merge or scope approval. A responsible
human maintainer must approve. Labels, automated advice, and
silence are not approval.

Linked issue

#1917 is open (type/refactor, area/marketplace,
theme/portability, priority/low, triage/recommended). It does
not currently carry status/accepted. Maintainer Daniel Meppiel (@danielmeppiel)
filed the issue on 2026-06-25 as a bounded extract-helper follow-up
from #1880. On 2026-06-26 an automated triage comment applied
status/triaged and said silence is approval; that is not
authorization. On 2026-09-12 the same maintainer replaced
status/triaged with triage/recommended. Previous bot labels or
the 2026-08-18 review-panel ship_now note are not approval to
merge.

Proposed classification

type/refactor, area/marketplace, theme/portability

The PR currently has no labels. This comment does not write human
decision labels.

Suggested next action

Leave #1992 open. Do not rebase, refresh, or merge it while #1917
has no human-recorded status/accepted (or an explicit defer /
design hold). Wait for a responsible human maintainer to record
bounded scope, acceptance criteria, and review contact on #1917.
Do not file a second issue.

Suggested PR comment

Thanks @WilliamK112 -- the extract of marketplace ref selection into
a shared helper, the 2026-07-23 rebase onto current main, and the
green marketplace/ref validation note are on the record.

This PR is not ready for review as a merge candidate. It closes
#1917, which is a real bounded refactor filed by a maintainer, but
that issue is not currently maintainer-accepted. The 2026-06-26
automated triage used "silence is approval," which is not
authorization. On 2026-09-12 the lead maintainer moved #1917 from
`status/triaged` to `triage/recommended` only. Please do not spend
time rebasing this branch for our benefit until a maintainer
records scope on #1917.

The 2026-08-18 APM Review Panel `ship_now` note is advisory. It
does not record `status/accepted` and is not merge approval.

CODEOWNERS: default owners are @danielmeppiel and
@sergio-sisternes-epam. The changed path is
`src/apm_cli/marketplace/resolver.py`, which uses those default
owners. Existing review requests are left as-is; this comment does
not request reviewers and is not merge approval.

When a maintainer re-invites the work, the still-open optional
follow-ups from the panel remain: GitLab HEAD implicit-ref
coverage, whether `master` belongs in the implicit-ref set, and a
docstring pointer to #1917. Those are polish, not a reason to
refresh the PR while the issue is on hold.

[Ching Wei Kang (WilliamK112)]

The issue now has a fresh human scope record and review contact, so I refreshed this existing PR within that exact approved boundary. The single refactor commit is rebased onto current main at 2920043; the only conflict was reconciled by retaining main’s newer explicit-path/dotted-directory behavior and applying the shared ref fallback only inside that path. Validation: 143 resolver tests and 75 related marketplace/ref tests pass; Ruff, format check, Mode B, and diff checks are clean. The PR body now links the scope approval and records the current evidence.