fix(artifactory): deterministic boundary probe for nested GitLab paths

[chkp-roniz]

Summary

Deps routed through an Artifactory VCS proxy can sit at arbitrary subgroup depth when the upstream is GitLab (e.g. group/subgroup/project). Parse-time can't tell where the repo path ends and the in-repo virtual sub-path begins -- the previous code disambiguated by hard-coded directory markers (skills/, prompts/, ...). This PR replaces the heuristic with a deterministic install-time probe.

• Mode 1 -- explicit FQDN deps (host/artifactory/key/owner/repo/...).
• Mode 2 -- bare shorthand (owner/repo/...) under PROXY_REGISTRY_URL + PROXY_REGISTRY_ONLY=1.

_resolve_artifactory_boundary walks candidate (owner, repo, virtual_path) splits shallow-first and HEAD-probes each candidate's archive URL on the proxy; the first split that responds 2xx-3xx wins. Mirrors the native GitLab resolver pattern (_try_resolve_gitlab_direct_shorthand) but uses the archive URL itself as the existence signal, so no separate metadata API is needed.

Key invariants:

• Deterministic -- single-candidate paths return the parse-time ref unchanged; ambiguous paths probe; if every candidate is rejected the function raises with a clear error (no silent fallback to a guess).
• Auth-vs-missing distinction -- 401/403 across all candidates raises a different error than 4xx-other, so a misconfigured token surfaces as an auth problem instead of a "missing repo".
• No cross-host token leakage -- allow_redirects=False on the HEAD probe means the Bearer token can't follow a redirect off the proxy host.
• Audience-correct auth -- Mode 1 uses the per-host auth resolver (the dep's host is the proxy); Mode 2 uses PROXY_REGISTRY_TOKEN from RegistryConfig (the dep's logical host is github.com but the request goes to the proxy).
• Lockfile stability -- when the probe confirms the parse-time boundary, the original ref is returned so apm.yml isn't rewritten from bare shorthand to a structured URL form.

Changes

• New apm_cli/install/artifactory_resolver.py — the deterministic probe.
• apm_cli/utils/github_host.py — iter_artifactory_boundary_candidates (mirrors iter_gitlab_direct_shorthand_boundary_candidates); parse_artifactory_path simplified to a structural rule; build_artifactory_archive_url uses the basename for nested GitLab archive filenames.
• apm_cli/models/dependency/reference.py — removes _VIRTUAL_PATH_ROOT_SEGMENTS; adds from_artifactory_boundary_probe; simplifies _bare_shorthand_repo_segment_count to default-deep + structural file-extension rule; _validate_url_repo_path now strips the artifactory/<key> prefix from URL-form deps so round-trip doesn't double-prefix download URLs.
• apm_cli/install/package_resolution.py — wires the resolver alongside the GitLab probe; renames the second tuple slot to direct_virtual_resolved since it now covers both probes.
• apm_cli/commands/install.py — imports the resolver.
• apm_cli/deps/artifactory_orchestrator.py — _split_owner_repo folds subgroup segments into the repo slug instead of dropping them.
• tests/unit/test_artifactory_support.py — new tests for the iterator, the resolver (mocked HEAD), Mode 2 path, and the 401-vs-404 distinction; existing tests updated to reflect the parse-time-shallow + install-time-authoritative model.
• docs/src/content/docs/enterprise/registry-proxy.md — new "Nested-group repos" section + "Explicit Artifactory FQDN: deterministic boundary probe" subsection.

Test plan

• pytest tests/unit/test_artifactory_support.py (Artifactory unit suite).
• pytest tests/unit/install/ tests/unit/deps/ tests/unit/marketplace/ tests/unit/commands/test_install_resolve_refs.py (resolver + install + marketplace tests).
• ruff check src/apm_cli/install/artifactory_resolver.py src/apm_cli/install/package_resolution.py src/apm_cli/utils/github_host.py src/apm_cli/models/dependency/reference.py src/apm_cli/commands/install.py tests/unit/test_artifactory_support.py.
• Live install Mode 1 against a real proxy: apm install <host>/artifactory/<key>/<group>/<subgroup>/<repo>#<ref>.
• Live install Mode 2 with PROXY_REGISTRY_URL + PROXY_REGISTRY_ONLY=1: apm install <group>/<subgroup>/<repo>#<ref>.
• Marketplace install through the proxy: apm install <plugin>@<marketplace> resolves and integrates without falling back to github.com.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds deterministic install-time resolution for Artifactory proxy “repo vs virtual path” boundaries (including GitLab subgroup-style nested repo paths), replacing parse-time heuristics and updating docs/tests accordingly.

Changes:

• Added an install-time Artifactory boundary probe that HEAD-checks candidate archive URLs and rebuilds dependency refs at the verified split.
• Updated Artifactory path parsing and archive URL construction to better support nested GitLab subgroup repos (basename-based GitLab archive filenames).
• Expanded unit tests and documentation for proxy/registry-only behavior and nested-group repos.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/unit/test_artifactory_support.py	Adds coverage for nested subgroup parsing, candidate enumeration, archive URL naming, and boundary probe behavior.
src/apm_cli/utils/github_host.py	Introduces Artifactory boundary candidate iterator; simplifies parse-time split; updates GitLab archive filename logic.
src/apm_cli/models/dependency/reference.py	Adds Artifactory boundary probe constructor; changes bare shorthand parsing rules under registry-only mode; strips Artifactory prefix in URL validation.
src/apm_cli/install/package_resolution.py	Allows optional Artifactory boundary resolver to rebuild refs and trigger structured persistence.
src/apm_cli/install/artifactory_resolver.py	New authoritative resolver that probes candidate boundaries against the proxy and rebuilds refs deterministically.
src/apm_cli/deps/artifactory_orchestrator.py	Updates owner/repo splitting and subdirectory download to preserve nested repo slugs.
src/apm_cli/commands/install.py	Wires the Artifactory resolver into install-time dependency normalization/persistence.
docs/src/content/docs/enterprise/registry-proxy.md	Documents nested-group repos behind proxy, probing behavior, and troubleshooting.
CHANGELOG.md	Notes new resolver behavior, removed heuristics, and fixes for nested-group installs and prefix round-tripping.

[danielmeppiel]

APM Review Panel: needs_rework

External enterprise contributor (Check Point) unblocks nested GitLab-subgroup installs behind Artifactory proxies -- valuable fix, but the new 280-line resolver module ships with zero automated regression coverage.

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

This PR delivers genuine enterprise value: it unblocks a real workflow (nested GitLab subgroups behind JFrog Artifactory) that APM cannot serve today, contributed by an external enterprise user (chkp-roniz, Check Point). The auth-expert verified the security claims hold in code -- token audience separation, redirect-leak prevention, and graceful degradation are all correctly implemented. The python-architect confirms module placement and pattern choices are sound. No panelist disputes the value or correctness of the fix itself.

The convergent signal across three independent panelists (test-coverage-expert, devx-ux-expert, supply-chain-security-expert) is unambiguous: the new install-pipeline module has no automated tests covering its core behavior. The test-coverage-expert grep'd the entire test tree and confirmed zero matches for _resolve_artifactory_boundary, _candidate_archive_status, _proxy_probe_headers, or any of the six public/semi-public symbols in the new module. This is a critical-promise surface (install pipeline) where the floor is integration-with-fixtures. The code is correct today; the problem is that nothing will notice if a future refactor breaks the token-audience split, the redirect-leak guard, or the 401-vs-404 error discrimination. Additionally, the silent network-exception swallow (raised independently by devx-ux and supply-chain-security) means a DNS failure or transient timeout could lock in a wrong boundary with no user signal -- this warrants at minimum an INCONCLUSIVE status or progress indicator.

The CHANGELOG entries need mechanical fixes (missing #1472 suffix per hard project rule, internal symbol names instead of user outcomes) and the RST-style backticks in error strings are a minor polish item. These are low-effort, low-risk fixes that should not hold up the contributor.

Aligned with: secure-by-default (allow_redirects=False and token-audience separation correctly implemented in code; verified by auth-expert -- gap: no regression test would catch removal of these guards); multi-harness/multi-host (extends APM's multi-host story to nested GitLab subgroups behind Artifactory -- a real enterprise topology APM could not serve before); governed-by-policy (lockfile env-dependence is documented but not surfaced at runtime); oss-community-driven (first external enterprise contributor landing a non-trivial install-pipeline fix); pragmatic-as-npm (install-time HEAD-probe adds network I/O to the default path with no progress indicator).

Growth signal. Check Point (chkp-roniz) is the first external enterprise contributor to land a fix in the install pipeline. This is a credibility signal worth amplifying: it demonstrates that enterprise shops behind Artifactory proxies are adopting APM and contributing back. Consider a README enterprise blurb update, a standalone APM+JFrog integration guide, and contributor recognition in the next release announcement.

Panel summary

Persona	B	R	N	Takeaway
python-architect	0	2	3	Well-structured install-time resolver mirrors GitLab prober pattern; module placement correct; two recommended improvements for coupling and SRP.
cli-logging-expert	0	1	2	Verbose probe output uses raw print() instead of canonical _rich_echo dim helper; error messages well-structured and token-safe but miss actionable fix hint.
devx-ux-expert	0	3	4	Ship-worthy enterprise fix; default-path silent multi-second network I/O and ambiguous network-vs-missing error are the two ergonomic gaps; lockfile env-dependence is documented but not surfaced at runtime.
supply-chain-security-expert	0	1	2	Token-leak guard (allow_redirects=False) and audience separation correctly implemented; no blocking security issues; one recommended hardening and two nits.
oss-growth-hacker	0	1	2	Strong enterprise-credibility beat: community contributor from Check Point unblocks GitLab-shop Artifactory users. Docs land in right sidebar; CHANGELOG dense.
auth-expert	0	0	1	Auth claims VERIFIED. Mode 1 delegates to AuthResolver.resolve_for_dep correctly; Mode 2 bypasses AuthResolver and sources bearer from RegistryConfig.from_env() (avoids github.com token leak); allow_redirects=False prevents cross-host token leakage. No blocking issues.
doc-writer	0	2	4	Docs accurate vs code. Fix CHANGELOG PR-number suffix and propagate PROXY_REGISTRY_ONLY semantic drift to env-var reference and auth pages.
test-coverage-expert	1	4	1	New resolver module ships ~280 lines of critical install-pipeline logic with ZERO automated tests; unit tests cover adjacent surfaces but the probe itself, its auth-audience split, redirect-leak prevention, and error discrimination are untested.

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] (blocking-severity) Add integration-with-fixtures tests for _resolve_artifactory_boundary covering: boundary probe happy path, allow_redirects=False mutation-break, 401/403 vs 4xx error discrimination, Mode 2 token audience (proxy bearer not github.com PAT), and 3+ segment subgroup folding. -- The entire new 280-line module is untested. On a critical-promise surface (install pipeline), missing tests are a regression-trap gap equivalent to a blocking defect. A single refactor could silently break token-audience separation or redirect-leak prevention with no CI signal.
2. [supply-chain-security-expert] Add INCONCLUSIVE candidate status: fail closed on transient network errors instead of silently skipping candidates. -- Silent exception swallow on DNS/TLS/timeout can lock in a wrong boundary or surface misleading "missing repo" error. Raised independently by devx-ux and supply-chain-security -- convergent signal on a security-relevant surface.
3. [doc-writer] Fix CHANGELOG entries: add (#1472) suffix, replace internal symbol names with user-outcome language, and cross-reference PROXY_REGISTRY_ONLY semantic drift to env-var reference page. -- Missing PR-number suffix violates a hard project rule (.github/instructions/changelog.instructions.md). Internal symbol leakage in CHANGELOG erodes the user-facing narrative.
4. [devx-ux-expert] Add progress indicator (spinner or dim echo) for HEAD-probe network I/O on the default install path; surface timeout feedback. -- 15s timeout x N candidates x 4 URL shapes can produce 60+ seconds of silent hang. Users will assume APM froze. Low effort to add via existing _rich_echo(dim) pattern.
5. [cli-logging-expert] Replace bare print() in verbose probe output with _rich_echo(dim) or CommandLogger to respect --quiet, styling, and test capture. -- Bypasses the canonical output helpers; will not be suppressed by --quiet and cannot be captured in integration tests.

Architecture

classDiagram
    class iter_artifactory_boundary_candidates {
        +iter() Iterator~ArtifactoryBoundaryCandidate~
    }
    class ArtifactoryBoundaryCandidate {
        +owner: str
        +repo: str
        +depth: int
    }
    class _resolve_artifactory_boundary {
        +resolve(dep_ref, ctx) DependencyReference
    }
    class _candidate_archive_status {
        +probe(url, headers) _CandidateStatus
    }
    class _proxy_probe_headers {
        +build(dep_ref, ctx) dict
    }
    class _proxy_routing_target {
        +build(dep_ref) str
    }
    class RegistryConfig {
        +from_env() RegistryConfig
        +bearer_token: str
    }
    class AuthResolver {
        +resolve_for_dep(dep_ref) AuthContext
    }
    _resolve_artifactory_boundary --> iter_artifactory_boundary_candidates
    _resolve_artifactory_boundary --> _candidate_archive_status
    _resolve_artifactory_boundary --> _proxy_probe_headers
    _resolve_artifactory_boundary --> _proxy_routing_target
    _proxy_probe_headers ..> RegistryConfig : Mode 2 lazy import
    _proxy_probe_headers ..> AuthResolver : Mode 1
    _proxy_routing_target ..> ArtifactoryOrchestrator : lazy import

Loading

flowchart LR
    install[install.py] --> resolve[_resolve_artifactory_boundary]
    resolve --> iter[iter_artifactory_boundary_candidates<br/>github_host.py]
    resolve --> probe[_candidate_archive_status<br/>HEAD probe]
    probe --> headers[_proxy_probe_headers]
    probe --> target[_proxy_routing_target]
    headers -.Mode 1.-> auth[AuthResolver]
    headers -.Mode 2.-> regconf[RegistryConfig.from_env]
    target -.lazy.-> orch[ArtifactoryOrchestrator]

Loading

Recommendation

The fix is correct and valuable, but shipping a new 280-line install-pipeline module with zero automated test coverage violates APM's regression-trap floor for critical-promise surfaces. Recommended path: ask the contributor to add integration-with-fixtures tests for the boundary resolver (offer to pair or co-author if needed to unblock), fix the CHANGELOG entries mechanically, then ship. The network-exception and progress-indicator items can land as fast follow-up issues to avoid blocking the contributor further. Prioritize keeping this contributor engaged -- the enterprise credibility signal is high.

---

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

[danielmeppiel]

Follow-ups from the apm-review-panel pass have landed. Summary:

• add-resolver-tests: Add integration-with-fixtures tests for the new boundary resolver covering all five panel-flagged behaviors (happy-path boundary probe, allow_redirects=False redirect-leak guard, 401/403-vs-other-4xx error discrimination, Mode 2 token-audience isolation, and _split_owner_repo 3+ segment subgroup folding) -- resolved in 4ae18ca.

New tests landed:

• tests/unit/install/test_artifactory_resolver.py (4 classes, 6 tests) -- focused regression traps for the security and audience-separation contracts:
  • TestRedirectLeakGuard::test_every_probe_disables_redirects -- asserts every requests.head probe sets allow_redirects=False.
  • TestModeTwoTokenAudience::test_authorization_header_is_proxy_bearer_not_github_pat -- asserts the Mode 2 probe Authorization header is the proxy bearer from RegistryConfig.from_env() and never the github.com PAT from AuthResolver.
  • TestErrorDiscrimination::test_all_403_raises_auth_specific_error, test_mixed_401_and_404_demotes_to_unresolved, test_429_is_not_auth -- 401/403 classify as AUTH; any non-auth 4xx demotes the candidate set to MISSING so the user-facing error stays accurate.
• tests/unit/deps/test_artifactory_orchestrator.py::TestSplitOwnerRepoSubgroupFolding (5 tests) -- parametrized 2/3/4/6-segment subgroup folding for _split_owner_repo, plus the malformed-single-segment guard.

The existing happy-path boundary-probe coverage in tests/unit/test_artifactory_support.py (TestArtifactoryBoundaryResolver, TestArtifactoryOrchestratorNestedRepo) was already in this PR and is left intact; the new tests are additive regression traps for the contracts not previously asserted.

Regression-trap evidence (mutation-break gate):

• tests/unit/install/test_artifactory_resolver.py::TestRedirectLeakGuard::test_every_probe_disables_redirects -- flipped allow_redirects=False to allow_redirects=True in _candidate_archive_status; test FAILED with AssertionError: allow_redirects must be False on every probe ... assert True is False; guard restored.
• tests/unit/install/test_artifactory_resolver.py::TestModeTwoTokenAudience::test_authorization_header_is_proxy_bearer_not_github_pat -- rewired the Mode 2 branch of _proxy_probe_headers to call auth_resolver.resolve_for_dep instead of RegistryConfig.from_env; test FAILED with AssertionError: Mode 2 must not carry the github.com PAT to the proxy host ... 'github-pat-LEAK' is contained here: Bearer github-pat-LEAK; guard restored.

This commit also applies a ruff format pass to three files (src/apm_cli/install/artifactory_resolver.py, tests/unit/test_artifactory_support.py, and the new test file) to bring them in line with the canonical formatter; the drift was pre-existing on this branch and would have failed CI lint.

Lint contract: uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both silent. uv run --extra dev python -m pylint --disable=all --enable=R0801 --min-similarity-lines=10 --fail-on=R0801 src/apm_cli/ rated 10.00/10. bash scripts/lint-auth-signals.sh clean.

CI: local test suite for the three affected test files (175 tests) all pass. The full GitHub Actions suite is not auto-triggered for this external-fork PR (microsoft/apm policy gates fork-PR workflows behind maintainer approval); the only registered check on the head commit is license/cla (SUCCESS). A maintainer will need to approve workflows to surface the full CI rollup.

Ready for maintainer review.

[chkp-roniz]

Thanks for the panel pass — addressed in 57a3c02 (on top of @danielmeppiel's 4ae18ca).

Addressed in this push

doc-writer (mechanical): Added (#1472) suffix to every CHANGELOG entry belonging to this PR, per the hard rule in .github/instructions/changelog.instructions.md. Also re-merged origin/main so the new [Unreleased] entries from #1488 / #1490 / pinned-constraint fix sit next to this PR's entries in a single block.

supply-chain-security-expert + devx-ux-expert (convergent): The silent network-exception swallow in _candidate_archive_status is gone. New _CandidateStatus.INCONCLUSIVE fires when every URL shape for a candidate raised a transport error (no HTTP response observed). The resolver fails closed on all-INCONCLUSIVE — raises a network-specific ValueError carrying the underlying exception and the proxy host, instead of mis-classifying as MISSING. Per-URL-shape transport errors are still tolerated as long as at least one URL shape per candidate produced an HTTP response (a 404 on /archive/refs/heads/... plus a ConnectionError on /zip/refs/heads/... still counts as MISSING, because we did observe the server).

Two regression-trap tests landed in tests/unit/install/test_artifactory_resolver.py:

• TestInconclusiveFailsClosed::test_all_transport_errors_raise_network_specific_error — requests.ConnectionError on every probe → ValueError with "could not reach the proxy" and the underlying error text; explicitly asserts the missing-repo phrasing is NOT used.
• TestInconclusiveFailsClosed::test_partial_transport_failure_does_not_fail_closed — one transport error + 404s on the rest → MISSING classification (resolver keeps walking the candidate list).

Punted to follow-up issues (per panel guidance)

The panel explicitly said:

The network-exception and progress-indicator items can land as fast follow-up issues to avoid blocking the contributor further.

Network-exception is now in this PR. The remaining nits I'd suggest land as their own follow-ups so this PR stays scoped:

• cli-logging-expert / devx-ux-expert: Replace the bare print() in the verbose probe trace with _rich_echo(dim) / CommandLogger; add a spinner or dim echo on the probe network I/O. The current behavior is functional but bypasses the canonical output helpers (won't respect --quiet, won't be captured in integration tests).
• doc-writer: Propagate the PROXY_REGISTRY_ONLY semantic-drift note to the env-var reference and auth pages (the change in this PR documents it in the registry-proxy page; the cross-references on other pages are still pre-fix wording).

Happy to open both as separate issues once this lands — they're low-effort, low-risk, and easier to review on their own.

Test + lint status

• pytest tests/unit/test_artifactory_support.py tests/unit/install/test_artifactory_resolver.py tests/unit/deps/test_artifactory_orchestrator.py tests/unit/commands/test_install_resolve_refs.py → 184 pass (3 pre-existing env failures unchanged).
• New INCONCLUSIVE tests: 7/7 in test_artifactory_resolver.py pass.
• ruff check and ruff format --check clean on touched files.
• Broader sweep: 2668 pass, 2 pre-existing MCP-registry failures (from origin/main's 0.15.0 work, unrelated).

PR is mergeable again.

[danielmeppiel]

APM Review Panel: ship_with_followups

Deterministic boundary probe replaces fragile directory-marker heuristic, unblocking nested GitLab subgroup paths behind enterprise Artifactory proxies.

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

This PR solves a real enterprise pain point -- nested GitLab subgroups behind Artifactory VCS proxies -- with a deterministic HEAD-probe that mirrors the existing GitLab native resolver pattern. The architecture is clean (python-architect confirms), the security posture is sound (supply-chain + auth confirm allow_redirects=False and audience separation), and the fail-closed INCONCLUSIVE semantics are correct. Zero blocking findings across all 8 panelists.

The panel converges on two polish themes worth tracking post-merge: (1) verbose-output hygiene -- cli-logging, devx-ux, and security all flag the same raw print() / backtick / credential-echo surface, suggesting a single follow-up to route through CommandLogger; (2) documentation completeness -- doc-writer identified two real drift gaps (dependencies.md still says object-form required, manifest-schema.md omits // marker) plus a missing INCONCLUSIVE row in the outcome table. These are real gaps that will confuse users reading docs after the feature ships, but they are addressable in a fast follow PR without blocking this one.

Test-coverage flags one missing integration-with-fixtures test (install pipeline end-to-end through boundary probe). The unit suite is thorough at 154 tests, but the integration gap means the full parse-probe-rebuild-lockfile chain has no automated regression trap. This is the highest-signal follow-up to track given the portability-by-manifest principle surface.

Aligned with: Secure by default (allow_redirects=False prevents token forwarding to redirect targets; Mode 2 audience separation isolates proxy credentials from upstream hosts; INCONCLUSIVE fails closed); Multi-harness / multi-host (unblocks nested GitLab subgroup resolution behind Artifactory VCS proxies -- the exact topology regulated enterprises deploy); OSS / community-driven (8-commit Check Point contributor solving their own production pain; validates the regulated-airgap adoption segment with real usage); Pragmatic as npm (deterministic HEAD-probe at install time mirrors npm registry resolution mental model -- probe, resolve, fetch -- rather than relying on brittle naming conventions).

Growth signal. @chkp-roniz is now an 8-commit power contributor focused on Artifactory/proxy enterprise flows. This is the strongest signal yet that regulated-airgap buyers (Check Point Software) adopt APM in production behind enterprise proxies. Opportunity: invite to co-author an "APM in regulated CI" guide and feature a dedicated "enterprise proxy improvements" section in the next release note to attract similar buyers.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	2	Clean new resolver module mirrors GitLab pattern without structural duplication; _CandidateStatus state machine is well-designed with correct fail-closed semantics; recommend encapsulating Mode routing into a small dataclass.
CLI Logging Expert	0	1	2	Verbose probe output bypasses CommandLogger/verbose_detail, using raw print() with a non-standard prefix; error messages use markdown backticks that won't render in a terminal.
DevX UX Expert	0	1	2	Solid error ergonomics; INCONCLUSIVE error should hint at the // escape hatch like the UNRESOLVED error does.
Supply Chain Security Expert	0	0	2	Token-leak guards (allow_redirects=False, Mode 2 audience separation) are correctly implemented; no blocking security issues.
OSS Growth Hacker	0	1	1	Repeat enterprise contributor (8 commits) from Check Point validates the regulated-airgap segment; docs additions excellent; CHANGELOG entries maintainer-voice -- mine for a launch beat.
Auth Expert	0	0	2	No auth bypass or token leak. Mode 1 HTTPS-only and Mode 2 audience separation are sound. Two hardening nits only.
Doc Writer	0	3	2	registry-proxy.md page reads cleanly and CHANGELOG rework addresses prior Copilot ask; two drift gaps (apm-usage/dependencies.md, reference/manifest-schema.md) and one missing failure-mode row.
Test Coverage Expert	0	1	1	Unit coverage is thorough for all 7 surfaces; one recommended gap: no integration-with-fixtures test exercises the full install pipeline through the boundary probe.

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 integration-with-fixtures test exercising full install pipeline through boundary probe -- Install pipeline is a critical-promise surface (portability-by-manifest). Unit tests cover resolver in isolation but no automated test wires parse->probe->rebuild->lockfile end-to-end. Missing integration coverage is a regression-trap gap on a critical surface.
2. [Doc Writer] Fix drift in dependencies.md and manifest-schema.md to document // boundary marker and env-gated nested-group behavior -- Two real documentation drift gaps: dependencies.md still claims object-form is required for nested groups, and the formal manifest spec omits // entirely. Users reading docs after this ships will get contradictory guidance.
3. [CLI Logging Expert] Route verbose output through CommandLogger.verbose_detail() instead of raw print() -- Converging signal from three panelists (cli-logging, devx-ux, security). Raw print() bypasses --quiet suppression, stderr routing, and structured capture. Also eliminates the credential-echo risk flagged by supply-chain.
4. [DevX UX Expert] Add // bypass hint to the INCONCLUSIVE error message -- INCONCLUSIVE (network failure) is exactly when users most need the escape hatch. Recovery-action principle: every error names one concrete next action. The UNRESOLVED error already does this; INCONCLUSIVE should match.
5. [OSS Growth Hacker] Rewrite CHANGELOG Fixed bullet as user-facing story for release notes -- Current wording references internal function names (parse_artifactory_path, build_artifactory_archive_url). Users searching "nested GitLab subgroup" or "Artifactory 404" won't find it. Release note should tell the user story.

Architecture

classDiagram
    direction LR
    class DependencyReference {
        <<DataClass>>
        +repo_url str
        +host str
        +reference str
        +virtual_path str
        +artifactory_prefix str
        +parse(package) DependencyReference
        +from_artifactory_boundary_probe() DependencyReference
        +from_gitlab_shorthand_probe() DependencyReference
        +is_artifactory() bool
    }
    class _CandidateStatus {
        <<ValueObject>>
        +EXISTS str
        +MISSING str
        +AUTH str
        +INCONCLUSIVE str
    }
    class AuthResolver {
        <<Strategy>>
        +resolve_for_dep(dep_ref) AuthContext
    }
    class ArtifactoryRouter {
        <<Facade>>
        +should_use_proxy(dep_ref) bool
        +parse_proxy_config() tuple
    }
    class RegistryConfig {
        <<ValueObject>>
        +from_env() RegistryConfig
        +get_headers() dict
    }
    class github_host {
        <<PureUtility>>
        +iter_artifactory_boundary_candidates(segs) Iterator
        +build_artifactory_archive_url(host,prefix,owner,repo,ref) tuple
        +sanitize_token_url_in_message(pkg,host) str
    }
    class artifactory_resolver {
        <<Module>>
        +_resolve_artifactory_boundary(pkg,auth) DependencyReference
        +_candidate_archive_status(host,...) tuple
        +_proxy_probe_headers(dep,auth,mode) dict
        +_proxy_routing_target(dep) tuple
        +_rebuild_dep_ref(dep,...) DependencyReference
    }
    class gitlab_resolver {
        <<Module>>
        +_try_resolve_gitlab_direct_shorthand(pkg,auth) DependencyReference
    }
    class package_resolution {
        <<Orchestrator>>
        +resolve_parsed_dependency_reference() tuple
    }
    package_resolution ..> artifactory_resolver : delegates
    package_resolution ..> gitlab_resolver : delegates
    artifactory_resolver ..> DependencyReference : reads and rebuilds
    artifactory_resolver ..> _CandidateStatus : returns
    artifactory_resolver ..> AuthResolver : resolves Mode 1 token
    artifactory_resolver ..> RegistryConfig : resolves Mode 2 token
    artifactory_resolver ..> ArtifactoryRouter : detects proxy routing
    artifactory_resolver ..> github_host : URL generation
    gitlab_resolver ..> DependencyReference : reads and rebuilds
    class artifactory_resolver:::touched
    class _CandidateStatus:::touched
    class DependencyReference:::touched
    class github_host:::touched
    class package_resolution:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A["apm install owner/sub/repo"] --> B["install.py: resolve_parsed_dependency_reference()"]
    B --> C{"needs_gitlab_direct_shorthand_probing?"}
    C -->|yes| D["gitlab_resolver._try_resolve_gitlab_direct_shorthand"]
    C -->|no| E{"resolve_artifactory_boundary is not None?"}
    E -->|no| F["return dep_ref unchanged"]
    E -->|yes| G["artifactory_resolver._resolve_artifactory_boundary()"]
    G --> H["_proxy_routing_target(dep_ref)"]
    H --> I{"routes through proxy?"}
    I -->|no| F
    I -->|yes| J["iter_artifactory_boundary_candidates(path_segments)"]
    J --> K{"len(candidates) == 1?"}
    K -->|yes| F
    K -->|no| L["_proxy_probe_headers(dep_ref, auth, is_mode_1)"]
    L --> M["Loop: for each candidate shallow-first"]
    M --> N["[NET] _candidate_archive_status: HEAD probe archive URLs"]
    N --> O{"status?"}
    O -->|EXISTS| P["_rebuild_dep_ref or return original"]
    O -->|MISSING| Q["continue to next candidate"]
    O -->|AUTH| Q
    O -->|INCONCLUSIVE| Q
    Q --> M
    M -->|exhausted| R{"failure mode?"}
    R -->|all INCONCLUSIVE| S["raise ValueError: network unreachable"]
    R -->|all AUTH| T["raise ValueError: authentication problem"]
    R -->|mixed| U["raise ValueError: boundary unresolved"]
    P --> V["return (dep_ref, direct_virtual_resolved)"]

Loading

sequenceDiagram
    participant User
    participant Install as install.py
    participant PkgRes as package_resolution
    participant ArtRes as artifactory_resolver
    participant Router as ArtifactoryRouter
    participant RegCfg as RegistryConfig
    participant GHHost as github_host
    participant Proxy as Artifactory Proxy

    User->>Install: apm install group/sub/repo
    Install->>PkgRes: resolve_parsed_dependency_reference(pkg)
    PkgRes->>ArtRes: _resolve_artifactory_boundary(pkg, auth, dep_ref)
    ArtRes->>Router: should_use_proxy(dep_ref)
    Router-->>ArtRes: True (PROXY_REGISTRY_ONLY=1)
    ArtRes->>Router: parse_proxy_config()
    Router-->>ArtRes: (host, prefix, scheme)
    ArtRes->>GHHost: iter_artifactory_boundary_candidates(segs)
    GHHost-->>ArtRes: [(pfx,group,sub,repo/None), (pfx,group,sub/repo,None)]
    ArtRes->>RegCfg: from_env().get_headers() [Mode 2]
    RegCfg-->>ArtRes: {Authorization: Bearer proxy-token}
    loop each candidate shallow-first
        ArtRes->>GHHost: build_artifactory_archive_url(host,pfx,owner,repo,ref)
        GHHost-->>ArtRes: (url1, url2, url3, url4, url5)
        ArtRes->>Proxy: HEAD url1 (allow_redirects=False)
        Proxy-->>ArtRes: 404
        ArtRes->>Proxy: HEAD url2 (allow_redirects=False)
        Proxy-->>ArtRes: 302 (EXISTS)
    end
    ArtRes->>ArtRes: _rebuild_dep_ref(dep, is_mode_1=False)
    ArtRes-->>PkgRes: rebuilt DependencyReference
    PkgRes-->>Install: (dep_ref, direct_virtual_resolved=True)

Loading

Recommendation

Ship this PR. Zero blocking findings, sound architecture, correct security posture, and it unblocks a real enterprise workflow from a proven contributor. Track the integration-test gap and the two doc-drift fixes as immediate follow-ups -- the maintainer should open a tracking issue for those three items before or shortly after merge. The verbose-output and error-wording polish can land in the same follow-up pass.

---

Full per-persona findings

Python Architect

• [recommended] _proxy_routing_target mixes routing-detection with config resolution; consider a ProxyTarget dataclass at src/apm_cli/install/artifactory_resolver.py:150
  Dataclass for structured data flowing between components. 4-tuple (host, prefix, scheme, is_mode_1) flows through three functions; positional unpacking at every call site. Future-proofs against adding modes.
• [recommended] _CandidateStatus should be an enum.StrEnum not a class with string constants at src/apm_cli/install/artifactory_resolver.py:61
  Equality comparisons are raw string checks with no type-checker help. enum.StrEnum (3.11+) gives exhaustiveness checking and prevents typos. Standard Python discriminated-union pattern.
• [nit] from_artifactory_boundary_probe classmethod placement is correct
  Mirrors from_gitlab_shorthand_probe; consistent with codebase convention.
• [nit] _proxy_routing_target placement in the resolver is correct
  Couples Artifactory detection + proxy detection + config resolution - all install-time concerns.

CLI Logging Expert

• [recommended] Verbose output uses raw print() instead of logger.verbose_detail() at src/apm_cli/install/artifactory_resolver.py:242
  Every other verbose path in src/apm_cli/install/ routes through logger.verbose_detail(). New resolver falls back to print(), bypassing CommandLogger lifecycle (no --quiet suppression, no stderr routing, no structured capture).
• [nit] Verbose prefix 'artifactory-resolve:' doesn't use STATUS_SYMBOLS convention at src/apm_cli/install/artifactory_resolver.py
• [nit] Error constant uses double-backtick markdown notation (//) that won't render in a terminal at src/apm_cli/install/artifactory_resolver.py:50

DevX UX Expert

• [recommended] INCONCLUSIVE (network) error omits the // bypass hint that the UNRESOLVED error includes at src/apm_cli/install/artifactory_resolver.py:275
  INCONCLUSIVE is exactly when the user needs the // escape hatch most (transient outage). Recovery-action principle: every error names one concrete next action.
• [nit] Docs use 'Mode 1' / 'Mode 2' labels that are internal jargon at docs/src/content/docs/enterprise/registry-proxy.md
• [nit] The // notation only documented in enterprise/registry-proxy page, not in cli-commands.md at docs/src/content/docs/reference/cli-commands.md

Supply Chain Security Expert

• [nit] verbose print echoes host/path to stdout without sanitization (user:pass@ in PROXY_REGISTRY_URL could leak) at src/apm_cli/install/artifactory_resolver.py:243
• [nit] INCONCLUSIVE info mixed with MISSING candidates is silently dropped from the final error at src/apm_cli/install/artifactory_resolver.py:270

OSS Growth Hacker

• [recommended] CHANGELOG entries are maintainer-facing; rewrite Fixed bullet as user-facing story for next release note at CHANGELOG.md
  'parse_artifactory_path, build_artifactory_archive_url...' won't surface for users Googling 'nested GitLab subgroup' or 'Artifactory 404'.
• [nit] New troubleshooting rows in registry-proxy.md are SEO surface; link from top-level troubleshooting at docs/src/content/docs/enterprise/registry-proxy.md

Auth Expert

• [nit] Mode 1 hard-codes scheme='https' without upstream enforcement in resolver module at src/apm_cli/install/artifactory_resolver.py:170
  _proxy_routing_target constructs the target URL with scheme='https' which is correct, but nothing in the resolver module asserts or rejects an http:// upstream if one were passed in.
• [nit] Duck-typed getattr token access instead of typed AuthContext.token attribute at src/apm_cli/install/artifactory_resolver.py:138
  Bypasses static type checking; a refactor that renames the token field would silently produce unauthenticated probes rather than raising AttributeError at development time.

Doc Writer

• [recommended] DRIFT: apm-usage/dependencies.md still says nested-group shorthand requires object form; PR makes it work under PROXY_REGISTRY_ONLY at packages/apm-guide/.apm/skills/apm-usage/dependencies.md:136
• [recommended] DRIFT: manifest-schema.md (formal spec) does not document the // boundary marker or env-gated nested-group behavior at docs/src/content/docs/reference/manifest-schema.md:307
• [recommended] registry-proxy.md boundary-probe outcome table omits INCONCLUSIVE / transport-error fail-closed mode at docs/src/content/docs/enterprise/registry-proxy.md:226
• [nit] // opt-out not cross-referenced from outcome table at docs/src/content/docs/enterprise/registry-proxy.md
• [nit] Positive ack: CHANGELOG rework correctly distinguishes explicit-FQDN vs bare-shorthand parse-time defaults

Test Coverage Expert

• [recommended] No integration-with-fixtures test exercises apm install through Artifactory boundary probe end-to-end at tests/integration/test_install_artifactory_boundary.py
  Install pipeline is critical-promise per tier-floor matrix. Unit tests cover resolver in isolation; no integration test wires parse->probe->rebuild->lockfile.
  Proof (missing at): tests/integration/test_install_artifactory_boundary.py::test_install_nested_group_dep_through_boundary_probe -- proves: apm install through registry proxy resolves nested-group repos and writes correct lockfile entry [devx,portability-by-manifest]
• [nit] No test asserts Mode 2 boundary probe uses http:// scheme when PROXY_REGISTRY_ALLOW_HTTP=1 at tests/unit/install/test_artifactory_resolver.py
  Proof (missing at): tests/unit/install/test_artifactory_resolver.py::test_mode_2_probe_uses_http_scheme_when_allow_http_set -- proves: Boundary probe over http:// isolated networks uses correct transport scheme [devx]

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

[danielmeppiel]

Shepherd: ready to merge

This PR has been shepherded to convergence. All wave-1 CEO follow-ups folded into this branch; CI is fully green on the latest push.

Folded follow-ups (5 / 5)

#	Item	Commit(s)
FU-1	Integration trap: probe -> rebuild -> yaml entry pipeline	987f2a5, c9d85e8
FU-2	Doc drift: scope the "object form required" claim and note the registry-proxy exception (dependencies.md + manifest-schema.md)	d359a0a0
FU-3	Route resolver verbose output through CommandLogger.verbose_detail instead of print	301e0a92
FU-4	Cite the // boundary-bypass escape hatch in the INCONCLUSIVE error message	301e0a92
FU-5	CHANGELOG rewrite in user voice (leads with user-observable behavior, names both invocation shapes, surfaces // escape)	d2041cde

Mutation-break gate

Confirmed on the new INCONCLUSIVE-hint trap: removing the // bypass clause from the error message makes test_transport_errors_raise_inconclusive_with_bypass_hint fail with a regex mismatch. The new integration test is a real regression trap, not tautological assertion.

Lint evidence

CI-mirror chain silent locally on c9d85e83:

• ruff check src/ tests/ (clean)
• ruff format --check src/ tests/ (clean)
• pylint R0801 --min-similarity-lines=10 rated 10.00/10
• scripts/lint-auth-signals.sh clean

CI evidence

All required checks pass on c9d85e83: Lint, Build & Test Shard 1/2 (Linux), Coverage Combine, APM Self-Check, PR Binary Smoke, Analyze (actions), Analyze (python), CodeQL, NOTICE Drift Check, Merge Gate, license/cla, build (deploy skipped as expected on PRs).

CodeQL recovery (1 iteration): wave-1 added substring URL assertions in the new integration test, which tripped py/incomplete-url-substring-sanitization and also violated the repo's tests/** URL-assertion convention (urllib.parse, never substring). c9d85e83 refactored those assertions to urlparse() exact-component matches.

Copilot signals

0 actionable inline comments from Copilot since the last shepherd checkpoint (57a3c02). Drained.

Deferred

None. All wave-1 follow-ups were in-scope quality-bar items (tests, docs, CHANGELOG, ergonomics) and folded into this branch.

Authorship

Each new commit carries dual Co-authored-by: trailers preserving @chkp-roniz as a co-author alongside the shepherd. The branch remains on the contributor's fork; maintainer-can-modify was already enabled by the author.

cc @chkp-roniz @danielmeppiel