docs: fix CLI reference drift from consistency reports #140 and #152#161
Merged
Conversation
4 tasks
Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix CLI reference drift from consistency reports
docs: fix CLI reference drift from consistency reports #140 and #152
Mar 4, 2026
danielmeppiel
pushed a commit
to chkp-roniz/apm
that referenced
this pull request
May 27, 2026
CodeQL flagged the substring assertions on entry['git'] (py/incomplete-url-substring-sanitization, high severity, alert microsoft#161 on PR microsoft#1472) and they also violated the repo's test convention: URL assertions in tests/** must use urllib.parse, never substring (.github/instructions/ tests.instructions.md). Switch to urlparse() + exact-match on hostname and ordered path segments. Same coverage (proxy host + artifactory/<key> prefix + probed owner/repo + virtual path), stricter assertion (a spoofed host containing 'art.example.com' as a substring would no longer pass), and CodeQL goes quiet. Co-authored-by: chkp-roniz <35386615+chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
pushed a commit
that referenced
this pull request
May 27, 2026
#1472) * fix(artifactory): deterministic boundary probe for nested GitLab paths Mirrors the native GitLab resolver pattern -- but uses HEAD on Artifactory archive URLs as the existence signal, so no separate metadata API is needed. Covers both routing modes: Mode 1 -- explicit FQDN deps (host/artifactory/key/owner/repo/...). Mode 2 -- bare shorthand under PROXY_REGISTRY_URL+PROXY_REGISTRY_ONLY. The resolver walks candidate (owner, repo, virtual_path) splits shallow-first and locks in the first one whose archive responds 2xx-3xx. For unambiguous paths it returns the parse-time ref unchanged. When every candidate is rejected it raises -- distinguishing "missing repo" from auth (401/403) errors so users get an actionable hint instead of silently anchoring on a wrong guess. ``allow_redirects=False`` on the HEAD call keeps the Bearer token from leaking cross-host. Removes the parse-time marker-segment heuristic (_VIRTUAL_PATH_ROOT_SEGMENTS / _ARTIFACTORY_VIRTUAL_MARKERS / _ARTIFACTORY_VIRTUAL_FILE_EXTENSIONS) that previously decided the boundary based on hard-coded directory names like ``skills/`` or ``prompts/``. Parse-time now defaults to all-as-repo with a structural file-extension rule on the last segment; the install-time resolver is the authoritative source of the boundary. Also fixes a pre-existing URL-roundtrip bug where ``to_github_url`` -> ``parse`` folded the ``artifactory/<key>`` prefix into ``repo_url``, causing the downloader to build double-prefixed archive URLs and 404. ``_validate_url_repo_path`` now strips the prefix before returning the bare ``owner/repo`` slug. * fix(artifactory): address Copilot review feedback on PR #1472 - parse_artifactory_path: reject ``owner//virtual`` (empty segment before the explicit boundary) instead of falling through and returning an empty ``repo`` slug. - _proxy_routing_target: thread the proxy ``scheme`` (https/http) through the resolver into ``build_artifactory_archive_url`` so installs that intentionally route through an ``http://`` proxy (under ``PROXY_REGISTRY_ALLOW_HTTP=1``) probe over the same transport. - _CandidateStatus.EXISTS: update inline comment to say ``2xx or 3xx`` (redirect-inclusive), matching the actual classification in ``_candidate_archive_status``. - CHANGELOG: split the parse-time-behavior note into explicit-FQDN vs bare-shorthand modes (the all-as-repo default applies only under ``PROXY_REGISTRY_ONLY``; Mode 1 stays shallow). - package_resolution: clarify the ``direct_virtual_resolved`` docstring -- GitLab path gates on ``is_virtual``, Artifactory path gates on probe rebuild; both signal "persist as structured ``git:`` + ``path:`` entry". - Add two regression tests for the empty-segment edge case (parser + iterator). * test(artifactory): add regression traps for boundary resolver Adds focused regression-trap tests for the new install-pipeline boundary resolver (#1472) covering the contracts a future refactor could silently break: - allow_redirects=False on every HEAD probe (token-leak guard against proxy-issued cross-host redirects) - Mode 2 Authorization header is sourced from RegistryConfig (proxy bearer), never from AuthResolver (which returns the github.com PAT and would leak it to the proxy host) - 403 (like 401) classifies as AUTH; mixed 401/non-auth-4xx demotes the candidate set to MISSING so the user-facing error stays accurate (avoids misreporting a missing repo as auth failure) - 429 and other non-{401,403} 4xx are MISSING, not AUTH (guards against accidental broadening of the AUTH set) - _split_owner_repo subgroup folding for 3+ segment GitLab boundaries (group/sub1/sub2/repo correctly folds to owner=group, repo=sub1/sub2/repo) Both mutation-break gates were exercised: flipping allow_redirects to True and rewiring Mode 2 to AuthResolver each made the corresponding test fail; restoring the source code made them pass. Also applies ruff format pass to bring three files in line with the canonical formatter (pre-existing drift on the PR branch that would have failed CI lint check). Co-authored-by: chkp-roniz <chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(artifactory): route resolver verbose output through CommandLogger + cite // bypass on inconclusive Folds two panel follow-ups on PR #1472: - Verbose probe output now flows through CommandLogger.verbose_detail() when a logger is available, so it honors the shared rich/no-rich console path and respects -v/--verbose self-gating like every other install-time detail emitter. Falls back to print() for callers that pass verbose=True without threading a logger (legacy unit tests). - The INCONCLUSIVE error message (transport error on every candidate URL) now cites the // boundary marker, matching the wording already present on the UNRESOLVED message. Users who hit a flaky proxy now learn about the explicit-boundary escape hatch from the message they actually saw, not just the parallel one. addresses CEO follow-ups FU-3 and FU-4 Co-authored-by: chkp-roniz <35386615+chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(artifactory): note proxy-probe lifts nested-shorthand ambiguity Folds the doc-drift follow-up from the apm-review-panel CEO on PR #1472. Both pages claimed that nested-group GitLab repos require the object form because shorthand is ambiguous on >2-segment paths. That claim is still true for direct (non-proxy) GitLab but is no longer true for deps that route through a registry proxy: this PR adds an install-time boundary probe that HEAD-walks candidate splits against the proxy and deterministically resolves the repo/virtual-path boundary, so nested shorthand works without the object form. - packages/apm-guide/.apm/skills/apm-usage/dependencies.md: add the registry-proxy exception sentence to the existing nested-group note. - docs/src/content/docs/reference/manifest-schema.md: scope the object-form REQUIRED clause to direct nested-group access and link the registry-proxy guide for the proxy-routed exception. addresses CEO follow-up FU-2 Co-authored-by: chkp-roniz <35386615+chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(changelog): rewrite #1472 fixed bullet in user voice Folds the CEO follow-up to make the Fixed entry release-notes-ready. The previous wording named internal symbols (parse_artifactory_path, build_artifactory_archive_url, _detect_virtual_package, ArtifactoryOrchestrator ._split_owner_repo) -- accurate but unreadable for a release-notes reader. New wording: leads with what the user can now do (apm install against a proxy works for nested-group repos), names the concrete invocation shapes (explicit FQDN and bare shorthand under PROXY_REGISTRY_ONLY), explains the auth-vs-missing distinction and the no-redirect token-leak guard at a level release-notes readers can act on, and surfaces the // escape hatch. addresses CEO follow-up FU-5 Co-authored-by: chkp-roniz <35386615+chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(artifactory): pipeline integration trap for probe -> rebuild -> yaml entry Folds the CEO follow-up calling for a full-pipeline regression trap on PR #1472. The existing unit tests cover the resolver in isolation; this file traps the cross-module chain that ships into apm.yml / lockfile: resolve_parsed_dependency_reference -> _resolve_artifactory_boundary (HEAD probe, mocked) -> _rebuild_dep_ref (proxy-verified boundary) -> dependency_reference_to_yaml_entry (structured git+path entry) Five scenarios: - Mode 1 (explicit FQDN), nested-path: probe locks in owner+repo split, pipeline marks the dep for structured-entry persistence, the yaml entry embeds the proxy host + artifactory/<key> prefix + virtual path. - Mode 1, unambiguous two-segment path: single-candidate short-circuit is preserved (no HEAD-probe traffic, apm.yml stays in original shape). - Mode 2 (bare shorthand under PROXY_REGISTRY_ONLY), nested-path: probe locks in the boundary but rebuilt ref stays bare (no artifactory_prefix embedded; host stays github.com), so lockfile shape stays env-portable. - All-404 across candidates: raises UNRESOLVED with the // bypass hint. - All-transport-error across candidates: raises INCONCLUSIVE with the // bypass hint AND surfaces 'could not reach the proxy' so the user knows the failure mode is reachability, not a missing repo. Mutation-break gate (run locally; not in the test file): removing the new // hint from the INCONCLUSIVE message makes the 'test_transport_errors_raise_inconclusive_with_bypass_hint' assertion fail (Regex pattern did not match), confirming the test is a real trap and not a logic-replay assertion. addresses CEO follow-up FU-1 Co-authored-by: chkp-roniz <35386615+chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(artifactory): assert yaml entry URL via urllib.parse, not substring CodeQL flagged the substring assertions on entry['git'] (py/incomplete-url-substring-sanitization, high severity, alert #161 on PR #1472) and they also violated the repo's test convention: URL assertions in tests/** must use urllib.parse, never substring (.github/instructions/ tests.instructions.md). Switch to urlparse() + exact-match on hostname and ordered path segments. Same coverage (proxy host + artifactory/<key> prefix + probed owner/repo + virtual path), stricter assertion (a spoofed host containing 'art.example.com' as a substring would no longer pass), and CodeQL goes quiet. Co-authored-by: chkp-roniz <35386615+chkp-roniz@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: chkp-roniz <chkp-roniz@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Brings
docs/cli-reference.mdfully in sync with the current CLI surface, addressing all inconsistencies flagged by the automated CLI Consistency Checker in #140 and #152, plus reviewer feedback on the initial doc fix PR.Changes
--verboseflag fromapm uninstallapm prunesection### apm mcpparent group section (apm mcp COMMAND [OPTIONS]); demotedlist/search/showto####— consistent withapm deps,apm runtime,apm configgroup patternsapm compileflags:--single-agents,-v/--verbose,--local-only,--cleanapm compile --targetchoices to show[vscode|agents|claude|all]copilotruntime to supported runtimes listapm runtime setupargument description (was "remove", should be "install"); added--version TEXToption--yestoapm runtime removeapm runtime statussectioncopilot → codex → llm(wascodex → llm)apm search/apm show→apm mcp search/apm mcp showin Quick Start and Tips sectionsapm deps updateType of change
Testing
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.