fix(install): actionable hint when self-hosted GitLab host swallows subpath#2074
Conversation
…ubpath (closes microsoft#2066) apm install <fqdn>/org/repo/subpath#ref only splits repo from subpath on hosts already recognised as GitLab-class (gitlab.com, or a host declared via GITLAB_HOST/APM_GITLAB_HOSTS). An unrecognised self-hosted FQDN instead folds the whole remaining path into repo_url, so git ls-remote fails against a bogus repo path and the user sees a generic 'not accessible or doesn't exist' error that looks like a permissions problem rather than a missing-config one. Add _generic_host_ambiguous_subpath_hint() to surface the real cause and point at GITLAB_HOST/APM_GITLAB_HOSTS (mirroring the existing GITHUB_HOST pattern for GHES), or the git:+path: object form as a fallback for non-GitLab generic hosts.
There was a problem hiding this comment.
Pull request overview
Improves apm install UX for shorthand Git references on unrecognized self-hosted GitLab hosts by emitting an actionable hint (set GITLAB_HOST / APM_GITLAB_HOSTS) when an ambiguous multi-segment repo_url likely swallowed a subpath into the repository portion and then failed validation.
Changes:
- Add
_generic_host_ambiguous_subpath_hint()to produce a targeted failure reason for ambiguous multi-segment paths on non-GitHub/ADO/GitLab-recognized hosts. - Wire the new hint into the install validation-failure reason chain ahead of the generic “not accessible” fallback.
- Add unit + CLI-level tests for the new hint behavior and update
CHANGELOG.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/install/validation.py |
Adds the new hint helper and GitHub/GitLab host checks to decide when to emit it. |
src/apm_cli/commands/install.py |
Plumbs the new hint into _resolve_package_references() failure handling. |
tests/unit/install/test_validation_error_handling.py |
Adds unit coverage for the hint helper and its guard branches. |
tests/unit/test_install_command.py |
Adds an end-to-end CLI test asserting the hint appears for an unrecognized self-hosted GitLab-like host. |
CHANGELOG.md |
Adds an Unreleased/Fixed entry describing the improved failure reason. |
…g format - Add str | None return annotation to _generic_host_ambiguous_subpath_hint, matching the annotated-helper convention elsewhere in this module. - Filter empty path segments before counting, so a trailing/double slash in repo_url can't inflate the segment count and misfire the hint. - Reformat the CHANGELOG entry to the repo convention: ends with the PR number, credit inline as "(by @user; closes #issue) (#pr)".
|
@microsoft-github-policy-service agree company="Globo" |
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 0 | 2 | Clean leaf-function addition slotted correctly into the existing failure-reason chain; no architectural concerns. |
| CLI Logging Expert | 0 | 1 | 3 | Good actionable hint replacing a misleading error; the message is too long for a single error line and could be tightened. |
| DevX UX Expert | 0 | 2 | 3 | Hint is a massive UX improvement over bare 'not accessible'; message length and GHES blind spot are the main polish items. |
| Supply Chain Security Expert | 0 | 0 | 0 | Pure diagnostic message on the failure path -- no security-relevant behavior change, no credential leakage, no injection surface. |
| OSS Growth Hacker | 0 | 0 | 2 | Strong friction-removal for enterprise GitLab users; hint text is actionable and copy-pasteable. Minor polish opportunities. |
| Auth Expert | 0 | 0 | 1 | Purely diagnostic hint on the failure path; no auth flow, credential resolution, or token selection is altered. Clean from an auth perspective. |
| Doc Writer | 0 | 1 | 2 | CHANGELOG entry is accurate; install-failures.md already covers GITLAB_HOST but misses the subpath-swallow symptom; attribution punctuation is inconsistent. |
| Test Coverage Expert | 0 | 1 | 1 | Hint function and CLI wiring are well covered at unit tier; one nit for APM_GITLAB_HOSTS guard branch parametrization. Ship. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [DevX UX Expert] Add GITHUB_HOST mention to the hint for self-hosted GHES users -- Four panelists independently flagged this. The current hint actively steers GHES users toward the wrong env var (GITLAB_HOST). Small code change, high correctness impact.
- [CLI Logging Expert] Split the ~430-char hint into a two-beat pattern: short error line + info/verbose detail line -- Consensus across CLI-logging, DevX UX, and growth hacker. Current wall-of-text buries the actionable fix. Two-beat pattern matches npm/cargo/pip conventions.
- [Doc Writer] Add subpath-swallow symptom to install-failures.md GitLab section -- Users who hit this error before upgrading, or who consult docs independently, would find no mention of the FQDN-folding failure mode. One-liner addition to existing section.
- [OSS Growth Hacker] File a docs issue for a 'Using APM with self-hosted GitLab' FAQ page -- High-leverage conversion content for enterprise evaluators. Covers GITLAB_HOST, APM_GITLAB_HOSTS, nested groups, and the subpath-swallow pitfall in one place.
- [Test Coverage Expert] Add parametrized test entry for APM_GITLAB_HOSTS guard branch at hint level -- Test-coverage-expert confirmed unit tests pass (10/10) but APM_GITLAB_HOSTS branch is only exercised via composed call through is_gitlab_hostname. Single parametrize entry makes coverage self-documenting. Low effort.
Architecture
classDiagram
direction LR
class DependencyReference {
<<ValueObject>>
+host str
+repo_url str
+is_local bool
+is_virtual bool
+reference str
+is_azure_devops() bool
+is_virtual_subdirectory() bool
}
class validation {
<<Module>>
+_local_path_failure_reason(dep_ref) str or None
+_generic_host_ambiguous_subpath_hint(dep_ref) str or None
+_validate_package_exists(package, ...) bool
}
class github_host {
<<Module>>
+is_github_hostname(h) bool
+is_gitlab_hostname(h) bool
+is_ado_auth_failure_signal(resp) bool
}
class install_command {
<<Module / Orchestrator>>
+_resolve_package_references(...)
}
install_command ..> validation : calls reason chain
validation ..> DependencyReference : reads
validation ..> github_host : delegates host classification
class validation:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A["install_command._resolve_package_references"] --> B{"_validate_package_exists\nreturns falsy?"}
B -->|no| C["validation_pass -- package OK"]
B -->|yes| D["_local_path_failure_reason(dep_ref)"]
D -->|reason| H["emit reason in error"]
D -->|None| E["[NEW] _generic_host_ambiguous_subpath_hint(dep_ref)"]
E -->|reason| H
E -->|None| F{"is_subdir_ref_chain?"}
F -->|yes| G["emit four-step probe hint"]
F -->|no| I["emit generic 'not accessible' error"]
style E fill:#fff3b0,stroke:#d47600
Recommendation
Ship this PR as-is. It replaces a genuinely harmful error message with an actionable hint for a real enterprise pain point, contributed by an external community member. All tests pass, no security/auth/perf surface is touched, and the architecture is clean. The GHES blind spot and message-density polish are real but non-blocking -- they improve an already-good hint rather than fixing a regression. File the GHES and message-split items as fast follow-ups; file the docs FAQ as a separate issue for the next docs cycle. Merging now rewards a community contributor promptly and unblocks enterprise GitLab users immediately.
Full per-persona findings
Python Architect
-
[nit] Missing type hint on dep_ref parameter at
src/apm_cli/install/validation.py:155
The codebase standard says Type hints on all public APIs. _local_path_failure_reason is equally untyped so this is a pre-existing gap, but since the function is new, adding it now is cheap and helps IDE navigation.
Suggested:def _generic_host_ambiguous_subpath_hint(dep_ref: 'DependencyReference') -> str | None: -
[nit] Consider GITHUB_HOST in the hint message alongside GITLAB_HOST at
src/apm_cli/install/validation.py:176
The hint mentions GitHub, ADO, and GitLab as the three recognised families but only tells the user about GITLAB_HOST/APM_GITLAB_HOSTS. A GHES instance not declared via GITHUB_HOST would hit the same code path.
Suggested: Append 'For self-hosted GitHub Enterprise, use GITHUB_HOST instead.' to the hint string.
CLI Logging Expert
-
[recommended] Hint message is ~430 chars -- too dense for a single error line in a terminal at
src/apm_cli/install/validation.py:174
validation_fail renders this as one [x] error line. At 500+ chars with the package prefix, it becomes a wall of text that buries the actionable fix. A two-line approach would be much easier to scan.
Suggested: Split into a short actionable reason and push the diagnostic explanation to a _rich_info or verbose-only detail line. -
[nit] Hint embeds actionable shell command ('set GITLAB_HOST=...') in prose without backtick-quoting it at
src/apm_cli/install/validation.py:183
Wrapping the env var assignment in backticks would visually distinguish the command from surrounding explanation, matching the pattern used elsewhere in APM error messages. -
[nit] 'isn't recognised' contraction -- 'not recognised' is slightly more scannable in error dumps at
src/apm_cli/install/validation.py:179
Minor CLI polish. -
[nit] Missing '--verbose' breadcrumb in the new hint path at
src/apm_cli/commands/install.py:549
The generic fallback appends 'run with --verbose for auth details' when not already verbose. The new hint replaces that path and loses the nudge toward verbose output.
DevX UX Expert
-
[recommended] Hint only mentions GitLab but the same failure hits self-hosted GHES users who have not set GITHUB_HOST at
src/apm_cli/install/validation.py:180
The guard clause checks is_github_hostname (false for unregistered GHES) so the hint fires for GHES users too, then actively steers them toward GITLAB_HOST -- the wrong env var.
Suggested: Add 'For GitHub Enterprise Server, set GITHUB_HOST={host}.' to the hint. -
[recommended] 429-char single-line hint is too dense; split into diagnosis + action beats at
src/apm_cli/install/validation.py:180
npm, cargo, pip all use a two-beat pattern: one short line saying WHAT, then a separate help line saying WHAT TO DO. The current hint packs parser internals, fix, alternative, and GitLab jargon into one wall of text. -
[nit] Hint leaks internal parser taxonomy ('isn't recognised as GitHub, Azure DevOps, or GitLab') at
src/apm_cli/install/validation.py:182
Users don't think in APM host classifier buckets; 'not a known Git host' is cleaner. -
[nit] 'for nested groups' is GitLab-specific jargon in a general-purpose hint at
src/apm_cli/install/validation.py:185
The concept is 'monorepo subpath splitting'; 'nested groups' is GitLab-internal and confuses GHES/Gitea users. -
[nit] 'git: + path: in apm.yml' fallback advice doesn't show the syntax at
src/apm_cli/install/validation.py:186
Recovery guidance should be one copy-paste away.
Supply Chain Security Expert
No findings.
OSS Growth Hacker
-
[nit] Hint message is developer-internal in tone -- 'repo/subpath resolution for nested groups' is jargon at
src/apm_cli/install/validation.py:176
A self-hosted GitLab user hitting this for the first time doesn't know APM's internal concepts. User-goal-oriented phrasing converts better.
Suggested: Consider:'{host}' is not recognised as a known Git host. If this is a self-hosted GitLab instance, set GITLAB_HOST={host} (or add it to APM_GITLAB_HOSTS) and re-run. Otherwise, use git: + path: in apm.yml. -
[nit] CHANGELOG entry leads with input shape rather than user outcome at
CHANGELOG.md:10
Leading with the outcome is more scannable for release note readers.
Suggested: 'Self-hosted GitLab installs with subpaths no longer fail with a misleading "not accessible" error; APM now suggests setting GITLAB_HOST / APM_GITLAB_HOSTS.'
Auth Expert
- [nit] Hint mentions GITLAB_HOST but not GITHUB_HOST for self-hosted GHES that also swallows subpaths at
src/apm_cli/install/validation.py:183
A self-hosted GHES instance not yet registered via GITHUB_HOST would hit the same codepath. Mentioning GITHUB_HOST as a second possibility would make the hint complete.
Suggested: Append: 'If it is a GitHub Enterprise Server, set GITHUB_HOST={host} instead.'
Doc Writer
-
[recommended] install-failures.md GitLab section should document the subpath-swallow symptom fixed by this PR at
docs/src/content/docs/troubleshooting/install-failures.md:63
Readers who hit this error before upgrading, or who consult docs independently, would find no mention of the FQDN-folding failure mode.
Suggested: Append: 'If apm install host/org/repo/subpath#ref reports not accessible with no auth error, APM may have folded the full path into a single repo URL -- set GITLAB_HOST= to enable subpath resolution.' -
[nit] CHANGELOG attribution punctuation differs from project pattern -- double parenthetical vs single at
CHANGELOG.md:16
'(by @rrazvd; closes [BUG] apm install does not support subpath shorthand for GitLab hosts (non-github.com) #2066) (fix(install): actionable hint when self-hosted GitLab host swallows subpath #2074)' vs the project's '(by @author, #PRNUM)' pattern.
Suggested: Change to '(by @rrazvd, fix(install): actionable hint when self-hosted GitLab host swallows subpath #2074)' -
[nit] 'unrecognised' (British) -- consistent with rest of CHANGELOG, no action needed at
CHANGELOG.md:13
Informational only.
Test Coverage Expert
-
[nit] APM_GITLAB_HOSTS guard branch not exercised in hint-level tests (only GITLAB_HOST is tested) at
tests/unit/install/test_validation_error_handling.py:245
test_selfhosted_host_already_registered_via_env_returns_none only sets GITLAB_HOST. The underlying is_gitlab_hostname is tested for APM_GITLAB_HOSTS in test_github_host.py so the composed guard works. A single parametrize entry at the hint level would make coverage self-documenting.
Suggested: Add parametrized sibling: setenv('APM_GITLAB_HOSTS','gitlab.company.com') with GITLAB_HOST unset, assert returns None.
Proof (passed):tests/unit/install/test_validation_error_handling.py::TestGenericHostAmbiguousSubpathHint::test_selfhosted_host_already_registered_via_env_returns_none-- proves: Hint suppressed when registered via GITLAB_HOST; APM_GITLAB_HOSTS path untested at hint level [devx]
assert _generic_host_ambiguous_subpath_hint(self._dep()) is None -
[recommended] CLI test is unit-tier (mocks validation); no integration-with-fixtures path exists for live self-hosted install failure at
tests/unit/test_install_command.py:473
The error-wording cascade reachability tier floor is integration-with-fixtures. The CLI test proves wiring is intact via CliRunner+mock but is unit-tier. A true integration test would require a live self-hosted GitLab -- impractical. The pragmatic unit coverage is sufficient given the hint is a pure function of dep_ref fields.
Suggested: Acceptable as-is. If a generic-host fixture is ever added, extend.
Proof (passed):tests/unit/test_install_command.py::TestValidationFailureReasonMessages::test_selfhosted_gitlab_like_host_hints_gitlab_host_env-- proves: Hint text surfaces through CLI output when validation fails for unrecognised self-hosted FQDN [devx]
assert 'GITLAB_HOST=gitlab.company.com' in output
Performance Expert -- inactive
None of the changed files touch the cache, deps, pipeline, resolve, or perf-instrumentation paths, and the new code runs only after a failed ls-remote on the error-reporting branch -- no hot-path impact.
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
CI Lint fix needed -- file-length guardrailThe Here is the minimal patch to bring it back to exactly 2100 lines. It uses a shorter import alias so ruff keeps both the import and the call on one line: diff --git a/src/apm_cli/commands/install.py b/src/apm_cli/commands/install.py
@@ -80,12 +80,11 @@
# Re-export validation leaf helpers so that existing test patches like
# @patch("apm_cli.commands.install._validate_package_exists") keep working.
-# _validate_and_add_packages_to_apm_yml stays here (not moved) because it
-# calls _validate_package_exists and _local_path_failure_reason via module-
-# level name lookup -- keeping it co-located means @patch on this module
-# intercepts those calls without test changes.
+# _validate_and_add_packages_to_apm_yml stays co-located (module lookup keeps @patch working).
+from apm_cli.install.validation import (
+ _generic_host_ambiguous_subpath_hint as _ambiguous_subpath_hint,
+)
from apm_cli.install.validation import (
- _generic_host_ambiguous_subpath_hint,
_local_path_failure_reason,
_local_path_no_markers_hint, # noqa: F401 ...
_validate_package_exists,
@@ -543,9 +542,7 @@
else:
- reason = _local_path_failure_reason(dep_ref)
- if not reason:
- reason = _generic_host_ambiguous_subpath_hint(dep_ref)
+ reason = _local_path_failure_reason(dep_ref) or _ambiguous_subpath_hint(dep_ref)All 150 affected tests pass with this patch. Please push this (or an equivalent) to unblock the Lint gate. |
sergio-sisternes-epam
left a comment
There was a problem hiding this comment.
Code is correct and well-tested (11 new tests covering all guard branches). The diagnostic hint for self-hosted GitLab users is exactly what is needed. Approving -- see the comment above for a mechanical lint fix the author needs to push to clear the file-length CI gate.
Extract the new hint import behind an alias and fold the two-step reason lookup into one line (or-chain), per the collaborator's suggested patch on the PR. Net effect: back to exactly 2100 lines. Also drop a redundant test assertion that CodeQL flagged as an incomplete-URL-substring-sanitization pattern -- the preceding assertion on the same reason string already covers the same substring, so the extra check added nothing beyond triggering the false positive.
Head branch was pushed to by a user without write access
|
Thanks for the quick diagnosis @sergio-sisternes-epam! Pushed in
All previously-passing tests still pass locally, lint/format clean. |
Description
apm install <fqdn>/org/repo/subpath#refonly splitsrepofromsubpathon hosts already recognised as GitLab-class: literallygitlab.com, or a host explicitly declared viaGITLAB_HOST/APM_GITLAB_HOSTS. When the host is an unrecognised self-hosted GitLab FQDN (the common case: a company GitLab that the user hasn't told APM about yet), the whole remaining path is folded intorepo_urlat parse time -- there's no ambiguous-boundary probing to fall back on, because that probe itself requires the host to already be GitLab-classified. The resultinggit ls-remotethen fails against a bogus repo path, and the user sees:which reads like a permissions/network problem, not a missing-config one.
Fixes #2066.
Investigation note
The issue as filed reproduces with
gitlab.comin the example, and cites PR #1684's "gitlab.com excluded" comment as the root cause. I verified end-to-end (including against the realgitlab.comhost, and against the exactapm==0.23.1build cited in the issue) that thegitlab.comcase already works correctly today -- that exclusion is in the GHES-specific host-detection branch, which is deliberately not the code path GitLab uses (GitLab has its own dedicated boundary-probe resolver,install/gitlab_resolver.py, already covered bytests/unit/install/test_gitlab_resolver.py).The actual reproducible gap is self-hosted GitLab without
GITLAB_HOST/APM_GITLAB_HOSTSconfigured -- confirmed end-to-end against a real self-hosted instance (gitlab.gnome.org) both before and after this change.Approach
Rather than trying to auto-detect arbitrary self-hosted FQDNs as GitLab-class (higher risk: APM can't safely guess an unknown host's namespace-depth convention), this keeps the existing GHES-style opt-in model (
GITHUB_HOSTalready works the same way for GitHub Enterprise) and fixes the UX gap: when a generic, unrecognised host fails validation with an ambiguous multi-segment path, the error message now explains why and points at the fix.Before:
After:
Implementation
src/apm_cli/install/validation.py: new_generic_host_ambiguous_subpath_hint(dep_ref), following the existing_local_path_failure_reasonpattern. Fires only when: the dep isn't local/virtual, the host isn't GitHub/ADO/GitLab-recognised, andrepo_urlhas more than 2 path segments (i.e. genuinely ambiguous -- a plainowner/repoon a generic host is unaffected).src/apm_cli/commands/install.py: wires the new helper into the existing failure-reason chain in_resolve_package_references, ahead of the generic "not accessible" fallback.gitlab.com, env-registered self-hosted host, two-segment path) plus one CLI-level test asserting the hint text end-to-end throughapm install.CHANGELOG.md:[Unreleased] / Fixedentry crediting this fix to [BUG] apm install does not support subpath shorthand for GitLab hosts (non-github.com) #2066.Type of change
Testing
gitlab.gnome.org) reproducing the failure before this change and confirming the fix after, plus theGITLAB_HOST-configured success pathuv run pytest tests/unit tests/test_console.py; the small number of pre-existing failures intest_install_target_copilot_app_e2e.py,test_install_target_copilot_cowork_e2e.py,test_mcp_integrator_install_flow.py/test_mcp_integrator_install_phase3.py, and two terminal-width-sensitive assertions intest_install_command.pyreproduce identically onmainwithout this change)ruff check/ruff format --checkcleanSpec conformance (OpenAPM v0.1)
apm-spec-waiver: error-message-only change, no install/resolution behavior delta
How to test