Fix {name} placeholder ignored in apm pack marketplace version resolution (#1822)#1841
Conversation
…tion
builder.py _resolve_version_range called build_tag_regex(pattern) without
name=entry.name. When the tag pattern contains {name} (e.g. {name}-v{version}
or {name}--v{version}), the {name} placeholder was compiled as the wildcard
[^/]+ instead of the literal package name. In a monorepo marketplace every
package's tags matched, so the highest global version tag was selected for
all entries instead of the per-package highest.
Fix: pass name=entry.name to build_tag_regex so {name} is substituted with
the literal (re.escaped) package name, scoping tag matching to the specific
package.
Adds TestMonorepoNameScopedTagPattern regression tests covering:
- Two packages from same repo, single-dash {name}-v{version} build pattern
- Two packages from same repo, double-dash {name}--v{version} build pattern
- Per-entry tag_pattern with {name} placeholder
- No match raised when only other-package tags exist (no cross-contamination)
Closes #1822
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
✅ Ready to approve
The fix is a minimal, targeted change aligned with existing build_tag_regex behavior and is backed by focused regression tests for the reported monorepo tag-collision scenario.
Note: this review does not count toward required approvals for merging.
Pull request overview
Fixes apm pack marketplace tag resolution for monorepos by ensuring {name} in tagPattern scopes matching to the current package, preventing all packages from collapsing onto the highest global semver tag.
Changes:
- Pass
name=entry.nameintobuild_tag_regex()during semver range resolution so{name}is no longer treated as a wildcard. - Add regression tests covering name-scoped tag patterns across multiple packages (single-dash, double-dash, and per-entry tag_pattern), plus the “no match when only other package tags exist” case.
File summaries
| File | Description |
|---|---|
src/apm_cli/marketplace/builder.py |
Fixes tag regex construction to correctly scope {name} patterns per entry during version-range resolution. |
tests/unit/marketplace/test_builder.py |
Adds regression tests for monorepo per-package tag resolution with {name} placeholders (issue #1822). |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
| # Only pkg-b tags present -- pkg-a should NOT pick these up | ||
| refs = {"acme/catalog": _make_refs("pkg-b-v1.0.0", "pkg-b-v2.0.0")} | ||
| with pytest.raises(NoMatchingVersionError): | ||
| _build_with_mock(tmp_path, yml, refs) | ||
|
|
||
|
|
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 0 | Correct one-line fix with strong regression suite; recommend standardising build_tag_regex(name=) in version_resolver.py to close the remaining divergence window. |
| CLI Logging Expert | 0 | 1 | 1 | Fix ends silent wrong-version success; NoMatchingVersionError hint too generic for {name}-pattern scoping failures the fix now correctly surfaces. |
| DevX UX Expert | 0 | 1 | 2 | Fix is correct and the deferred inconsistency is safe; one recommended: error detail should show effective pattern, not raw {name} placeholder. |
| Supply Chain Security Expert | 0 | 0 | 2 | Fix closes a real monorepo cross-package tag-hijacking surface; re.escape sentinel strategy is sound and injection-proof; no new attack surface introduced. |
| OSS Growth Hacker | 0 | 1 | 1 | High-value monorepo correctness fix; CHANGELOG entry missing means silent win for a key adoption segment -- add one liner to Fixed before merge. |
| Doc Writer | 0 | 1 | 0 | CHANGELOG.md [Unreleased] Fixed block missing entry for #1822; all docs already describe correct per-package {name} scoping behavior -- no doc drift. |
| Test Coverage Expert | 0 | 1 | 1 | 4 regression traps cover core isolation bug at integration-pipeline quality; 2 minor gaps noted below. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
-
[OSS Growth Hacker] Add CHANGELOG.md [Unreleased] Fixed entry before merge: "
apm packnow resolves{name}in tag patterns per-package instead of treating it as a wildcard, so monorepo marketplaces no longer pin every entry to the global highest version tag. (closes [BUG] apm pack marketplace resolution ignores {name}, pins every package to the highest global version tag #1822) (Fix {name} placeholder ignored in apm pack marketplace version resolution (#1822) #1841)" -- Dual-persona convergence (oss-growth-hacker + doc-writer); users bitten by [BUG] apm pack marketplace resolution ignores {name}, pins every package to the highest global version tag #1822 will not discover the fix without a release-note entry. This is the only pre-merge action the panel identified. -
[CLI Logging Expert] Improve
NoMatchingVersionErrorUX post-merge: (1) show the effective substituted pattern in error detail rather than the raw{name}template (builder.py:738, devx-ux-expert); (2) add a hint branch for{name}-pattern failures directing users to verify per-package tags exist, not just their semver range (marketplace/__init__.py:1049, cli-logging-expert). -- Two panelists converge on the same error surface; the current hint actively misdirects users after the fix. -
[Test Coverage Expert] Add
@pytest.mark.parametrizecase for regex-special package name intest_tag_pattern.py:build_tag_regex('{name}-v{version}', name='my.pkg')should matchmy.pkg-v1.0.0and notmyXpkg-v1.0.0. --outcome: missingon asecure_by_defaultsurface (convergent with supply-chain-security); the PR body asserts the safety property but there.escapecontract is unpinned and would go undetected through a future refactor. -
[Python Architect] Open a follow-up issue to standardize
version_resolver.py:80to thename=kwarg form: replacepinned_pattern = tag_pattern.replace('{name}', plugin_name); tag_rx = build_tag_regex(pinned_pattern)withtag_rx = build_tag_regex(tag_pattern, name=plugin_name). -- Currently produces identical output but diverges from the canonical pattern this PR proved correct; ifbuild_tag_regexevolves, this call site will drift silently. -
[Supply Chain Security Expert] Add a comment to
tag_pattern.py:124on theand namewildcard fallback guard marking it as defensive-only: schema enforces non-empty name at the builder call-site; direct API callers must not passname=""when{name}scoping is required. --build_tag_regexis a public API; a future caller passingname=''would unknowingly restore the monorepo tag-bleeding behavior this PR fixes.
Architecture
classDiagram
direction TB
class MarketplaceBuilder {
<<Builder>>
+resolve() ResolveResult
-_resolve_entry(entry) ResolvedPackage
-_resolve_version_range(entry, resolver, owner_repo, yml) ResolvedPackage
-_resolve_explicit_ref(entry, resolver, owner_repo) ResolvedPackage
}
class PackageEntry {
<<ValueObject>>
+name str
+version str
+tag_pattern str
+ref str
}
class MarketplaceBuild {
<<ValueObject>>
+tag_pattern str
}
class ResolvedPackage {
<<ValueObject>>
+name str
+ref str
+sha str
}
class RefResolver {
<<IOBoundary>>
+list_remote_refs(owner_repo) list
}
class build_tag_regex {
<<Pure>>
build_tag_regex(pattern, name) Pattern
}
class resolve_version_constraint {
<<Pure>>
resolve_version_constraint(plugin_name, owner_repo, version_range) tuple
}
MarketplaceBuilder ..> PackageEntry : reads name and tag_pattern
MarketplaceBuilder ..> MarketplaceBuild : reads build tag_pattern
MarketplaceBuilder ..> RefResolver : list_remote_refs
MarketplaceBuilder ..> build_tag_regex : calls with name=entry.name
MarketplaceBuilder ..> ResolvedPackage : produces
resolve_version_constraint ..> build_tag_regex : calls via str.replace bypass
note for MarketplaceBuilder "Fix: _resolve_version_range now passes name=entry.name so tag pattern scopes to this package only"
note for resolve_version_constraint "Inconsistency: manually replaces name placeholder via str.replace before build_tag_regex call -- deferred cleanup"
class MarketplaceBuilder:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["apm pack CLI"] --> B["MarketplaceBuilder.resolve() -- builder.py:760"]
B --> C["_resolve_entry(entry) per PackageEntry -- builder.py:566 via ThreadPoolExecutor"]
C --> D{"entry.ref is not None?"}
D -- yes --> E["_resolve_explicit_ref(entry, ...) -- builder.py:605"]
D -- no --> F["_resolve_version_range(entry, resolver, owner_repo, yml) -- builder.py:702"]
F --> G["pattern = entry.tag_pattern or yml.build.tag_pattern -- builder.py:717"]
G --> H["build_tag_regex(pattern, name=entry.name) -- tag_pattern.py:71 -- FIXED"]
H --> I["name_rx = re.escape(entry.name) -- tag_pattern.py:124 scopes regex to this package only"]
I --> J["RefResolver.list_remote_refs(owner_repo) -- ref_resolver.py"]
J --> K["iter_semver_tags(refs, tag_rx) -- _shared.py:16"]
K --> L["prerelease + satisfies_range filter -- builder.py:726-732"]
L --> M{"candidates empty?"}
M -- yes --> N["raise NoMatchingVersionError -- errors.py"]
M -- no --> O["candidates.sort descending SemVer -- builder.py:742"]
O --> P["return ResolvedPackage(ref=best_tag, sha=best_sha) -- builder.py:745"]
Recommendation
The fix is clean, correct, and proven at integration-with-fixtures tier with no blocking panel findings. The sole pre-merge action is adding the CHANGELOG entry (recommended independently by both oss-growth-hacker and doc-writer) -- one line under [Unreleased] Fixed that turns a silent correctness fix into a visible upgrade signal for monorepo marketplace users. All other follow-ups (error message UX, re.escape parametrize test, version_resolver kwarg standardization, tag_pattern guard comment) are post-merge. Add the CHANGELOG entry and merge.
Full per-persona findings
Python Architect
- [recommended] version_resolver.resolve_version_constraint bypasses the name= kwarg via manual str.replace, creating a silent divergence risk at
src/apm_cli/marketplace/version_resolver.py:80
resolve_version_constraint doespinned_pattern = tag_pattern.replace('{name}', plugin_name)then callsbuild_tag_regex(pinned_pattern)with noname=arg. This pre-substitutes the name manually rather than routing through the canonical kwarg the PR just proved is correct. Ifbuild_tag_regexever evolves its name handling, this call site silently diverges without a test to catch it. All other external callers already use thename=kwarg form.
Suggested: Replacepinned_pattern = tag_pattern.replace('{name}', plugin_name); tag_rx = build_tag_regex(pinned_pattern)withtag_rx = build_tag_regex(tag_pattern, name=plugin_name).
CLI Logging Expert
-
[recommended] NoMatchingVersionError hint is too generic for {name}-pattern monorepo failures now correctly raised by the fix at
src/apm_cli/commands/marketplace/__init__.py:1049
Before the fix, monorepo users withtagPattern: '{name}-v{version}'never sawNoMatchingVersionError-- the wildcard silently matched another package's tag and the CLI printed a success. After the fix they correctly see this error when their own per-package tags are absent. The current hint directs them to audit their semver range, but the actual issue is missing per-package tags matching the{name}pattern.
Suggested: When the exc detail includes a pattern containing '{name}', emit a second hint line: verify that per-package tags (e.g. pkg-a-v1.2.3) exist on the remote. -
[nit] No logger.debug() trace in _resolve_version_range for the scoped regex or candidate set at
src/apm_cli/marketplace/builder.py:733
The adjacent setup paths already emit logger.debug(). _resolve_version_range logs nothing: no record of which regex was compiled, which entry.name scoped it, or how many candidates survived.
Suggested: After the candidates list is built, add:logger.debug("Resolving '%s': pattern=%r name=%r -> %d candidate(s)", entry.name, pattern, entry.name, len(candidates))
DevX UX Expert
-
[recommended] NoMatchingVersionError detail exposes raw {name} placeholder; show the effective substituted pattern instead at
src/apm_cli/marketplace/builder.py:738
After the fix, when a monorepo package has no matching tags, the error detail reads:pattern='{name}-v{version}', remote='acme/catalog'. Showing the raw placeholder is confusing -- users will stare at{name}and wonder if substitution happened at all (the exact bug this PR fixed). Cargo and pip both show the effective resolved spec, not the template.
Suggested:detail=f"effective-pattern='{pattern.replace('{name}', entry.name)}', remote='{owner_repo}'"(or equivalent). -
[nit] _render_build_error hint for NoMatchingVersionError is generic; mention name-pattern scoping at
src/apm_cli/commands/marketplace/__init__.py
Post-fix, the most common new failure mode is a tag-name prefix mismatch. Adding a note about package name matching the tag prefix would give users a concrete next action. -
[nit] Deferred inconsistency (version_resolver str.replace vs builder name= kwarg) is cosmetically inconsistent but not a split-brain risk at
src/apm_cli/marketplace/version_resolver.py:80
Both code paths produce identical regex for the same input. The PR Trade-offs note is accurate -- this is a style inconsistency, not a correctness gap.
Supply Chain Security Expert
-
[nit] Silent wildcard fallback on empty name not documented as an unreachable defensive guard at
src/apm_cli/marketplace/tag_pattern.py:124
name_rx = re.escape(name) if _PLACEHOLDER_NAME in pattern and name else r'[^/]+'. Theand nameguard silently reverts to wildcard when name is falsy. In practice unreachable (schema enforces non-empty at the builder call-site), butbuild_tag_regexis a public API and a future caller passingname=''would unknowingly restore monorepo cross-package tag-bleeding.
Suggested: Add comment:# defensive only; schema enforces non-empty name at builder call-site. Direct API callers must not pass name="" when {name} scoping is required. -
[nit] Regression suite omits a package name containing regex metacharacters at
tests/unit/marketplace/test_builder.py
The four new regression tests use only plain alphanumeric-hyphen names. No test pins the re.escape contract for regex-special chars (e.g.my.pkg). A future refactor bypassing re.escape would go undetected.
Suggested: Add@pytest.mark.parametrize('pkg_name', ['pkg-a', 'my.pkg', 'pkg+extra'])case totest_tag_pattern.py.
OSS Growth Hacker
-
[recommended] Missing [Unreleased] CHANGELOG entry for a monorepo correctness fix that closes a tracked issue at
CHANGELOG.md
Monorepo marketplace support is a key differentiator; users bitten by [BUG] apm pack marketplace resolution ignores {name}, pins every package to the highest global version tag #1822 will not know to upgrade unless this fix appears in release notes. The [Unreleased] Fixed block has no entry for this PR.
Suggested: Add under [Unreleased] Fixed: 'apm packnow resolves{name}in tag patterns per-package instead of treating it as a wildcard, so monorepo marketplaces no longer pin every entry to the global highest version tag. (closes [BUG] apm pack marketplace resolution ignores {name}, pins every package to the highest global version tag #1822) (Fix {name} placeholder ignored in apm pack marketplace version resolution (#1822) #1841)' -
[nit] Two competing {name}-substitution idioms in builder vs. version_resolver; worth a follow-up issue for discoverability at
src/apm_cli/marketplace/builder.py:719
A new contributor adding a third call site has no canonical signal on which pattern to follow.
Suggested: Open a follow-up issue: 'Standardize {name} substitution in tag regex calls to name= kwarg form'.
Auth Expert -- inactive
PR only touches src/apm_cli/marketplace/builder.py and tests/unit/marketplace/test_builder.py -- no auth fast-path files modified.
Doc Writer
- [recommended] CHANGELOG.md [Unreleased] Fixed section has no entry for this bug fix ([BUG] apm pack marketplace resolution ignores {name}, pins every package to the highest global version tag #1822) at
CHANGELOG.md
apm packsilently pinning every monorepo package to the highest global version is a user-facing behavioral regression. The [Unreleased] Fixed block already records five other fixes but omits this one.
Suggested: Add under [Unreleased] Fixed: 'apm packno longer pins every monorepo package to the highest global version tag whenbuild.tagPatternortag_patterncontains{name}. (closes [BUG] apm pack marketplace resolution ignores {name}, pins every package to the highest global version tag #1822) (Fix {name} placeholder ignored in apm pack marketplace version resolution (#1822) #1841)'
Test Coverage Expert
-
[recommended] Scenario Evidence table absent from PR body -- principle mapping for the 'Governed by policy' promise is unverified at
tests/unit/marketplace/test_builder.py
Per the scenario-evidence-rubric, behavior-change PRs should include a Scenario Evidence table mapping each user scenario to an APM principle and a test path. The 4 regression traps via_build_with_mockare well-targeted and pass, but the principle mapping is absent from the PR body.
Proof (passed at integration-with-fixtures):tests/unit/marketplace/test_builder.py::TestMonorepoNameScopedTagPattern::test_two_packages_resolve_independently-- proves: Each package in a monorepo marketplace resolves only against its own version tags, not the global maximum [governed-by-policy]
assert resolved_by_name['pkg-a'] == 'pkg-a-v1.0.0'; assert resolved_by_name['pkg-b'] == 'pkg-b-v2.0.0' -
[nit] Safety claim 're.escape handles special chars in package name' is unverified by any test at builder or tag-pattern level at
tests/unit/marketplace/test_tag_pattern.py
The PR body states 'Empty names and names with special chars are safe (re.escape)'. Code inspection confirms the claim, but no existing test exercisesbuild_tag_regexwith a regex-special character in the name. A future refactor bypassing re.escape would go undetected.
Proof (missing at unit):tests/unit/marketplace/test_tag_pattern.py::TestBuildTagRegex::test_name_with_regex_special_chars_escaped-- proves: A package name containing a regex-special character is correctly anchored by re.escape [secure-by-default]
assert rx.match('my.pkg-v1.0.0') is not None; assert rx.match('myXpkg-v1.0.0') is None
Performance Expert -- inactive
Only src/apm_cli/marketplace/builder.py and tests/unit/marketplace/test_builder.py are touched; neither is in the perf fast-path, and the one-line change narrows the tag-regex candidate set (net non-regression).
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1841 · sonnet46 14.5M · ◷
Resolve strangler-fig conflicts in install.py, hook_integrator.py, and builder.py: keep HEAD's relocated structure and fold main's behavioural deltas into the sibling helper modules that now own the code. - #1816 registry-routing: port default_registry param + probe-skip bypass into install/pkg_resolution.py (RULE B seam for update_existing_dependency_entry_if_needed re-exported from commands/install.py). - #1840 cursor hooks "version": 1 top-level default: port into integration/hook_transforms.py + injection in hook_integrator.py. - #1841 pack {name} placeholder fix: port name=entry.name into marketplace/_builder_resolve.py. Fix merge-introduced ruff complexity regressions in #1816 code: extract _registry_manifest_range_or_row (outdated.py PLR0911), _resolve_package_accessibility + _intercept_marketplace_ref (pkg_resolution.py C901/PLR0915). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TL;DR
apm packwas pinning every monorepo package to the same (highest global) version tag instead of each package's own tag. One missing argument caused{name}to become a wildcard.Problem (WHY)
When
_resolve_version_rangeinbuilder.pybuilds the tag-matching regex:The
namekeyword argument was omitted.build_tag_regexhandles a missingnameby substituting[^/]+(wildcard) for{name}in the compiled regex. In a monorepo marketplace with pattern{name}-v{version}, this means:pkg-a-v1.0.0matched (wildcard coverspkg-a)pkg-b-v2.0.0also matched (wildcard coverspkg-b)Both packages competed in the same candidate pool, so the one with the highest semver won --
pkg-b-v2.0.0for both entries. Closes #1822.Approach (WHAT)
Pass
name=entry.nametobuild_tag_regex:build_tag_regexalready supports this argument (re.escape(name)substitution, implemented attag_pattern.py:124). It is correctly used ininfer_tag_pattern,parse_tag_version, andversion_resolver.resolve_version_constraint. The omission in_resolve_version_rangewas the sole source of the bug.Implementation (HOW)
One line, one file.
build_tag_regexis fully defensive: patterns without{name}are unaffected (the guard_PLACEHOLDER_NAME in pattern and nameshort-circuits). Empty names and names with special chars are safe (re.escape).Diagrams
flowchart TD A["_resolve_version_range(entry, ...)"] --> B["pattern = entry.tag_pattern or yml.build.tag_pattern"] B --> C{"pattern contains {name}?"} C -->|"Before fix (no name arg)"| D["build_tag_regex(pattern)\n{name} -> [^/]+ wildcard"] C -->|"After fix"| E["build_tag_regex(pattern, name=entry.name)\n{name} -> re.escape(entry.name)"] D --> F["All packages' tags match\n-> highest global version wins"] E --> G["Only this package's tags match\n-> correct per-package resolution"]Trade-offs
version_resolver.resolve_version_constraintuses a different but equivalent pattern (externalstr.replacebefore callingbuild_tag_regex). Both patterns are correct; standardizing on thename=kwarg form is deferred as a clean-up (no functional impact).Validation evidence
Full lint chain passes:
New regression tests (all passing):
Full
test_builder.py: 138 passed.How to test