fix(install): align validation auth chain with install#941
fix(install): align validation auth chain with install#941danielmeppiel merged 16 commits intomicrosoft:mainfrom
Conversation
|
@microsoft-github-policy-service agree |
a1icja
requested review from
danielmeppiel and
sergio-sisternes-epam
as code owners
There was a problem hiding this comment.
Pull request overview
Aligns apm install <owner>/<repo>/<subdir>#<ref> validation behavior with the actual install/clone authentication fallbacks, reducing false-negative validation failures when the GitHub Contents API rejects a token but git clone would succeed.
Changes:
- Extend
validate_virtual_package_existswith a Contents API directory-exists probe and a gatedgit ls-remoteauth chain fallback (mirroring_clone_with_fallback). - Thread verbose logging through validation so probe attempts are visible under
--verbose. - Add targeted tests for the
ls-remotefallback chain and document the updated validation/auth behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/deps/github_downloader.py |
Adds new validation probes and git ls-remote fallback helpers to reconcile validation with install auth behavior. |
src/apm_cli/install/validation.py |
Passes through verbose_callback so the new probes emit --verbose diagnostics. |
tests/test_github_downloader.py |
Introduces tests pinning the new ls-remote fallback chain behavior. |
docs/src/content/docs/getting-started/authentication.md |
Documents that CLI validation now follows the same auth chain as install. |
CHANGELOG.md |
Adds an Unreleased Fixed entry for the validation/auth alignment change. |
The new _ref_exists_via_ls_remote helper logged GitCommandError messages verbatim under --verbose. For basic-auth git URLs (the default), git surfaces the full URL including the embedded token in its error output, so a failing authenticated attempt would leak the token into the verbose log stream. Route the exception message through _sanitize_git_error before logging to keep the auth chain consistent with _clone_with_fallback (which has sanitized for the same reason since microsoft#856). Adds test_ls_remote_failure_log_scrubs_token_from_url pinning the guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve conflicts in CHANGELOG.md, src/apm_cli/deps/github_downloader.py, and tests/test_github_downloader.py: keep PR's auth-chain helpers (_directory_exists_at_ref, _ref_exists_via_ls_remote, _ssh_attempt_allowed) and marker-paths refactor; adopt main's reformatted download_virtual_file_package signature; move microsoft#941 entry into the existing 0.11.0 Fixed section; ruff-fix import ordering and Optional->| None. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (7 items)
Nits (11 items, skip if you want)
CEO arbitrationThe panel reached rare structural agreement: every required finding targets a genuine correctness or UX contract gap, and no panelist escalated a nit to required inappropriately. The Python Architect's two required findings are the load-bearing ones. The SHA-ref gap is a logical contradiction in the PR's own claim -- it says it mirrors The CLI Logging Expert's two required findings are independently necessary: seven Supply Chain Security and Auth Expert findings were all classified as nits and that classification is correct -- none rises to a blocking correctness or security gap at this scope. Dissent resolved: No cross-panelist classification disagreements were observed. All five required-finding authors placed their findings at the correct severity level. The SHA-ref gap was filed by the Python Architect alone; no other panelist contradicted it. Siding with the Architect: the PR title's explicit "mirrors Growth/positioning note: The OSS Growth Hacker's rewrite surfaces the real positioning value hiding in this fix: enterprise and EMU users with narrower env-var PATs are the target constituency APM is actively courting, and this bug directly blocked them. The corrected CHANGELOG line -- "no longer false-rejects packages whose git credentials differ from your API token" -- is the kind of user-story framing that converts enterprise evaluators reading the release notes. If this PR ships with that line, it doubles as positioning collateral for the enterprise adoption playbook. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class GitHubPackageDownloader {
<<IOBoundary>>
+auth_resolver AuthResolver
+git_env dict
+_protocol_pref ProtocolPreference
+_allow_fallback bool
+validate_virtual_package_exists(dep_ref, verbose_callback) bool
+_directory_exists_at_ref(dep_ref, path, ref, log) bool
+_ref_exists_via_ls_remote(dep_ref, ref, log) bool
+_ssh_attempt_allowed() bool
+_clone_with_fallback(dep_ref, ...) Path
+_resolve_dep_token(dep_ref) str
+_resolve_dep_auth_ctx(dep_ref) AuthContext
+_build_repo_url(...) str
+_build_noninteractive_git_env(...) dict
+_resilient_get(url, headers, timeout) Response
}
class AuthResolver {
<<Strategy>>
+resolve(host, org, port) AuthContext
+build_error_context(...) ErrorContext
}
class AuthContext {
<<ValueObject>>
+token str
+auth_scheme str
+source str
+git_env dict
}
class DependencyReference {
<<ValueObject>>
+repo_url str
+reference str
+virtual_path str
+host str
+port int
+is_insecure bool
+is_virtual_subdirectory() bool
+is_virtual_collection() bool
+is_virtual_file() bool
+is_artifactory() bool
+is_azure_devops() bool
}
class ProtocolPreference {
<<Enum>>
SSH
HTTPS
AUTO
}
class TransportSelector {
<<Strategy>>
+select(dep_ref, cli_pref, allow_fallback, has_token) TransportPlan
}
GitHubPackageDownloader *-- AuthResolver : resolves auth
GitHubPackageDownloader *-- TransportSelector : selects transport
GitHubPackageDownloader ..> AuthContext : reads
GitHubPackageDownloader ..> DependencyReference : reads
GitHubPackageDownloader ..> ProtocolPreference : reads
AuthResolver ..> AuthContext : returns
note for GitHubPackageDownloader "Chain of Responsibility (new in PR): marker-file probes -> Contents API -> git ls-remote"
note for AuthResolver "Strategy: token -> env-var -> credential-helper"
class GitHubPackageDownloader:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A([apm install owner/repo/sub#ref]) --> B[_validate_package_exists\nvalidation.py:189]
B --> C[GitHubPackageDownloader\n.validate_virtual_package_exists]
C --> D{dep_ref type?}
D -- is_virtual_collection --> E[_probe vpath.collection.yml\ngithub_downloader.py:1282]
D -- is_virtual_file --> F[_probe vpath\ngithub_downloader.py:1285]
D -- is_virtual_subdirectory --> G[marker file loop\ngithub_downloader.py:1289-1302]
G -- "apm.yml / SKILL.md /\nplugin.json variants / README.md" --> H["[NET] download_raw_file\nraw.githubusercontent.com"]
H -- 200 --> Z([return True])
H -- all 404 --> I["_directory_exists_at_ref\ngithub_downloader.py:1325"]
I -- ADO or non-GitHub host --> J[skip, return False]
I -- GitHub host --> K["[NET] _resilient_get\napi.github.com/repos/owner/repo/contents/path?ref=ref"]
K -- HTTP 200 --> Z
K -- HTTP 404 or error --> L{dep_ref.reference\n!= None?}
L -- No explicit ref --> M([return False])
L -- Explicit ref given --> N["_ref_exists_via_ls_remote\ngithub_downloader.py:1390"]
N --> O{dep_token\nresolved?}
O -- has token --> P["[NET][EXEC] git ls-remote --heads --tags URL ref\nAttempt 1: authenticated HTTPS\nenv=self.git_env GIT_ASKPASS=echo"]
P -- ref found --> Z
P -- GitCommandError/empty --> Q
O -- no token --> Q["[NET][EXEC] git ls-remote --heads --tags URL ref\nAttempt 2: plain HTTPS + credential helper\nenv=_build_noninteractive_git_env"]
Q -- ref found --> Z
Q -- fails --> R{_ssh_attempt_allowed?}
R -- No --> M
R -- Yes --> S["[NET][EXEC] git ls-remote --heads --tags URL ref\nAttempt 3: SSH\nGIT_SSH_COMMAND=ssh -o BatchMode=yes -o ConnectTimeout=10"]
S -- ref found --> Z
S -- fails --> M
E --> H
F --> H
Design patterns
Required: 2 (ls-remote SHA gap, bare except Exception). Nits: 3. CLI Logging ExpertRequired:
Nits:
DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertNo findings. Nits:
Auth ExpertNo findings. Nits:
OSS Growth HackerRequired:
Nits:
Verdict computed deterministically: 7 required findings across 6 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
added
panel-rejected
|
Hi @a1icja -- thanks for pushing this through. The fresh state of the branch (head Blocker 1: file-size cap
Blocker 2: APM Review Panel REJECT (7 required findings)From the panel comment dated 2026-04-30T17:01:08Z. All seven are independently required.
(Eleven nits in the panel comment are skip-if-you-want; only the seven above are required.) How to land both blockers in one revisionThe seven fixes net out to roughly +7 lines (the SHA-pin path adds ~5, the
I deliberately did not push any of this myself -- this is your PR and the structural call (trim vs split) is yours to make. Happy to open a stacked targeted PR against your branch implementing whichever option you prefer if that helps; just say the word. |
…ngs + line-cap compliance
- Extract _directory_exists_at_ref, _ref_exists_via_ls_remote, _ssh_attempt_allowed,
validate_virtual_package_exists into src/apm_cli/deps/github_downloader_validation.py
(sibling module). github_downloader.py drops 2493 -> 2279 lines, well under
the 2400 cap. Class keeps thin instance-method shims so existing test mocks
(patch GitHubPackageDownloader.validate_virtual_package_exists / _ref_exists_via_ls_remote)
continue to work unchanged.
- SHA-pin detection: re.fullmatch(r'[0-9a-fA-F]{7,40}', ref) gates ls-remote
to omit --heads --tags for hex refs and scan the full advertised-refs
list for a SHA-prefix match. Fixes silent false-reject of commit-SHA pins.
- Narrow except Exception -> except ImportError in _ssh_attempt_allowed so
AttributeError/NameError from import machinery surface instead of being
swallowed.
- Traffic-light: marker-file misses on the success path log [i] (expected),
HTTP 404 from the directory-exists probe logs [i] (expected), and [x] is
reserved for unexpected HTTP statuses, request exceptions, and genuine
ls-remote failures.
- Header line gains the [i] STATUS_SYMBOLS prefix:
' [i] Validating virtual package at ref "...": ...'
- ls-remote fallback now emits a non-verbose [!] warning via warn_callback
when path validation is deferred to install-time, so users in
default-verbosity mode notice the deferral.
- CHANGELOG entry rewritten as a user story (auth-chain alignment, EMU/SSO
PAT scenarios) instead of internal helper names.
- docs/src/content/docs/reference/cli-commands.md verbose-mode bullet now
documents the per-probe output for subdirectory packages with explicit refs.
- See microsoft#941 panel verdict 2026-04-30T17:01:08Z
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (8 items)
Nits (16 items, skip if you want)
CEO arbitrationPR #941 is rejected on eight required findings distributed across five active panelists. The panel is not split on verdict; every active voice with required findings agrees the code is not shippable as written. The two heaviest findings are independently disqualifying. Supply-chain flags that the The remaining six required findings -- missing type hints on the new public validation API, the unguarded The author should address the two blocking security findings first -- replace the Dissent resolved: No verdict dissent. The only overlap -- python-architect marking Growth/positioning note: The oss-growth-hacker is correct that EMU and SSO credential friction is the highest-leverage silent blocker for enterprise adoption in the 0.11 cycle. Once the PR is clean, surface this as a named win for enterprise platform engineers: "APM now works out of the box in EMU orgs -- no credential gymnastics." Hold the release note language; do not publish it against a build that still has a fail-open path-existence gate. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class GitHubPackageDownloader {
<<Facade>>
+validate_virtual_package_exists(dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(dep_ref, path, ref, log) bool
+_ref_exists_via_ls_remote(dep_ref, ref, log) bool
+_ssh_attempt_allowed() bool
+_resilient_get(url, headers, timeout) Response
+_build_repo_url(repo_url, use_ssh, dep_ref, token, auth_scheme) str
+_resolve_dep_token(dep_ref) str
+_resolve_dep_auth_ctx(dep_ref) AuthContext
+_sanitize_git_error(msg) str
}
class github_downloader_validation {
<<Module / IOBoundary>>
+validate_virtual_package_exists(downloader, dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(downloader, dep_ref, path, ref, log) bool
+_ref_exists_via_ls_remote(downloader, dep_ref, ref, log) bool
+_ssh_attempt_allowed(downloader) bool
+_is_sha_pin(ref) bool
}
class AuthResolver {
<<Strategy>>
+resolve(host, owner, port) AuthContext
}
class AuthContext {
<<ValueObject>>
+token str
+auth_scheme str
+git_env dict
}
class DependencyReference {
<<ValueObject>>
+repo_url str
+reference str
+virtual_path str
+host str
+port int
+is_virtual bool
+is_virtual_collection() bool
+is_virtual_file() bool
+is_virtual_subdirectory() bool
+is_azure_devops() bool
+is_artifactory() bool
+is_insecure bool
}
class InstallValidation {
<<Orchestrator>>
+validate_dependencies(deps) ValidationResult
}
class GitHubPackageDownloader:::touched
class github_downloader_validation:::touched
class InstallValidation:::touched
GitHubPackageDownloader ..> github_downloader_validation : delegates (lazy import)
GitHubPackageDownloader *-- AuthResolver : auth_resolver
AuthResolver ..> AuthContext : returns
github_downloader_validation ..> DependencyReference : reads
github_downloader_validation ..> AuthContext : reads via downloader
InstallValidation ..> GitHubPackageDownloader : calls validate_virtual_package_exists
note for github_downloader_validation "Chain of Responsibility: marker-file probe -> Contents API -> git ls-remote"
note for AuthResolver "Strategy: token -> credential-helper -> SSH"
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install owner/repo/sub#ref\nInstallValidation.validate_dependencies"])
B["validate_virtual_package_exists\ngithub_downloader.py (thin shim)"]
C["validate_virtual_package_exists\ngithub_downloader_validation.py"]
D{dep_ref.is_virtual?}
E(["raise ValueError"])
F{"dep_ref type?"}
G["[NET] _probe collection marker\nvpath.collection.yml @ ref"]
H["[NET] _probe file\nvpath @ ref"]
I["[NET] _probe marker files\napm.yml / SKILL.md / plugin.json..."]
J{"any marker found?"}
K["[NET] _directory_exists_at_ref\nContents API GET /repos/owner/repo/contents/path?ref=ref"]
L{"HTTP 200?"}
M{"dep_ref.reference set?"}
N["[EXEC] _ref_exists_via_ls_remote\ngit ls-remote (3-attempt chain)"]
O{"token available?"}
P["[EXEC] git ls-remote authenticated HTTPS\ntoken URL + token_env"]
Q["[EXEC] git ls-remote plain HTTPS\ncredential-helper env"]
R{"SSH allowed?\n_ssh_attempt_allowed"}
S["[EXEC] git ls-remote SSH\nssh -o BatchMode=yes"]
T{"any attempt matched ref?"}
U["warn_callback: path validation deferred\n[!] use --verbose"]
V(["return True\n(defer path check to install)"])
W(["return False"])
X(["return True"])
A --> B
B --> C
C --> D
D -- No --> E
D -- Yes --> F
F -- collection --> G --> X
F -- file --> H --> X
F -- subdirectory --> I --> J
J -- Yes --> X
J -- No --> K --> L
L -- Yes --> X
L -- No --> M
M -- No ref --> W
M -- Has ref --> N
N --> O
O -- Yes --> P --> T
O -- No --> Q --> T
T -- No from HTTPS --> R
R -- Yes --> S --> T
T -- matched --> U --> V
T -- no match --> W
Design patterns
Required: 2 (see aggregated section above) CLI Logging ExpertRequired: 1 (double DevX UX ExpertRequired: 2 (warning message gives no next action; apm-guide skill resources not updated -- see aggregated section) Supply Chain Security ExpertRequired: 2 (fail-open path existence; unguarded virtual_path in Contents API URL -- see aggregated section) Auth ExpertRequired: 1 (bearer token embedded in URL instead of Authorization header for ADO -- see aggregated section) OSS Growth HackerNo findings. Verdict computed deterministically: 8 required findings across 5 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
Round 2 of panel-review folding for microsoft#941. Addresses 8 findings on SHA 7150c2a: 1. Full type annotations on github_downloader_validation public API 2. Length-guard on repo_url.split('/', 1) prevents ValueError 3. Strip [!] from warn message strings (logger prepends symbol) 4. Add recovery action to access-denied warning 5. Update packages/apm-guide/.apm/skills/apm-usage/commands.md 6. Close fail-open: shallow-fetch + ls-tree probes vpath after ls-remote 7. Apply validate_path_segments(vpath) before URL interpolation 8. ADO bearer tokens via http.extraHeader, not URL embedding New regression tests cover findings 6, 7, 8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (12 items)
Nits (16 items, skip if you want)
CEO arbitrationThis PR lands a genuine auth-chain improvement -- credential-fallback sequencing for EMU/SSO users is the right problem to solve, and the architectural direction (Chain of Responsibility in The warn_callback findings from cli-logging, devx-ux, and oss-growth-hacker are three lenses on the same UX failure: a warning fires on the happy path, uses implementation jargon the user cannot act on, references a non-standard "Contents-API scope" string that maps to no known OAuth scope, and uses '@' as a ref separator where the CLI contract is '#'. All three are required. The fact that three specialists reached this conclusion independently signals the warning was written for the engineer who wrote the code, not the user who reads it at 11pm wondering why their CI pipeline printed something alarming. The fix is a single, human-readable line with the '#' separator and a concrete scope string ('repo' or 'read:packages'), suppressed entirely on the default path and surfaced only under --verbose. The CHANGELOG entry requires a rewrite. 'EMU/SSO users with a narrower env-var PAT' and 'lockfile-driven install would have handled silently' are maintainer notes, not user-facing copy. The rewrite angle is: APM now respects the same credential chain the user's git client already uses. No new configuration. That is the correct lede and it writes itself once the jargon is stripped. Growth/positioning note: The underlying fix is a strong "it just works" story -- enterprise credential complexity (EMU, SSO, PAT scoping, SSH fallback) silently handled by APM itself rather than pushed onto the user. A clean CHANGELOG entry, once the required rewrite lands, is ready to lift into a release post with the frame: "apm now respects the same credential chain your git client uses -- nothing new to configure." That is a defensible, concrete differentiator. The headline writes itself. Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class GitHubPackageDownloader {
<<Downloader>>
+validate_virtual_package_exists(dep_ref, verbose_callback, warn_callback) bool
-_directory_exists_at_ref() bool
-_ref_exists_via_ls_remote() bool
-_ssh_attempt_allowed() bool
}
class GitHubPackageDownloaderValidation {
<<Module>>
+validate_virtual_package_exists(downloader, dep_ref, verbose_callback, warn_callback) bool
+_build_validation_attempts(downloader, dep_ref, log) list
+_ref_exists_via_ls_remote(downloader, dep_ref, ref, log) bool
+_path_exists_in_tree_at_ref(downloader, dep_ref, vpath, ref, log) bool
+_directory_exists_at_ref(downloader, dep_ref, path, ref, log) bool
+_ssh_attempt_allowed(downloader) bool
+_is_sha_pin(ref) bool
+_split_owner_repo(repo_url) tuple
}
note for GitHubPackageDownloaderValidation "Chain of Responsibility:\nPAT HTTPS -> plain HTTPS (cred helper) -> SSH\nExtracted to respect 2400-line module cap"
class DependencyReference {
<<ValueObject>>
+repo_url str
+reference str
+virtual_path str
+is_virtual bool
}
class AuthResolver {
<<Strategy>>
+resolve(host, owner, port) AuthContext
}
class InstallValidation {
<<Module>>
+validate_package_exists(dep_ref, verbose_log, logger) bool
}
GitHubPackageDownloader ..> GitHubPackageDownloaderValidation : delegates (lazy import)
GitHubPackageDownloaderValidation ..> DependencyReference : reads
GitHubPackageDownloaderValidation ..> AuthResolver : reads via downloader
InstallValidation ..> GitHubPackageDownloader : calls validate
class GitHubPackageDownloaderValidation:::touched
class GitHubPackageDownloader:::touched
class InstallValidation:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install owner/repo/sub#ref"])
B["install/validation.py\nvalidate_virtual_package_exists(dep_ref,\n verbose_callback, warn_callback)"]
C{"dep_ref.is_virtual_subdirectory?"}
D["[FS] validate_path_segments(vpath)\npath_security.py"]
E["REJECT: PathTraversalError\nreturn False"]
F["[NET] marker-file probes\ndownload_raw_file x7\n(apm.yml, SKILL.md, ...)"]
G{"Any probe hit?"}
H["return True"]
I["[NET] _directory_exists_at_ref\nGET /repos/owner/repo/contents/path?ref=ref"]
J{"HTTP 200?"}
K{"dep_ref.reference is not None?"}
L["[EXEC] _ref_exists_via_ls_remote\ngit ls-remote (up to 3 attempts)\nPAT -> plain HTTPS -> SSH"]
M{"Ref found?"}
N["[EXEC][FS] _path_exists_in_tree_at_ref\ngit init --bare + fetch --depth=1 --filter=tree:0\n+ git ls-tree FETCH_HEAD vpath\nBUG: only tries attempts[0] (PAT)"]
O{"vpath in tree?"}
P["warn_callback: 'validated via git fallback'\nreturn True"]
Q["return False"]
R["return False (no explicit ref)"]
A --> B --> D
D -->|"'..' detected"| E
D -->|"clean"| C
C -->|"no"| F2["[NET] file/collection probes\nreturn True/False"]
C -->|"yes"| F
F --> G
G -->|"yes"| H
G -->|"no"| I
I --> J
J -->|"200"| H
J -->|"404 / error"| K
K -->|"no"| R
K -->|"yes"| L
L --> M
M -->|"no"| Q
M -->|"yes"| N
N --> O
O -->|"yes"| P
O -->|"no"| Q
style N fill:#ffd6d6,stroke:#cc0000
style E fill:#ffd6d6,stroke:#cc0000
Design patterns
Required: see item 1 above. Nits: backward-compat private-method shims unnecessary; CLI Logging ExpertRequired: Nits: DevX UX ExpertRequired: warn_callback fires on the happy path with opaque jargon (violates 'quiet on the happy path'; breaks CI pipelines); warn_callback uses '@' as ref separator instead of the CLI-canonical '#'. Nits: Supply Chain Security ExpertRequired: Non-ADO token embedded in git URL (credential-bearing URL visible in OS process table and written into temp bare repo Nits: misleading comment about single-dot segment tolerance in Auth ExpertRequired: ADO auth scheme hardcoded to 'Bearer' -- ADO PATs require Nits: OSS Growth HackerRequired: CHANGELOG entry uses internal jargon ('env-var PAT', 'lockfile-driven install') -- fails the repost test; warn message uses 'Contents-API scope' which is not a named OAuth scope users can find or add. Nits: double-negative in authentication.md ('never rejects...would accept'); 'virtual subdirectory packages' undefined at first use in commands.md; Verdict computed deterministically: 12 required findings across 6 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
Round 3 panel surfaced 12 required findings across architect, auth,
supply-chain-security, cli-logging, devx-ux, and oss-growth personas.
This commit folds all 12 with regression tests appended.
Architecture / auth (the attempts[0] bug):
- _ref_exists_via_ls_remote now returns (bool, AttemptSpec | None) so the
*winning* auth attempt survives the call. _path_exists_in_tree_at_ref
takes that winning_attempt and uses the same env+url for the tree probe.
No more fanning the path probe at attempts[0].
- New AttemptSpec NamedTuple (label, url, env) is the carrier between
ref check and path probe.
ADO auth correctness:
- ADO PATs now use HTTP Basic with base64(':<PAT>') (the leading colon
is the ADO convention). Bearer fallback for AAD tokens preserved.
- Non-ADO HTTPS attempts inject the token via http.extraheader, never
via the URL. Closes 'token in process argv' leak.
Security:
- Tree-probe cleanup uses safe_rmtree(tmpdir, base_temp), never the
raw robust_rmtree -- matches the project-wide path-safety rule.
CLI logging:
- All ls-remote 'no match' / 'tree miss' messages use [!] (was [i]).
- _warn in install/validation.py is verbose-gated and no longer falls
back to _rich_echo (logger is always present in install path).
DevX:
- Warn message now reads
'Validated <ref> via git fallback (API check skipped). Run with --verbose for details.'
using DependencyReference.to_canonical() (# separator).
CHANGELOG:
- microsoft#941 entry rewritten in user voice.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (7 items)
Nits (18 items, skip if you want)
CEO arbitrationTwo supply-chain-security findings are the sharpest blockers and must be addressed before merge. The empty-string ref bypass and the empty-vpath reaching git ls-tree are both correctness bugs introduced by this PR that directly contradict documented security invariants. Both are in the new validation module and must be fixed here. The fixes are small (one-character change for the ref gate; a null guard for the empty vpath), but the correctness consequences are significant -- an attacker who controls a dep string could add a trailing cli-logging-expert and devx-ux-expert converge from different angles on the same silent-fallback problem. cli-logging-expert frames it as a traffic-light violation: actionable fallback output is suppressed below --verbose. devx-ux-expert frames it as a UX anti-pattern: flags that change behavior without telling the user. The two framings are consistent, not contradictory, and together make the case that a non-verbose stderr warning is required when the git fallback resolves a package the token probe rejected. devx-ux-expert additionally raises that the all-probes-fail error shape is unspecified and that commands.md has absorbed implementation detail that belongs in a troubleshooting reference. python-architect found the module decomposition sound and raised no required findings; the extraction of validation logic into Dissent resolved: There is no dissent between panelists. python-architect's nit on the warn_callback docstring is consistent with cli-logging-expert's required finding on suppression behavior -- both point at the same mismatch, python-architect at the documentation gap and cli-logging-expert at the runtime consequence. oss-growth-hacker's note does not conflict with any security or UX finding; it is purely additive framing for post-merge positioning. Growth/positioning note: Once the security and UX blockers are resolved, the release note should lead with the user-facing outcome: "apm install now works with your existing credential chain, including SSO, EMU, and GHES tokens." Enterprise and EMU credential friction is a documented evaluation-stage drop-off point for internal tooling. A short Discussions post asking whether users hit silent install rejections before this fix would surface churned evaluators and provide organic signal for the roadmap. The module extraction to Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class GitHubPackageDownloader {
<<Facade>>
+validate_virtual_package_exists(dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(dep_ref, path, ref, log) bool
+_ref_exists_via_ls_remote(dep_ref, ref, log) bool
+_ssh_attempt_allowed() bool
+download_raw_file(dep_ref, path, ref)
+_resilient_get(url, headers, timeout) Response
-auth_resolver AuthResolver
-git_env dict
}
class github_downloader_validation {
<<Module>>
+validate_virtual_package_exists(downloader, dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(downloader, dep_ref, path, ref, log) bool
+_ref_exists_via_ls_remote(downloader, dep_ref, ref, log) tuple
+_path_exists_in_tree_at_ref(downloader, dep_ref, vpath, ref, log, winning) bool
+_build_validation_attempts(downloader, dep_ref, log) list
+_ssh_attempt_allowed(downloader) bool
}
class AttemptSpec {
<<ValueObject>>
+label str
+url str
+env dict
}
class AuthResolver {
<<Strategy>>
+resolve(host, owner, port) AuthContext
}
class DependencyReference {
<<ValueObject>>
+repo_url str
+virtual_path str
+reference str
+is_virtual_subdirectory() bool
+is_azure_devops() bool
}
class GitHubPackageDownloader:::touched
class github_downloader_validation:::touched
class AttemptSpec:::touched
GitHubPackageDownloader ..> github_downloader_validation : delegates (lazy import)
github_downloader_validation *-- AuthResolver : resolves via downloader
github_downloader_validation ..> AttemptSpec : produces chain
github_downloader_validation ..> DependencyReference : reads
GitHubPackageDownloader *-- AuthResolver : auth_resolver
note for github_downloader_validation "Chain of Responsibility: token HTTPS -> plain HTTPS -> SSH via AttemptSpec list"
note for GitHubPackageDownloader "Shim methods: backward-compat delegation so existing mocks keep working"
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A[apm install CLI] --> B[_validate_package_exists\ninstall/validation.py]
B --> C[GitHubPackageDownloader\n.validate_virtual_package_exists shim]
C --> D[gdv.validate_virtual_package_exists\ngithub_downloader_validation.py]
D --> SEC{validate_path_segments\nvpath traversal check}
SEC -- PathTraversalError --> REJECT[return False]
SEC -- ok --> TYPE{dep_ref type?}
TYPE -- subdirectory --> MF[[NET] marker-file probes\napm.yml SKILL.md plugin.json README.md]
MF -- any hit --> RT[return True]
MF -- all miss --> DIR[[NET] _directory_exists_at_ref\nContents API GET /repos/.../contents/vpath]
DIR -- 200 --> RT
DIR -- 404/err --> REF{dep_ref.reference\nnot None?}
REF -- no ref --> RF[return False]
REF -- has ref --> LS[[EXEC] _ref_exists_via_ls_remote\ngit ls-remote via AttemptSpec chain\ntoken-HTTPS -> plain-HTTPS -> SSH]
LS -- all fail --> RF
LS -- ok, winning_attempt --> TREE[[EXEC][FS] _path_exists_in_tree_at_ref\ngit init bare + fetch depth=1\nfilter=tree:0 + ls-tree FETCH_HEAD vpath]
TREE -- vpath present --> WARN[warn_callback fired if caller wires it] --> RT
TREE -- vpath absent --> RF
Design patterns
CLI Logging ExpertRequired findings (see above). Nits: DevX UX ExpertRequired findings (see above). Nits: CHANGELOG "same credential chain" is slightly imprecise (shallow-fetch has no analog in a normal install). authentication.md new paragraph uses double-negative. --verbose description in cli-commands.md omits the shallow-fetch/ls-tree step. Supply Chain Security ExpertRequired findings (see above). Nits: StrictHostKeyChecking safety is comment-only with no test enforcement. Git fallback availability is credential-dependent, weakening lockfile determinism across environments; document in security.md. Auth ExpertNo findings. Nits: GHES Contents-API URL OSS Growth HackerNo findings. Nits: CHANGELOG headline buries the enterprise/SSO/EMU audience. authentication.md new paragraph has no runnable example. No release beat planned. No re-engagement path for users who hit this pre-fix. Module extraction is a compounding win worth naming in the CHANGELOG. Verdict computed deterministically: 7 required findings across 6 active panelists. APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
…osed Round 4 of panel fold for microsoft#941. 7 required findings on 4fdc431: - supply-chain (2): empty-ref truthy gate; reject_empty=True on vpath - cli-logging (2): warn_callback surfaced in non-verbose; _rich_warning fallback when logger is None; suffix-stripping symmetry - devx-ux (3): named four-probe chain in terminal error; commands.md collapsed to one-sentence link; validation chain detail in authentication.md Validated locally via 6-persona panel + apm-ceo synthesizer (APPROVE). Lint clean. Full unit suite: 6933 passed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (3 items)
Nits (13 items, skip if you want)
CEO arbitrationThe panel converges on a sound architecture: the four-step validation probe chain correctly mirrors the actual install auth chain, the Two blockers must be resolved before merge. First, devx-ux-expert flags both the failure message and the yellow-path warning as maintainer-oriented prose masquerading as user guidance. These are not style nits -- user-facing message quality is a correctness requirement under APM's enterprise-evaluator audience. Second, auth-expert identifies a violated architecture invariant at line 253: All remaining findings from python-architect, cli-logging-expert, supply-chain-security-expert, and oss-growth-hacker are nits: tightening Dissent resolved: No inter-panelist dissent on required vs nit classification. All three required findings were raised by a single specialist and not contradicted by any other panelist. No arbitration tiebreak was needed. Growth/positioning note: oss-growth-hacker correctly identifies this as the highest-friction enterprise first-run failure -- a user's first Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class GitHubPackageDownloader {
<<IOBoundary>>
+validate_virtual_package_exists(dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(...) bool
+_ref_exists_via_ls_remote(...) bool
+_ssh_attempt_allowed() bool
+download_raw_file(...)
+_build_noninteractive_git_env(...) dict
}
note for GitHubPackageDownloader "Shim layer: all validation methods
are now thin delegators to
github_downloader_validation module"
class github_downloader_validation {
<<Pure>>
+validate_virtual_package_exists(downloader, dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(downloader, dep_ref, vpath, ref, log) bool
+_build_validation_attempts(downloader, dep_ref, log) list
+_ref_exists_via_ls_remote(downloader, dep_ref, ref, log) tuple
+_path_exists_in_tree_at_ref(downloader, dep_ref, vpath, ref, log, winning_attempt) bool
+_ssh_attempt_allowed(downloader) bool
}
note for github_downloader_validation "Chain of Responsibility:
marker-file -> Contents API ->
git ls-remote -> shallow-fetch+ls-tree"
class AttemptSpec {
<<ValueObject>>
+label str
+url str
+env dict
}
class DependencyReference {
<<ValueObject>>
+is_virtual bool
+reference str
+virtual_path str
+repo_url str
+is_virtual_file() bool
+is_virtual_collection() bool
+is_virtual_subdirectory() bool
+is_azure_devops() bool
}
class InstallValidation {
<<IOBoundary>>
+_validate_package_exists(dep_ref, ...) bool
}
class AuthResolver {
<<Strategy>>
+resolve_for_dep(dep_ref) AuthContext
}
InstallValidation ..> GitHubPackageDownloader : constructs + calls
GitHubPackageDownloader ..> github_downloader_validation : lazy import delegates
github_downloader_validation *-- AttemptSpec : produces chain
github_downloader_validation ..> DependencyReference : reads
github_downloader_validation ..> GitHubPackageDownloader : first-arg collaborator
GitHubPackageDownloader o-- AuthResolver : injected
class GitHubPackageDownloader:::touched
class github_downloader_validation:::touched
class InstallValidation:::touched
classDef touched fill:#fff3b0,stroke:#d47600
flowchart TD
A(["apm install\n(CLI entry)"])
B["install/validation.py:\n_validate_package_exists"]
C["[FS] Construct GitHubPackageDownloader\nwith auth_resolver"]
D["GitHubPackageDownloader.validate_virtual_package_exists\n(thin shim -- lazy import)"]
E["github_downloader_validation.validate_virtual_package_exists"]
F{"validate_path_segments\n(vpath, reject_empty=True)"}
G(["return False\n(PathTraversalError)"])
H{"dep_ref type?"}
I["[NET] _probe: download_raw_file\nmarker-file (collection.yml / file / apm.yml...)"]
J{"marker hit?"}
K(["return True"])
L["[NET] _directory_exists_at_ref\nContents API GET /repos/{owner}/{repo}/contents/{vpath}"]
M{"API 200?"}
N{"dep_ref.reference\ntruthy?"}
O(["return False"])
P["[EXEC] _build_validation_attempts\nbuild AttemptSpec chain\n(token-header / plain-HTTPS / SSH)"]
Q["[NET][EXEC] _ref_exists_via_ls_remote\ngit ls-remote per AttemptSpec"]
R{"ref found?\nwinning_attempt != None?"}
S["[FS][EXEC] _path_exists_in_tree_at_ref\ngit fetch --depth=1 + ls-tree FETCH_HEAD vpath\n(uses WINNING AttemptSpec -- not attempts[0])"]
T{"ls-tree hit?"}
U["warn_callback fired\n(yellow: git-cred chain wider than API PAT)"]
A --> B
B --> C
C --> D
D --> E
E --> F
F -- "traversal / empty" --> G
F -- "ok" --> H
H -- "collection" --> I
H -- "file" --> I
H -- "subdirectory" --> I
I --> J
J -- "yes" --> K
J -- "no (all markers missed)" --> L
L --> M
M -- "yes" --> K
M -- "no" --> N
N -- "no (default branch)" --> O
N -- "yes" --> P
P --> Q
Q --> R
R -- "no" --> O
R -- "yes" --> S
S --> T
T -- "no" --> O
T -- "yes" --> U
U --> K
Design patterns
No findings. CLI Logging ExpertNo findings. DevX UX ExpertRequired:
Nits:
Supply Chain Security ExpertNo findings. Auth ExpertRequired:
Nits:
OSS Growth HackerNo findings. Verdict computed deterministically: 3 required findings across 2 active panelists (devx-ux-expert: 2, auth-expert: 1). APPROVE iff N == 0. Push a new commit to clear this verdict label automatically. Note 🔒 Integrity filter blocked 2 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
github-actions
Bot
removed
the
panel-review
Replace direct resolve(host, owner, port=...) call with the per-dep API resolve_for_dep(dep_ref). Functionally equivalent today, but keeps this call site aligned with the AuthResolver invariant so future per-dep enrichment (ADO org disambiguation, port-aware org overrides) flows through automatically. Round 5 panel finding (auth-expert). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
added
panel-review
danielmeppiel
removed
the
panel-review
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel Verdict: REJECT
Required before merge (5 items)
Nits (20 items, skip if you want)
CEO arbitrationThe panel is unanimous that PR #941 solves a real and painful enterprise problem -- silent auth rejection when an env-var PAT fails but the system credential helper would have succeeded. The two auth-expert required findings, however, reveal that the new validation chain introduces its own correctness gap: it does not mirror the ADO AAD bearer fallback that The two devx-ux-expert required findings are not cosmetic -- surfacing internal probe names in default error output and emitting a yellow warning on a success path will erode trust in exactly the enterprise audience this fix is trying to win. These findings are consistent with the cli-logging-expert's nits and reinforce each other; the panel is effectively unanimous on the user-facing messaging concerns even where individual severity ratings differ. The oss-growth-hacker's required finding is editorially correct: the CHANGELOG is a conversion surface for evaluators who just hit this bug, and naming the internal module there is release notes written for maintainers. The architectural extraction from Dissent resolved: The cli-logging-expert filed the yellow warning on success path as a nit; devx-ux-expert filed it as required. Siding with devx-ux-expert: a warning-level signal on a success path is a UX correctness issue, not a style preference, because it trains users to ignore warnings and causes panic in a CI log where a yellow signal implies degraded state. The supply-chain-security-expert's TOCTOU + SHA-pin note is correctly filed as a nit -- it is a hardening opportunity, not a regression introduced by this PR. Growth/positioning note: This fix is a quiet enterprise conversion unlock. The class of user it unblocks -- teams running SSO, EMU, or GHES with system credential helpers -- is exactly the evaluator who hits Per-persona findings (full)Python ArchitectclassDiagram
direction LR
class GitHubPackageDownloader {
<<Facade>>
+validate_virtual_package_exists(dep_ref, verbose_callback, warn_callback) bool
+_directory_exists_at_ref(dep_ref, path, ref, log) bool
+_ref_exists_via_ls_remote(dep_ref, ref, log) bool
+_ssh_attempt_allowed() bool
+_clone_with_fallback() Repo
-auth_resolver AuthResolver
}
class validate_virtual_package_exists {
<<IOBoundary>>
+__call__(downloader, dep_ref, verbose_callback, warn_callback) bool
}
class _build_validation_attempts {
<<Pure>>
+__call__(downloader, dep_ref, log) list~AttemptSpec~
}
class _ref_exists_via_ls_remote {
<<IOBoundary>>
+__call__(downloader, dep_ref, ref, log) tuple~bool, AttemptSpec~
}
class _path_exists_in_tree_at_ref {
<<IOBoundary>>
+__call__(downloader, dep_ref, vpath, ref, log, winning_attempt) bool
}
class _directory_exists_at_ref {
<<IOBoundary>>
+__call__(downloader, dep_ref, path, ref, log) bool
}
class AttemptSpec {
<<ValueObject>>
+label str
+url str
+env dict
}
class DependencyReference {
<<ValueObject>>
+repo_url str
+reference str
+virtual_path str
+host str
+is_virtual bool
+is_virtual_subdirectory() bool
+is_azure_devops() bool
}
class AuthResolver {
<<Strategy>>
+resolve_for_dep(dep_ref) AuthContext
}
class GitHubPackageDownloader:::touched
class validate_virtual_package_exists:::touched
class _build_validation_attempts:::touched
class _ref_exists_via_ls_remote:::touched
class _path_exists_in_tree_at_ref:::touched
class _directory_exists_at_ref:::touched
class AttemptSpec:::touched
classDef touched fill:#fff3b0,stroke:#d47600
GitHubPackageDownloader ..> validate_virtual_package_exists : delegates (lazy import shim)
GitHubPackageDownloader o-- AuthResolver : uses
validate_virtual_package_exists ..> _directory_exists_at_ref : Fallback 1
validate_virtual_package_exists ..> _ref_exists_via_ls_remote : Fallback 2
_ref_exists_via_ls_remote ..> _build_validation_attempts : builds chain
_ref_exists_via_ls_remote ..> AttemptSpec : returns winning
_path_exists_in_tree_at_ref ..> AttemptSpec : reuses winning
validate_virtual_package_exists ..> _path_exists_in_tree_at_ref : path confirm
validate_virtual_package_exists ..> DependencyReference : reads
_build_validation_attempts ..> AttemptSpec : produces
_build_validation_attempts ..> AuthResolver : resolves token
note for _build_validation_attempts "Chain of Responsibility:\ntoken header -> plain HTTPS + cred helper -> SSH\nMirrors _clone_with_fallback auth ordering"
flowchart TD
A(["apm install owner/repo/sub#BRANCH"]) --> B["install/validation.py\n_validate_package_exists"]
B --> C["GitHubPackageDownloader\n.validate_virtual_package_exists (shim)"]
C --> D["github_downloader_validation\n::validate_virtual_package_exists"]
D --> E["validate_path_segments(vpath)\n[security gate: blocks .. traversal + empty]"]
E -->|PathTraversalError| FAIL(["return False"])
E --> F{"dep_ref type?"}
F -->|collection| G["[NET] download_raw_file\nmarker: vpath.collection.yml@ref"]
F -->|file| H["[NET] download_raw_file\nvpath@ref"]
F -->|subdirectory| I["[NET] download_raw_file\n7 marker paths (apm.yml, SKILL.md, ...)"]
I -->|any 200| OK(["return True"])
I -->|all miss| J["[NET] _directory_exists_at_ref\nContents API GET /repos/owner/repo/contents/vpath?ref=ref"]
J -->|HTTP 200| OK2(["return True"])
J -->|404 or error| K{"dep_ref.reference truthy?"}
K -->|no| FAIL2(["return False"])
K -->|yes| L["[EXEC] _ref_exists_via_ls_remote\n_build_validation_attempts -> AttemptSpec list"]
L --> M["[EXEC] git ls-remote --heads --tags url ref\nper attempt (token-header HTTPS, plain HTTPS, SSH)"]
M -->|all fail| FAIL3(["return False, None"])
M -->|first succeeds| N["winning AttemptSpec returned"]
N --> O["[FS] tempfile.mkdtemp(prefix=apm-validate-)\ngit init --bare probe.git"]
O --> P["[EXEC] git fetch --depth=1 --filter=tree:0\norigin ref (winning_attempt url+env)"]
P -->|GitCommandError| FAIL4(["return False"])
P --> Q["[EXEC] git ls-tree FETCH_HEAD vpath"]
Q -->|empty output| FAIL5(["return False"])
Q -->|has output| R["[FS] safe_rmtree(tmpdir)"]
R --> S["warn_callback: 'API validation skipped...\ngit credential fallback'"]
S --> OK3(["return True"])
G -->|200| OK
H -->|200| OK
Design patterns
Required: None. CLI Logging ExpertNo required findings. DevX UX ExpertRequired: 2 (see aggregated required above). Supply Chain Security ExpertNo required findings. Auth ExpertRequired: 2 (see aggregated required above). OSS Growth HackerRequired: 1 (see aggregated required above). Verdict computed deterministically: 5 required findings across 3 active panelists (devx-ux-expert: 2, auth-expert: 2, oss-growth-hacker: 1). APPROVE iff N == 0. Push a new commit to clear this verdict label automatically.
|
github-actions
Bot
added
the
panel-rejected
fix(install): align validation auth chain with install
TL;DR
apm install owner/repo/sub#BRANCHwould false-fail validation when the GitHub Contents API rejected the env-var PAT butgit clone(driven by the system credential helper) would have succeeded — the same package lands fine when added toapm.ymldirectly because that path skips validation entirely. This PR teachesvalidate_virtual_package_existstwo new fallbacks: a directory-exists Contents API probe, and agit ls-remotechain that mirrors_clone_with_fallback's auth attempts. Validation now agrees with install for any virtual subdirectory dep that carries an explicit#ref; path-typo rejection on the default branch is preserved.Note
The
ls-remotefallback is gated ondep_ref.reference is not Noneso unsuffixedowner/repo/sub(no explicit#ref) still fails fast on path typos — the gate keeps that signal.Problem (WHY)
apm install foo/bar/gee#sho --verboseerrors withnot accessible or doesn't existwhile the equivalent entry inapm.ymlinstalls cleanly. Verbose log shows the resolver found a per-org PAT (source=GITHUB_APM_PAT_FOO, type=classic) butvalidate_virtual_package_existsstill returned False.validate_virtual_package_existsonly walks the GitHub Contents API._clone_with_fallbackwalks an authenticated PAT attempt then a plain HTTPS attempt that lets the system credential helper supply the token — two stages of auth-asymmetry today.Why this matters: validation is the gate on a deterministic action, so it must agree with that action. PROSE: "Grounding outputs in deterministic tool execution transforms probabilistic generation into verifiable action." — when the validator's view of "accessible" diverges from
git clone's view, the deterministic layer no longer dominates and the install pipeline never gets to run.Approach (WHAT)
validate_virtual_package_exists: when no marker file is found, requestGET /repos/{owner}/{repo}/contents/{path}?ref={ref}for the directory itself. A 200 means install will find the path.git ls-remote --heads --tags <url> <ref>fallback gated ondep_ref.reference is not None. Walks the same auth chain as_clone_with_fallback, so any token / credential that clones is accepted by validation.verbose_callbackfrom_validate_package_existsintovalidate_virtual_package_existsso each probe attempt is visible under--verbose([+] path@ref/[x] path@ref (reason)) without re-running withprintstatements.Implementation (HOW)
src/apm_cli/deps/github_downloader.py—validate_virtual_package_existsrewritten with a_probe(path)helper, marker probes, and two new fallbacks. Three new private helpers:_directory_exists_at_ref,_ref_exists_via_ls_remote,_ssh_attempt_allowed. The auth chain in_ref_exists_via_ls_remotedeliberately mirrors_clone_with_fallback's ordering and env builders — per-attempt env selection (self.git_envfor the token attempt vs_build_noninteractive_git_envfor the credential-helper attempt) is preserved so the locked-down env is only used for the token-bearing attempt.src/apm_cli/install/validation.py— one-line change: passverbose_callback=verbose_logso the new per-probe diagnostics flow through--verbose.tests/test_github_downloader.py— addsTestRefExistsViaLsRemote(8 tests) pinning the auth chain: token short-circuits, 403-on-token falls back to credential helper, no-token skips first attempt, all-fail returns False, empty output is a miss not a hit, Artifactory short-circuits, SSH skipped by default, SSH added whenProtocolPreference.SSHis set.CHANGELOG.md/docs/src/content/docs/getting-started/authentication.md— one Fixed entry under[Unreleased]and a paragraph describing that validation walks the same chain as install.Diagrams
Legend: control flow inside
validate_virtual_package_existsfor a virtual-subdirectory dep — markers fast-path where present, the two new fallbacks reconcile validation with_clone_with_fallback. Look first at theRefGatenode — that is the new gate that preserves path-typo rejection for unsuffixed deps.flowchart TD Start[validate_virtual_package_exists] --> Markers{Probe markers:<br/>apm.yml, SKILL.md,<br/>plugin.json, README.md} Markers -->|hit| Pass[return True] Markers -->|all 404| DirProbe{Contents API<br/>directory-exists<br/>at ref} DirProbe -->|200| Pass DirProbe -->|404 or non-GitHub| RefGate{dep_ref.reference<br/>is not None?} RefGate -->|no| Fail[return False] RefGate -->|yes| LsRemote{git ls-remote<br/>chain} LsRemote -->|attempt 1: token + git_env| Pass LsRemote -->|attempt 2: plain HTTPS<br/>credential helper| Pass LsRemote -->|attempt 3: SSH<br/>only if --ssh or<br/>--allow-protocol-fallback| Pass LsRemote -->|all fail| FailTrade-offs
ls-remotefallback on explicit#ref. Chose to requiredep_ref.reference is not None; rejected unconditional fallback because path-typo rejection on the default branch is a useful fast signal. Without the gate, a typo likeowner/repo/skilswould pass validation just because the repo and default branch resolve.git, rejected "go straight to ls-remote" because the Contents API path stays correct for the common case (public repos, fully-authorized PATs) and avoids spawning a git subprocess.ProtocolPreference.SSHor--allow-protocol-fallback, rejected always-attempt because users without SSH configured would see noisy validation output and possibly hung passphrase prompts.BatchMode=yesplusConnectTimeout=10further protects users who DO opt in.GitHubPackageDownloader, not a new class. Chose to colocate the new helpers with the existing auth state; rejected a separateValidatorclass because the auth resolver, git env, and protocol prefs already live on the downloader and a sibling class would duplicate that state.Benefits
apm install foo/bar/gee#sho) now succeeds: validation defers to install for any explicit-ref virtual subdir dep.apm install <pkg>.ls-remoteauth chain (token short-circuit, 403 fallback, SSH gating, empty-output miss, Artifactory bypass) so a future refactor cannot silently regress lenience.--verbosenow shows every probe attempt ([+] path@ref/[x] path@ref (reason)); future user reports are debuggable without re-running withprintstatements.github_downloader.py(not subject to the install-engine LOC budget) andcommands/install.pyis untouched.Validation
pytest— 8 new + 24 existing validation/architecture tests:New `TestRefExistsViaLsRemote` test list (all 8 PASSED)
Pre-existing failures observed on `main` (NOT introduced by this PR)
Reproduced on
mainwithgit stash && pytest <node-id>:tests/test_github_downloader.py::TestGitHubPackageDownloader::test_resolve_git_reference_commit— sandboxed clone of fake repo.tests/test_github_downloader.py::TestEnterpriseHostHandling::test_clone_fallback_respects_enterprise_host— same.tests/test_github_downloader.py::TestDownloaderCredentialFallback::test_credential_fill_used_when_no_env_token— pre-existing assertion ongithub_tokencache.tests/unit/test_install_command.py::TestInstallGlobalFlag::test_global_without_packages_and_no_manifest_errors— output-string assertion that depends on terminal width.How to test
GITHUB_APM_PAT_<ORG>) and a separate, more-authorized credential cached viagh auth setup-git/ OS keychain, runapm install foo/bar/gee#sho --verboseagainst a virtual subdirectory dep on a non-default branch. Expect[+] ls-remote ok via plain HTTPS w/ credential helper(orvia authenticated HTTPS) in the verbose output and successful install.apm install <org>/<repo>/<wrong-sub-name> --verbose(no#ref) and confirm validation still fails fast withnot accessible or doesn't exist— the gate on explicit-ref preserves path-typo rejection..venv/bin/python -m pytest tests/test_github_downloader.py::TestRefExistsViaLsRemote -vand confirm all 8 tests pass in <1s.feat/install-branch-flag: confirm this PR contains only the validation/install auth-alignment changes (no--branchflag, no newapm_cli/install/branch_flag.py, nocommands/install.pyedit).Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com