feat: persist transport flags via apm config

[Aaryan-Dadu]

Description

Users who authenticate to GitHub via SSH keys (no PAT configured) today
must pass --ssh or --allow-protocol-fallback on every apm install
invocation, or export environment variables in their shell profile which is a
cross-platform friction point (especially on Windows PowerShell / cmd).
This PR adds allow-protocol-fallback and ssh as first-class
apm config keys, so users can persist their transport preference once
and forget about it:

# Persist SSH preference for all future installs
apm config set ssh true
# Persist cross-protocol fallback
apm config set allow-protocol-fallback true

Changes:

• config.py: add get/set_allow_protocol_fallback(), get/set_ssh(), get_apm_allow_protocol_fallback() and get_apm_protocol_pref() resolution helpers that encode the full env > config > default chain
• commands/install.py: wire config into the transport-selection block;
• commands/config.py: register both keys in setters/getters/valid-keys, add them to all listing paths (rich table, plain-text, get all-keys)
• docs/reference/cli/config.md: new rows in keys table, resolution-order
  section for transport keys, usage examples
• docs/reference/environment-variables.md: new Transport and protocol section documenting APM_GIT_PROTOCOL and APM_ALLOW_PROTOCOL_FALLBACK
• tests: new test cases across test_config_command.py and new test_protocol_config_precedence.py

Closes #1243.

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

[Copilot]

Pull request overview

Adds support for persisting transport-selection preferences via apm config, so users no longer need to re-specify --ssh / --allow-protocol-fallback (or set env vars) on every apm install. This extends the existing config surface and wires the effective-value resolution into the install transport-selection path.

Changes:

• Introduces new persisted config keys: allow-protocol-fallback and ssh, plus helpers that resolve env > config > default.
• Wires apm install transport selection to read the effective preference/fallback from config helpers when CLI flags are not provided.
• Expands apm config CLI support (valid keys, set/get/list output) and adds docs + unit tests for precedence and CLI behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/apm_cli/config.py	Adds stored getters/setters for transport flags and env/config resolution helpers used by install.
src/apm_cli/commands/install.py	Uses config helpers to resolve protocol preference and fallback behavior when CLI flags are absent.
src/apm_cli/commands/config.py	Registers new keys for apm config set/get and includes them in listing output and valid-keys messaging.
tests/unit/test_config_command.py	Adds unit tests for new config keys, CLI routing, and listing/valid-keys behavior.
tests/unit/test_protocol_config_precedence.py	Adds precedence-focused tests for CLI/env/config/default resolution and round-trip config persistence.
docs/src/content/docs/reference/environment-variables.md	Documents new transport env vars and how they relate to persisted apm config keys.
docs/src/content/docs/reference/cli/config.md	Documents new config keys, their precedence, and usage examples.

Comments suppressed due to low confidence (3)

docs/src/content/docs/reference/environment-variables.md:41

• Documentation mismatch: this table says APM_GIT_PROTOCOL only accepts 'ssh' and 'https', but the implementation also accepts 'http' (and ProtocolPreference.from_str maps 'http' to HTTPS). Either document 'http' as accepted (and clarify how it's treated), or reject it in get_apm_protocol_pref() for consistency.

| Variable | Purpose | Default | Notes |
|---|---|---|---|
| `APM_GIT_PROTOCOL` | Preferred clone protocol for shorthand (`owner/repo`) dependencies. Accepted values: `ssh`, `https`. | unset | Equivalent to `--ssh` / `--https` flag. Resolution: CLI flag → env var → `ssh` key in `~/.apm/config.json` → git `insteadOf` rules → HTTPS. |
| `APM_ALLOW_PROTOCOL_FALLBACK` | Set to `1` (or `true`/`yes`/`on`) to enable the legacy cross-protocol fallback chain. When enabled, a failed clone is retried with the opposite protocol. | unset | Equivalent to `--allow-protocol-fallback`. Resolution: CLI flag → env var → `allow-protocol-fallback` key in `~/.apm/config.json` → `false`. |

tests/unit/test_protocol_config_precedence.py:185

• Non-ASCII Unicode right-arrow characters (U+2192) are used in this comment ('... -> fallthrough ...'), violating the printable-ASCII-only rule for source files. Replace with ASCII '->'.

    def test_unrecognised_env_val_falls_through_to_config(self):
        """An unrecognised APM_GIT_PROTOCOL value falls through to config."""
        import apm_cli.config as cfg_module

        with (
            patch.object(cfg_module, "get_ssh", return_value=True),
            patch.dict(os.environ, {"APM_GIT_PROTOCOL": "git"}),
        ):
            # 'git' is not in (ssh, https, http) → fallthrough to config (ssh=True)
            assert cfg_module.get_apm_protocol_pref() == "ssh"

docs/src/content/docs/reference/cli/config.md:73

• This section includes non-ASCII punctuation: an em dash (U+2014) in the list item and a Unicode ellipsis (U+2026) in 'apm config set ...'. Replace with ASCII equivalents (e.g., '-' and '...') to comply with the printable-ASCII-only docs rule.

`allow-protocol-fallback` and `ssh` follow the layered transport precedence:

1. CLI flag (`--allow-protocol-fallback`, `--ssh`) — highest priority
2. Environment variable (`APM_ALLOW_PROTOCOL_FALLBACK=1`, `APM_GIT_PROTOCOL=ssh`)
3. Value in `~/.apm/config.json` (`apm config set …`)
4. Built-in default (`false` / no preference)

[github-actions]

.

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 4.5M · ◷

[github-actions]

APM Review Panel: needs_rework

feat: persist transport flags via apm config -- eliminates the daily --ssh repetition for SSH-only and corporate users; two functional gaps (ghost call-sites, broken unset) need resolution before the feature delivers on its full promise.

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

This PR targets a concrete, well-documented pain point: users in SSH-only and corporate GHES environments who must re-pass --ssh on every apm install invocation. The layered resolution design (CLI flag > env var > config > default) is architecturally consistent with APM's existing get_apm_* helper pattern, the docs are accurate and well-placed, and the test surface for the new precedence logic is solid. The panel is broadly positive on direction and scope.

Two functional gaps demand attention before this ships as-is. First, the auth-expert identified that github_downloader.py and validation.py retain direct calls to protocol_pref_from_env() / is_fallback_allowed(), bypassing the new config layer entirely. A user who runs apm config set ssh true will get SSH transport from the install.py path but revert to env-only semantics on any code path that constructs GithubDownloader without explicit args or that calls validation.py:317 directly. This is a user-visible correctness break: the config key appears to work but silently fails on a subset of calls. The test-coverage-expert independently confirmed no integration-tier test guards the full config-to-install wiring, which means neither the ghost call-site gap nor the end-to-end user promise "set once, install forever" is defended automatically. Second, the test-coverage-expert verified (evidence: outcome=missing, severity=recommended) that apm config unset allow-protocol-fallback and apm config unset ssh fall through to logger.error / sys.exit(1) because no unset branch exists for these keys. This is a silent, undocumented failure that breaks a standard CLI contract users rely on.

The remaining panel signals -- env-var constant duplication (python-architect + supply-chain, converged), http aliasing undocumented (supply-chain, auth, doc-writer, test-coverage, converged), apm config get showing stored not effective value (cli-logging), ssh key naming (devx-ux), noise in apm config output (devx-ux + cli-logging, converged), and missing CHANGELOG entry (oss-growth) -- are all recommended follow-ups that do not break the happy path. The http alias issue carries a mild future-caller risk flagged by both the auth-expert and supply-chain-security-expert, but ProtocolPreference.from_str currently absorbs it safely; normalizing or documenting it is a low-cost hardening step. The ssh key naming question (prefer-ssh vs ssh) raised by devx-ux is the only area where a pre-merge decision is worth making: renaming after users have set ssh in dotfiles is a breaking change, while renaming now costs only a search-replace. The maintainer should make a deliberate call here rather than letting it ship by default.

Dissent. The devx-ux-expert and doc-writer mildly disagree on unset ergonomics: devx-ux calls apm config set ssh false non-idiomatic and prefers unset, while doc-writer notes that unset is currently unimplemented and suggests documenting set false as the workaround. The test-coverage-expert's evidence confirms unset is broken (exit 1), so devx-ux wins on principle but doc-writer's workaround is the pragmatic near-term answer -- implement unset properly and then document it as idiomatic.

Aligned with: Portable by manifest (config-layer persistence means transport preference travels with the user's environment rather than living only in shell aliases), Secure by default (both keys default to false; no transport downgrade occurs without explicit opt-in), Governed by policy (layered resolution gives platform teams a clean env-var override surface enforceable centrally), Pragmatic as npm (npm's prefer-* convention for protocol keys signals protocol preference over capability toggle -- the bare ssh key reads ambiguously).

Growth signal. "Set once, install forever" is the punchy, tweetable story for SSH-only and GHES corporate users -- a high-value segment that APM has not yet explicitly targeted in release communications. The missing CHANGELOG entry is the single highest-leverage gap: the prior SSH/transport feature (#778) set a precedent for a detailed changelog line that became the reference for downstream narrative. This PR should get an equivalent entry under [Unreleased] before it ships so the release post can lead with this quality-of-life beat and the FAQ entry ("Do I need to pass --ssh every time? No, run apm config set ssh true once") can be grounded in a specific version.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	2	Clean, well-scoped feature; env-var constant duplication is the only compounding architectural debt.
CLI Logging Expert	0	1	3	New keys integrate cleanly; apm config get shows stored, not effective, value -- debugging friction for the target user segment.
DevX UX Expert	0	3	2	Solves a real pain point; key name ssh is ambiguous pre-merge decision; unconditional default-false display adds noise.
Supply Chain Security Expert	0	2	1	No blocking supply-chain issues; env-var constant duplication risks silent name-drift; http alias undocumented.
OSS Growth Hacker	0	1	2	Genuine friction-killer for SSH-only and corporate users; missing CHANGELOG entry is the highest-leverage gap before ship.
Auth Expert	0	2	1	Config layer correctly wired in install.py; ghost call-sites in github_downloader.py and validation.py bypass it entirely.
Doc Writer	0	1	4	Docs accurate and well-structured; APM_GIT_PROTOCOL Notes column omits http as accepted alias.
Test Coverage Expert	0	2	2	Install pipeline config-layer wiring has no integration test; apm config unset for new keys silently exits 1.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Auth Expert] Ghost call-sites in github_downloader.py and validation.py bypass the new config layer and silently revert to env-only transport semantics. -- Users who set ssh=true in config will get inconsistent transport behavior depending on which internal code path is taken; this is the most user-visible correctness gap in the PR and the only one that could cause silent mis-behavior post-merge.

2. [Test Coverage Expert] apm config unset allow-protocol-fallback and apm config unset ssh silently exit 1 -- the unset command handler has no branch for these keys and no backend implementation. -- Shipping a config key that cannot be unset without hand-editing JSON breaks a standard CLI contract and will generate support noise.

3. [Test Coverage Expert] No integration-tier test validates the full config-to-install wiring -- the user promise "apm config set ssh true causes subsequent installs to use SSH" is unguarded at the integration-with-fixtures tier. -- Unit mocks validate the helper in isolation but cannot catch the ghost call-site regression or any future refactor that breaks the wiring.

4. [OSS Growth Hacker] No CHANGELOG entry exists for this feature under [Unreleased]. -- The prior SSH/transport feature (Transport Selection v1: honor user-specified transport, no silent downgrade #778) set the changelog precedent that feeds release narrative and the FAQ; missing this entry means the feature ships silently and the existing narrative for Transport Selection v1: honor user-specified transport, no silent downgrade #778 feels incomplete.

5. [DevX UX Expert] Config key ssh is ambiguous -- prefer prefer-ssh to match npm conventions and signal protocol preference rather than capability toggle. -- Renaming after users have set ssh in dotfiles is a breaking change; a deliberate decision now costs only a search-replace, deferring forecloses the option.

Architecture

classDiagram
    direction LR

    class config {
        <<Module>>
        +get_allow_protocol_fallback() bool
        +set_allow_protocol_fallback(enabled) None
        +get_ssh() bool
        +set_ssh(enabled) None
        +get_apm_allow_protocol_fallback() bool
        +get_apm_protocol_pref() str|None
        +get_config() dict
        +update_config(patch) None
    }
    note for config "Resolution helpers follow env > config > default"

    class transport_selection {
        <<Module>>
        +ENV_PROTOCOL: str
        +ENV_ALLOW_FALLBACK: str
        +protocol_pref_from_env() ProtocolPreference
        +is_fallback_allowed() bool
    }
    note for transport_selection "Declares same env-var names as config.py"

    class ProtocolPreference {
        <<Enum>>
        NONE
        SSH
        HTTPS
        +from_str(value) ProtocolPreference
    }

    class install_cmd {
        <<Module>>
        +install(...)
    }

    class config_cmd {
        <<Module>>
        +VALID_KEYS: dict
        +cmd_config_set(...)
        +cmd_config_get(...)
    }

    install_cmd ..> config : "get_apm_protocol_pref()\nget_apm_allow_protocol_fallback()"
    install_cmd ..> transport_selection : "ProtocolPreference"
    transport_selection *-- ProtocolPreference : defines
    config_cmd ..> config : "set_allow_protocol_fallback()\nset_ssh()"

    class config:::touched
    class install_cmd:::touched
    class config_cmd:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Loading

flowchart TD
    A([apm install --ssh / --allow-protocol-fallback / neither]) --> B{CLI flag set?}
    B -- yes --> C[ProtocolPreference.SSH / .HTTPS CLI flag wins]
    B -- no --> D[config.get_apm_protocol_pref]
    D --> E{APM_GIT_PROTOCOL env set?}
    E -- ssh/https/http --> F[return env value]
    E -- not set --> G{config.get_ssh from config.json}
    G -- true --> H[return ssh]
    G -- false --> I[return None - ProtocolPreference.NONE]
    F --> J[ProtocolPreference.from_str]
    H --> J
    I --> J
    C --> K{allow_protocol_fallback CLI flag set?}
    J --> K
    K -- no --> L[config.get_apm_allow_protocol_fallback]
    L --> M{APM_ALLOW_PROTOCOL_FALLBACK env set?}
    M -- 1/true/yes/on --> N[allow_protocol_fallback = True]
    M -- not set --> O{config allow_protocol_fallback key}
    O -- true --> N
    O -- false --> P[allow_protocol_fallback = False]
    K -- yes --> N
    N --> Q[TransportSelector.select]
    P --> Q

Loading

sequenceDiagram
    actor User
    participant CLI as apm install
    participant Config as config.py
    participant FS as ~/.apm/config.json
    participant TS as TransportSelector

    User->>CLI: apm config set ssh true
    CLI->>Config: set_ssh(True)
    Config->>FS: write {"ssh": true}

    User->>CLI: apm install pkg
    CLI->>Config: get_apm_protocol_pref()
    Config->>FS: read config
    FS-->>Config: {"ssh": true}
    Config-->>CLI: "ssh"
    CLI->>TS: ProtocolPreference.from_str("ssh") - SSH
    CLI->>Config: get_apm_allow_protocol_fallback()
    Config-->>CLI: False
    CLI->>TS: select(pref=SSH, fallback=False)
    TS-->>CLI: git clone via SSH

Loading

Recommendation

Address the two functional gaps -- ghost call-sites in github_downloader.py / validation.py and the broken apm config unset handler -- before this merges. Both are straightforward fixes: wire the config helpers into GithubDownloader's default-arg resolution path (mirroring the install.py change), add unset_ssh / unset_allow_protocol_fallback functions to config.py, and add the corresponding unset branches to the config command handler. Add a CHANGELOG entry under [Unreleased] in the same pass. Once those three items land, the maintainer should make a deliberate call on the ssh vs prefer-ssh naming question before merge -- that is the one decision that cannot be cleanly deferred. The remaining panel findings (integration test, effective-value display in apm config get, http alias doc, constant extraction, output noise) are solid post-merge follow-up material.

---

Full per-persona findings

Python Architect

• [recommended] Env-var name constants are duplicated across two modules -- extract to a shared leaf at src/apm_cli/config.py:9
APM_ALLOW_PROTOCOL_FALLBACK and APM_GIT_PROTOCOL are declared as ENV_ALLOW_FALLBACK / ENV_PROTOCOL in transport_selection.py AND as _ENV_ALLOW_PROTOCOL_FALLBACK / _ENV_GIT_PROTOCOL in config.py. If either set is renamed, the two modules silently read different environment variables with no compiler or test catching the divergence. Fix: a zero-dependency src/apm_cli/deps/_env_keys.py (no imports) lets both modules import safely without a circular-import cycle.
  Suggested: Create src/apm_cli/deps/_env_keys.py with ENV_PROTOCOL = "APM_GIT_PROTOCOL" and ENV_ALLOW_FALLBACK = "APM_ALLOW_PROTOCOL_FALLBACK", then import from it in both modules.

• [nit] get_apm_protocol_pref() can leak the raw string "http" -- normalize to "https" at src/apm_cli/config.py:220
  Docstring promises "ssh", "https", or None. APM_GIT_PROTOCOL=http returns raw "http". ProtocolPreference.from_str absorbs it correctly today, but any future caller pattern-matching on the return value will silently mis-handle it.
  Suggested: return "https" if env_val == "http" else env_val

• [nit] Two unjustified function-body lazy imports in install.py:1307
config.py is a leaf module with no circular-import risk. The underscore-alias inline import pattern signals "circular import workaround" -- using it without a cycle inverts the convention's meaning for future readers.

CLI Logging Expert

• [recommended] apm config get / show surfaces stored value, not effective value at src/apm_cli/commands/config.py
get_allow_protocol_fallback() / get_ssh() are called for display. If a user has APM_ALLOW_PROTOCOL_FALLBACK=1 in their shell but false in config.json, apm config get allow-protocol-fallback prints False while apm install honours True. Call the env-resolving wrappers (get_apm_allow_protocol_fallback / get_apm_protocol_pref) for display, with source annotation when the env var wins.

• [nit] Fallback-path labels use raw kebab-case key, inconsistent with surrounding labels
  Surrounding fallback lines use Title Case (APM CLI Version, Temp Directory). New lines emit allow-protocol-fallback: and ssh: as raw keys.

• [nit] Both new keys shown unconditionally at default False, adding noise to every user's output
  Follow the temp-dir pattern: only show when the value is non-default. apm config get <key> can still show the explicit value on direct request.

• [nit] Python str(bool) renders True/False; convention expects true/false
  Config values are documented as lowercase booleans in the docs table. Use str(value).lower() at output sites.

DevX UX Expert

• [recommended] Config key ssh is ambiguous -- prefer prefer-ssh at src/apm_cli/commands/config.py
  The bare key ssh reads as a feature toggle. npm uses the prefer-* pattern for protocol preference keys (prefer-offline, prefer-dedupe). prefer-ssh maps immediately onto the mental model. Renaming after users have set ssh in dotfiles is a breaking change.
  Suggested: Rename config key to prefer-ssh (internal: prefer_ssh). Keep --ssh as the CLI flag unchanged.

• [recommended] apm config always prints both keys at False, adding noise to every user's output
  Violates "quiet on the happy path" -- noise for the 95% of users who have never changed these settings. Follow the temp-dir pattern.

• [recommended] SSH auth error path doesn't hint at apm config set ssh true at src/apm_cli/commands/install.py
  User who encounters Permission denied (publickey) or HTTPS clone failure has no way to discover the config key without reading docs. npm prints actionable hints at the point of failure.
  Suggested: Add a transport-error hint in the clone failure branch pointing at apm config set ssh true.

• [nit] Docs example resets preference with set ssh false instead of unset at docs/src/content/docs/reference/cli/config.md
apm config unset ssh is idiomatic; set false leaves an explicit false in config.json while unset removes the entry entirely.

• [nit] allow-protocol-fallback called "legacy" in PR body but docs don't signal deprecation
  If genuinely legacy, add a deprecation note so users know to prefer the explicit --ssh path going forward.

Supply Chain Security Expert

• [recommended] Env-var constant duplication risks silent name-drift at src/apm_cli/config.py
_ENV_ALLOW_PROTOCOL_FALLBACK and _ENV_GIT_PROTOCOL are copy-pasted strings in config.py (lines 11-12) while transport_selection.py declares ENV_ALLOW_FALLBACK and ENV_PROTOCOL with the same string values. No compile-time or test-time assertion ensures they stay in sync.
  Suggested: Add a standalone test asserting config._ENV_ALLOW_PROTOCOL_FALLBACK == transport_selection.ENV_ALLOW_FALLBACK, or refactor into a zero-dependency _env_keys.py module.

• [recommended] get_apm_protocol_pref() accepts and returns "http" but docstring omits it at src/apm_cli/config.py:220
ProtocolPreference.from_str maps "http" to HTTPS correctly today, but any future call-site using the raw string would open an unauthenticated HTTP clone path. Normalize "http" to "https" before returning, or drop it from the accepted set.

• [nit] Raw config value for allow_protocol_fallback returned without bool-coercion at src/apm_cli/config.py:202
  A manually edited config.json with string "yes" or integer 1 activates the flag without going through the env-var truthy-set check. Wrap in bool() or validate on set_allow_protocol_fallback.

OSS Growth Hacker

• [recommended] Missing CHANGELOG entry in [Unreleased] at CHANGELOG.md
  The prior SSH/transport feature (Transport Selection v1: honor user-specified transport, no silent downgrade #778) got a detailed changelog line. Suggested: "apm config set ssh true and apm config set allow-protocol-fallback true persist transport preferences to ~/.apm/config.json; SSH-only and corporate users no longer need to pass --ssh on every apm install invocation. Closes Support persisting CLI flags via apm config (e.g. allow-protocol-fallback, ssh) #1243. (feat: persist transport flags via apm config #1308)"

• [nit] No first-run hook -- SSH-only users won't find this without reading docs
  Consider a one-sentence callout on the apm install reference page pointing at apm config set ssh true.

• [nit] Transport section placement on env-vars page buries the lead for new users
  Moving "Transport and protocol" above "Registry (MCP and proxy)" keeps it closer to the auth/credential content where frustrated SSH users are already looking.

Auth Expert

• [recommended] Ghost call-sites bypass config layer: github_downloader.py and validation.py still call protocol_pref_from_env() / is_fallback_allowed() directly at src/apm_cli/deps/github_downloader.py
GithubDownloader.__init__ falls back to protocol_pref_from_env() and is_fallback_allowed() when the caller passes protocol_pref=None / allow_fallback=None. These functions read only the env var -- they never consult ~/.apm/config.json. Any caller that constructs GithubDownloader without explicit values silently reverts to env-only semantics. validation.py:317 has the same gap for is_fallback_allowed().

• [recommended] get_apm_protocol_pref() silently accepts "http" but docstring promises only "ssh"/"https" at src/apm_cli/config.py:220
APM_GIT_PROTOCOL=http returns raw "http". In install.py this is immediately fed to ProtocolPreference.from_str("http") which maps it to ProtocolPreference.HTTPS -- no credentials travel over cleartext HTTP through this path. But if any future code uses the raw return value without passing it through from_str(), it could reach the _HTTP transport attempt. Reject "http" with a warning, or explicitly document and test the alias.

• [nit] Late import inside function body for config helpers in install.py:1307 is unnecessary
  config.py is already imported at the module level in install.py; no circular-import risk justifies the inline pattern.

Doc Writer

• [recommended] APM_GIT_PROTOCOL Notes column omits "http" as accepted value at docs/src/content/docs/reference/environment-variables.md
  The code accepts "http" and ProtocolPreference.from_str maps it to HTTPS. Change "Accepted values: ssh, https." to "Accepted values: ssh, https, http (alias for https)."

• [nit] apm config unset section doesn't explicitly mention new keys are reset via set false
  A reader who tries apm config unset ssh will get an error with no explanation. Add: "This includes allow-protocol-fallback and ssh -- use apm config set allow-protocol-fallback false or apm config set ssh false to restore defaults."

• [nit] allow-protocol-fallback example missing reset comment
  The ssh example includes # Reset to default: apm config set ssh false but the allow-protocol-fallback example does not. Add for consistency.

• [nit] Table ordering -- transport booleans appear between path-type keys
  No change required for this PR. If the table grows, consider grouping by type.

• [nit] Transport section placement in env-vars page is reasonable
  No change required. Placement after Authentication and before Registry is logical.

Test Coverage Expert

• [recommended] apm config unset allow-protocol-fallback / apm config unset ssh silently fails with exit 1
  The unset command handler has explicit branches for temp-dir and copilot-cowork-skills-dir. allow-protocol-fallback and ssh have no branch, so apm config unset allow-protocol-fallback falls through to logger.error("Unknown configuration key") and sys.exit(1). No unset_allow_protocol_fallback or unset_ssh functions exist in config.py (grep confirmed). No test exercises runner.invoke(config, ["unset", "allow-protocol-fallback"]).
  Proof (missing at unit): tests/unit/test_config_command.py::TestConfigUnset::test_unset_allow_protocol_fallback_exits_0 -- proves: Running apm config unset allow-protocol-fallback succeeds with exit 0, not silently fails with exit 1 [devx]
  Suggested: Add unset_allow_protocol_fallback and unset_ssh to config.py, wire them in the unset handler, add tests.

• [recommended] Install pipeline config-layer wiring has no integration test
  The diff in install.py replaces protocol_pref_from_env() / is_fallback_allowed() with config helpers. All tests are unit-level with patch.object mocks. tests/integration/test_transport_selection_integration.py exists but grep for config/set_ssh/get_apm_protocol_pref returned 0 hits.
  Proof (missing at integration-with-fixtures): tests/integration/test_transport_selection_integration.py::test_config_ssh_true_routes_install_to_ssh_transport -- proves: apm config set ssh true causes apm install to attempt SSH URLs -- the full config-to-install wiring is live, not just the helper in isolation [portability-by-manifest, devx]
  Suggested: Add fixture-based integration test: set_ssh(True) against isolated config file, invoke install command, assert attempted clone URL starts with git@ or `(redacted)

• [nit] APM_GIT_PROTOCOL=http path is untested
  config.py:220 accepts "http". Grep of test files for APM_GIT_PROTOCOL.*http returned 0 hits.
  Proof (missing at unit): tests/unit/test_protocol_config_precedence.py::TestProtocolPrefPrecedence::test_env_var_http_returns_http_string

• [nit] Precedence tests simulate CLI flag with Python variable, not real CLI invocation
  Tests like test_cli_flag_true_wins_over_everything set cli_flag = True and compute cli_flag or get_apm_allow_protocol_fallback(). Correctly tests the OR expression in install.py but does not verify Click wires the --allow-protocol-fallback option to the parameter.
  Proof (passed at unit): tests/unit/test_protocol_config_precedence.py::TestAllowProtocolFallbackPrecedence::test_cli_flag_true_wins_over_everything -- proves: OR expression short-circuits correctly when CLI flag is True [devx]

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

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 4.5M · ◷

[Aaryan-Dadu]

The copilot's panel-review flagged that the config key name ssh is ambiguous i.e., it reads as a capability toggle rather than a preference signal. prefer-ssh would align with npm conventions. So whichever will be preferred I can incorporate that.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

tests/unit/test_protocol_config_precedence.py:194

• This comment contains a non-ASCII Unicode arrow. Repo encoding guidance requires printable ASCII in source files; replace with ASCII (e.g., '->').

            patch.dict(os.environ, {"APM_GIT_PROTOCOL": "git"}),
        ):
            # 'git' is not in (ssh, https, http) → fallthrough to config (ssh=True)
            assert cfg_module.get_apm_protocol_pref() == "ssh"

docs/src/content/docs/reference/cli/config.md:73

• This section introduces non-ASCII characters (an em dash and a Unicode ellipsis in apm config set ...). Repo encoding guidance requires printable ASCII in docs; replace with ASCII equivalents (e.g., '-' and '...').

`allow-protocol-fallback` and `ssh` follow the layered transport precedence:

1. CLI flag (`--allow-protocol-fallback`, `--ssh`) — highest priority
2. Environment variable (`APM_ALLOW_PROTOCOL_FALLBACK=1`, `APM_GIT_PROTOCOL=ssh`)
3. Value in `~/.apm/config.json` (`apm config set …`)
4. Built-in default (`false` / no preference)

[danielmeppiel]

Please rename key to prefer-ssh - thank you!

[Aaryan-Dadu]

Please rename key to prefer-ssh - thank you!

I have made the changes @danielmeppiel but the ruff checks are failing due to changes unrelated to this PR.

[github-actions]

APM Review Panel: ship_with_followups

PR #1308 lets SSH-first teams run 'apm config set prefer-ssh true' once and never pass --ssh again; two doc correctness bugs must be fixed before merge.

cc @danielmeppiel -- a fresh advisory pass is ready for your review.

This PR delivers a clean config-layer for transport persistence: env vars retain precedence, config file sits beneath them, and code defaults remain unchanged. The layering is sound and the auth-expert confirms no protocol-bypass regressions. The design is directionally correct and the user payoff is concrete. However, two documentation correctness bugs flagged by doc-writer are load-bearing: environment-variables.md names the config key as 'ssh' when it is actually 'prefer-ssh', meaning any user who hand-edits config.json following the docs will silently set an unrecognised key and get no effect. The second bug -- config.md line 48 contradicting its own examples on whether booleans are valid unset targets -- creates a support burden. Both must be fixed before this ships.

The devx-ux-expert raised the stale _CONFIG_KEY_DISPLAY_NAMES entry as blocking, but supply-chain-security-expert and cli-logging-expert independently classified it as nit/recommended dead code. The map is defined but not consumed by any live code path, so the practical blast radius is zero; fixing the stale 'ssh' entry is still correct hygiene but does not gate shipment. The two missing integration tests (test-coverage-expert, both 'missing' on an install-pipeline surface) are meaningful gaps: the config-file branch of allow-protocol-fallback through _validate_package_exists and the constructor config-aware default both lack fixtures-level coverage. These should be filed as follow-up issues, not treated as blockers, because the unit-level mock coverage is present and the feature logic is correct.

The supply-chain-security finding on config directory permissions (os.makedirs without mode=0o700) is the one finding with a genuine security implication on multi-user systems. It is classified recommended rather than blocking by the panelist, and no auth-expert objection was raised, but the fix is a one-liner and should be bundled in this PR rather than deferred. The CI/shared-home warning on allow-protocol-fallback and the FALLBACK_HINT update are similarly low-cost hardening steps worth landing now.

Dissent. devx-ux-expert classified the stale _CONFIG_KEY_DISPLAY_NAMES entry ("ssh": "ssh") as blocking; cli-logging-expert classified it recommended; supply-chain-security-expert classified it nit. CEO sides with cli-logging-expert and supply-chain-security-expert: the dict is dead code with no consumer, so its staleness has no runtime impact. The correct key ("prefer_ssh": "prefer-ssh") should be inserted and the stale entry removed, but this is not a ship-gate. Separately, doc-writer raised two blocking doc findings; no other panelist disputed them and CEO affirms them at blocking weight -- the env-vars.md wrong key name is a direct user-harm correctness bug.

Aligned with: Secure by default -- defaults are False for both new flags; the config directory permissions gap (os.makedirs without mode restriction) slightly undermines this on multi-user hosts and the one-liner fix should land in this PR. Pragmatic as npm -- persistent config flags reduce per-invocation friction for SSH-first and fallback-dependent teams; this is exactly the "set once, forget" ergonomics APM should offer.

Growth signal. OSS growth hacker identifies this as a launch-beat candidate for the enterprise/SSH cohort: 'APM now remembers your SSH preference.' The concrete hook -- 'SSH-first teams can run apm config set prefer-ssh true once and drop --ssh from every install' -- should lead the CHANGELOG [Unreleased] entry and any release-announcement copy. Long-term signal: apm init detecting SSH agent presence and offering to set prefer-ssh true during onboarding would close the discovery gap flagged in installation.md.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	2	Solid env>config>default layering; stale display-name entry, duplicated env-var strings, and repeated unset boilerplate are the three concrete clean-ups worth making before the module grows further.
CLI Logging Expert	0	3	2	Two recommended fixes (always-on bool display adds noise; Python-capitalised True/False breaks CLI convention) plus one stale dead-code entry to remove; fallback-path label inconsistency is a nit.
DevX UX Expert	1	3	1	One correctness bug in the display-name map, one doc contradiction on unset, one stale env-var resolution string, and a protocol asymmetry gap that will confuse HTTPS-preferring users.
Supply Chain Security Expert	0	3	1	Defaults are secure (False); three recommended hardening gaps: config dir world-readable, stale FALLBACK_HINT, and silent CI persistence risk. One dead-code nit. No blocking issues.
OSS Growth Hacker	0	2	2	Solid friction-reducer for SSH/enterprise users; authentication.md still shows the old env-var escape hatch without mentioning the new persistent config alternative -- a quiet conversion miss.
Auth Expert	0	1	1	No auth bypasses or security regressions; config-aware transport resolution is consistent with AuthResolver initialization and env-var precedence is preserved.
Doc Writer	2	1	1	One blocking correctness bug (wrong config key name in env-var doc), one internal contradiction in config.md's unset description vs examples, one CHANGELOG structure issue, and a nit.
Test Coverage Expert	0	2	1	Precedence-chain unit coverage is solid; the persisted-config branch of the install-pipeline integration test is missing at the floor tier.

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.

Top 5 follow-ups

1. [Doc Writer] (blocking-severity) Fix environment-variables.md: change 'ssh key' to 'prefer-ssh key' in APM_GIT_PROTOCOL resolution chain -- Users who hand-edit config.json following the current docs will silently set an unrecognised key and see no effect. Direct user-harm correctness bug; should be fixed before merge.
2. [Doc Writer] (blocking-severity) Fix config.md line 48: unset description must include prefer-ssh and allow-protocol-fallback as valid targets -- The current text contradicts the code examples two pages later. A user reading the description will believe unset does not work for boolean keys and reach for the wrong workaround.
3. [Supply Chain Security Expert] Set mode=0o700 in os.makedirs and chmod 0o600 on config.json after initial creation -- One-liner fix; world-readable config on a multi-user host leaks allow_protocol_fallback state. Low effort, real hardening.
4. [Test Coverage Expert] Add integration-with-fixtures tests for (1) config-file path of allow-protocol-fallback through _validate_package_exists and (2) constructor config-aware SSH default -- Both evidence blocks returned 'missing' on an install-pipeline surface. Unit mocks cover the logic but not the wiring; a future refactor could silently break either path.
5. [CLI Logging Expert] Suppress False rows in 'config show' and render booleans as lowercase true/false -- Always-visible False rows add noise for users who never configured transport. Python-capitalised True/False breaks CLI convention. Both are low-effort polish fixes that reinforce the pragmatic-as-npm positioning.

Architecture

classDiagram
    direction TB

    class config_module {
        <<Module>>
        +get_apm_allow_protocol_fallback() bool
        +get_apm_protocol_pref() str|None
        +get_allow_protocol_fallback() bool
        +set_allow_protocol_fallback(bool)
        +unset_allow_protocol_fallback()
        +get_prefer_ssh() bool
        +set_prefer_ssh(bool)
        +unset_prefer_ssh()
        -_ENV_ALLOW_PROTOCOL_FALLBACK str
        -_ENV_GIT_PROTOCOL str
    }
    note for config_module "env > config > default resolver\nenv-var strings duplicated from transport_selection"

    class transport_selection_module {
        <<Module>>
        +is_fallback_allowed(cli_flag, env) bool
        +protocol_pref_from_env(env) ProtocolPreference
        +ENV_ALLOW_FALLBACK str
        +ENV_PROTOCOL str
    }
    note for transport_selection_module "ENV_ALLOW_FALLBACK / ENV_PROTOCOL\nduplicate config_module private constants"

    class TransportSelector {
        <<PureDecisionEngine>>
        +select(host, pref, allow_fallback) TransportPlan
    }

    class GitHubPackageDownloader {
        <<Facade>>
        -_protocol_pref ProtocolPreference
        -_allow_fallback bool
        -_transport_selector TransportSelector
        +__init__(auth_resolver, transport_selector, protocol_pref, allow_fallback)
    }
    note for GitHubPackageDownloader "Deferred imports of get_apm_protocol_pref\nand get_apm_allow_protocol_fallback in __init__\nwork around circular import"

    class commands_config_module {
        <<CLIGroup>>
        +set(key, value)
        +get(key)
        +unset(key)
        -_CONFIG_KEY_DISPLAY_NAMES dict
    }
    note for commands_config_module "_CONFIG_KEY_DISPLAY_NAMES has stale\n'ssh':'ssh' entry; missing 'prefer_ssh':'prefer-ssh'"

    class config_module:::touched
    class commands_config_module:::touched
    class GitHubPackageDownloader:::touched
    class transport_selection_module:::touched

    classDef touched fill:#fff3b0,stroke:#d47600

    config_module ..> transport_selection_module : duplicates env-var strings
    GitHubPackageDownloader *-- TransportSelector : owns
    GitHubPackageDownloader ..> config_module : deferred import in __init__
    commands_config_module ..> config_module : lazy imports via _get_config_setters/getters

Loading

flowchart TD
    A(["CLI: apm install / apm config set prefer-ssh true"]) --> B

    subgraph commands_config ["commands/config.py"]
        B["unset / set / get subcommand"] --> C["_get_config_setters() / _get_config_getters()\n[lazy import from config.py]"]
    end

    subgraph config_py ["config.py"]
        C --> D["set_prefer_ssh(bool) / set_allow_protocol_fallback(bool)\nupdate_config({key: val})\n[FS] writes ~/.apm/config.json"]
        E["get_apm_protocol_pref()"] --> F{"APM_GIT_PROTOCOL env set?"}
        F -- yes --> G["return env value\n(ssh/https/http)"]
        F -- no --> H{"get_prefer_ssh() == True?"}
        H -- yes --> I["return 'ssh'"]
        H -- no --> J["return None"]

        K["get_apm_allow_protocol_fallback()"] --> L{"_parse_allow_protocol_fallback_env\n(APM_ALLOW_PROTOCOL_FALLBACK) is not None?"}
        L -- yes --> M["return parsed bool"]
        L -- no --> N["get_allow_protocol_fallback()\n[FS] reads ~/.apm/config.json via get_config()"]
    end

    subgraph downloader ["deps/github_downloader.py"]
        O["GitHubPackageDownloader.__init__\nprotocol_pref=None, allow_fallback=None"] --> P["[deferred import] get_apm_protocol_pref()"]
        O --> Q["[deferred import] get_apm_allow_protocol_fallback()"]
        P --> E
        Q --> K
        P --> R["ProtocolPreference.from_str(pref_str)"]
        Q --> S["self._allow_fallback = bool"]
    end

    subgraph validation ["install/validation.py"]
        T["_validate_package_exists()"] --> U["get_apm_allow_protocol_fallback()"]
        U --> K
    end

    subgraph install_cmd ["commands/install.py"]
        V["_build_downloader() / install flow"] --> W["get_apm_protocol_pref()\nget_apm_allow_protocol_fallback()"]
        W --> E
        W --> K
    end

Loading

Recommendation

Ship after the two doc correctness bugs are resolved in this PR (wrong key name in environment-variables.md; unset description contradiction in config.md) and the config directory permissions one-liner is added. Everything else -- display-name dead-code cleanup, bool rendering, FALLBACK_HINT update, CI warning, integration test gaps -- should be filed as follow-up issues and linked from the PR description. The feature logic is sound, the security defaults are correct, and the user payoff is real. Do not let the accumulation of nits delay a change that directly unblocks SSH-first enterprise teams.

---

Full per-persona findings

Python Architect

• [recommended] _ENV_ALLOW_PROTOCOL_FALLBACK / _ENV_GIT_PROTOCOL are declared twice -- once in config.py and once in transport_selection.py at src/apm_cli/config.py:11
  The comment says 'to avoid a circular import' but duplicating the string literals is a latent drift risk: if either side renames the constant the other side silently diverges. Canonical fix: extract both constants to a thin third module (e.g. apm_cli/deps/transport_constants.py) with no imports from either consumer.
  Suggested: Create src/apm_cli/deps/transport_constants.py containing the two constants; import from there in both config.py and transport_selection.py.

• [recommended] Four unset_* functions in config.py repeat the same invalidate->load->del->write->invalidate pattern verbatim at src/apm_cli/config.py:136
  DRY threshold exceeded (4 call sites). A _unset_config_key(key: str) private helper eliminates all repetition and makes future bug fixes single-site.
  Suggested: Add def _unset_config_key(key: str) -> None: and delegate from all four unset_* functions.

• [nit] _CONFIG_KEY_DISPLAY_NAMES still contains "ssh": "ssh" -- leftover from before the prefer_ssh rename at src/apm_cli/commands/config.py:24
  Dead entry, correct key should be "prefer_ssh": "prefer-ssh".
  Suggested: Replace "ssh": "ssh" with "prefer_ssh": "prefer-ssh".

• [nit] In-constructor deferred imports in GitHubPackageDownloader.__init__ signal an unresolved layering tension at src/apm_cli/deps/github_downloader.py:188
  Workaround for circular import; resolves cleanly once constants are extracted to a shared module.

CLI Logging Expert

• [recommended] Stale "ssh": "ssh" entry in _CONFIG_KEY_DISPLAY_NAMES should be removed at src/apm_cli/commands/config.py:24
  Maps to nothing used elsewhere and will silently diverge if the dict is ever iterated for validation or documentation generation.
  Suggested: Delete "ssh": "ssh". Add "prefer_ssh": "prefer-ssh" if the dict is used for normalisation.

• [recommended] allow-protocol-fallback and prefer-ssh always appear in config show, even when at their defaults at src/apm_cli/commands/config.py:137
temp-dir is guarded by if _temp_dir_val:. The new bools unconditionally emit rows showing False with no signal that these are defaults, not explicit choices. Violates signal-to-noise rule.
  Suggested: Mirror the temp-dir pattern: only emit rows when value deviates from the default (True).

• [recommended] Booleans render as Python-capitalised True/False instead of lowercase true/false at src/apm_cli/commands/config.py:137
  CLI conventions (POSIX, YAML, JSON, every other config tool) use lowercase. Creates friction for users comparing output to documented values.
  Suggested: Use str(val).lower() or a _fmt_bool helper throughout.

• [nit] Fallback click.echo path uses kebab-case key names for new entries but a human-readable label for temp-dir at src/apm_cli/commands/config.py:181
  Minor inconsistency in the fallback output path.
  Suggested: Use human-readable labels: ' Allow Protocol Fallback: ...' and ' Prefer SSH Transport: ...'.

• [nit] apm config get prints False for unset booleans but Not set (using system default) for unset temp-dir at src/apm_cli/commands/config.py:315
  A user debugging the precedence chain cannot tell from this output which layer resolved the value.

DevX UX Expert

• [blocking] _CONFIG_KEY_DISPLAY_NAMES still maps "ssh": "ssh" instead of "prefer_ssh": "prefer-ssh" at src/apm_cli/commands/config.py:24
  Internal config key was renamed but display-name table was not updated. Any code path that looks up the display name via the internal key will silently miss the entry, leaving prefer-ssh unlabelled or invisible in rendered output.
  Suggested: Change "ssh": "ssh" to "prefer_ssh": "prefer-ssh".

• [recommended] Docs say unset only works for path keys; code supports it for both new boolean keys too at docs/src/content/docs/reference/cli/config.md:48
  config.md line 48 contradicts examples at lines 96-97 showing apm config unset prefer-ssh. Cannot both be correct.
  Suggested: Update the ### apm config unset KEY description to include allow-protocol-fallback and prefer-ssh as valid targets.

• [recommended] Stale ssh key reference in environment-variables.md resolution chain at docs/src/content/docs/reference/environment-variables.md:40
  Line 40 describes APM_GIT_PROTOCOL resolution using ssh key in config.json, but the actual key is prefer_ssh. Users hand-editing config.json will write the wrong key and get silent no-op behaviour.
  Suggested: Replace 'ssh key in ~/.apm/config.json' with 'prefer_ssh key' or reference the CLI name.

• [recommended] prefer-ssh boolean has no prefer-https counterpart; users who want to force HTTPS have no config escape hatch at docs/src/content/docs/reference/cli/config.md:57
APM_GIT_PROTOCOL accepts both ssh and https, but config only controls SSH preference. No way to persist 'always use HTTPS' in config.
  Suggested: Add a note: 'To force HTTPS in all contexts, use APM_GIT_PROTOCOL=https; there is currently no config-file equivalent for pinning HTTPS.'

• [nit] config get output prints Python-cased False/True instead of lowercase false/true at src/apm_cli/commands/config.py:181
  Suggested: Use str(val).lower() or explicit "true"/"false" strings.

Supply Chain Security Expert

• [recommended] ~/.apm/ directory created without explicit mode restriction at src/apm_cli/config.py:23
ensure_config_exists() calls os.makedirs(CONFIG_DIR) with no mode argument. On a multi-user system the directory inherits the process umask (commonly 755), making config.json world-readable. allow_protocol_fallback: true is a security-relevant state that other local users could observe or tamper with.
  Suggested: Use os.makedirs(CONFIG_DIR, mode=0o700, exist_ok=True) and os.chmod(CONFIG_FILE, 0o600) after initial creation.

• [recommended] FALLBACK_HINT in transport_selection.py is now incomplete after this PR at src/apm_cli/deps/transport_selection.py:31
  The hint reads 'pass --allow-protocol-fallback or set APM_ALLOW_PROTOCOL_FALLBACK=1'. After this PR a third source exists: apm config set allow-protocol-fallback true. Users will be confused about which mechanism to use.
  Suggested: Update FALLBACK_HINT to include the config path. Add a note that the setting persists across sessions.

• [recommended] No CI/shared-home warning when persisting allow_protocol_fallback: true at src/apm_cli/commands/config.py:254
  A user who runs apm config set allow-protocol-fallback true in a CI environment where $HOME is shared silently enables fallback for all subsequent installs. No warning at write time.
  Suggested: Emit logger.warning(...) when setting allow-protocol-fallback to True, noting that the setting persists and advising use of the env var in CI.

• [nit] _CONFIG_KEY_DISPLAY_NAMES contains stale "ssh": "ssh" and is never read -- dead code at src/apm_cli/commands/config.py:24
  Either delete the dict or correct the entry to "prefer_ssh": "prefer-ssh" and wire it up.

OSS Growth Hacker

• [recommended] authentication.md still teaches export APM_GIT_PROTOCOL=https without a forward-pointer to apm config set prefer-ssh false at docs/src/content/docs/getting-started/authentication.md
  Line 449 is the exact moment an SSH-blocked user is looking for the escape hatch. Showing only the env-var form leaves them one shell-profile edit away from forgetting about APM config -- a high-intent conversion miss.
  Suggested: After the export APM_GIT_PROTOCOL=https block, add: 'To persist this preference across sessions: apm config set prefer-ssh false'.

• [recommended] CHANGELOG entry buries the user-facing hook inside a resolution-order explanation at CHANGELOG.md
  The lead sentence front-loads the mechanism rather than the payoff. 'SSH users: you never have to type --ssh again' is what gets copy-pasted into a Slack or tweet.
  Suggested: Rewrite the opening to lead with payoff: 'SSH-first teams can now run apm config set prefer-ssh true once and drop --ssh from every subsequent apm install.'

• [nit] apm config unset prefer-ssh works but docs say only path keys are unsettable -- contradictory at docs/src/content/docs/reference/cli/config.md:48

• [nit] No apm init or first-install touchpoint guides SSH-first users to discover prefer-ssh at docs/src/content/docs/getting-started/installation.md
  Suggested: Add a post-install callout: 'SSH users: run apm config set prefer-ssh true once to avoid passing --ssh on every install.'

Auth Expert

• [recommended] Constructor docstring still says 'reads APM_GIT_PROTOCOL env' -- config layer is now consulted but not mentioned at src/apm_cli/deps/github_downloader.py:173
  Lines 173-176 describe the protocol_pref=None fallback as env-only. The implementation now resolves env -> config -> default. A caller reading only the docstring will not know that a persisted config value can override their expectations.
  Suggested: Update the docstring to mention ~/.apm/config.json as a resolution source.

• [nit] allow_fallback_env variable name in validation.py is now misleading at src/apm_cli/install/validation.py:317
  Reads from config (env > config > False) but _env suffix implies env-only. Will confuse future readers tracing why fallback is enabled despite no env var being set.
  Suggested: Rename to allow_fallback or allow_fallback_cfg.

Doc Writer

• [blocking] environment-variables.md names the config key as ssh, not prefer-ssh at docs/src/content/docs/reference/environment-variables.md:40
  Line 40 reads '-> ssh key in ~/.apm/config.json'. The actual CLI key is prefer-ssh. A user who runs apm config set ssh true will set an unrecognised key and get silent no-op behaviour. config.md correctly uses prefer-ssh throughout; env-vars.md contradicts it.
  Suggested: Change '-> ssh key in ~/.apm/config.json' to '-> prefer-ssh key in ~/.apm/config.json'.

• [blocking] apm config unset description contradicts the examples for prefer-ssh at docs/src/content/docs/reference/cli/config.md:48
  Line 48 states 'Only temp-dir and copilot-cowork-skills-dir are unsettable; boolean keys are reset by set-ing them to their default.' Lines 96-97 show apm config unset prefer-ssh as valid. These cannot both be correct.
  Suggested: Update line 48: 'Boolean keys (prefer-ssh, allow-protocol-fallback) can be removed with unset; the built-in default (false) then applies. temp-dir and copilot-cowork-skills-dir are also unsettable.'

• [recommended] CHANGELOG [Unreleased] contains two separate ### Added sections at CHANGELOG.md:21
  Keep a Changelog specifies one subsection per type per release. Having two ### Added headings will confuse changelog parsers and readers.
  Suggested: Merge the second ### Added block's bullet points into the first, then delete the duplicate heading.

• [nit] env-vars.md transport section intro has explanatory padding that could be tightened at docs/src/content/docs/reference/environment-variables.md:36
  Suggested: 'These settings can also be persisted via apm config set.'

Test Coverage Expert

• [recommended] No integration test exercises the config-file (not env-var) path of get_apm_allow_protocol_fallback through _validate_package_exists at tests/unit/install/test_validation_strict_transport.py
  The existing integration test covers the env-var branch (APM_ALLOW_PROTOCOL_FALLBACK=1). The persisted-config branch -- where env var is absent and config file has allow_protocol_fallback: true -- is only covered by unit-level mocks. Install-pipeline surface floor tier is integration-with-fixtures.
  Proof (missing): tests/unit/install/test_validation_strict_transport.py::test_config_persisted_allow_fallback_chains_both_schemes -- proves: When user has run apm config set allow-protocol-fallback true and no env var is set, _validate_package_exists still probes both transports [secure-by-default]

• [recommended] Downloader constructor's config-aware default for protocol_pref has no integration test at tests/unit/deps/test_github_downloader_validation.py
  The new branch where the constructor is instantiated without explicit protocol_pref and config file says prefer_ssh: true has no integration-with-fixtures test. Grepped all of tests/ for get_apm_protocol_pref and get_apm_allow_protocol_fallback: only hits in the two new unit files.
  Proof (missing): tests/unit/deps/test_github_downloader_validation.py::test_constructor_reads_ssh_preference_from_config -- proves: When get_prefer_ssh returns True and no explicit protocol_pref arg is given, downloader selects SSH [devx]

• [nit] test_protocol_config_precedence.py simulates CLI-flag path inline rather than calling real install dispatch at tests/unit/test_protocol_config_precedence.py
  Future refactors of install.py could silently break the wiring. Not blocking -- install invocation is covered elsewhere and the config-module unit tests are well-targeted.

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

Note

🔒 Integrity filter blocked 2 items

The following items were blocked because they don't meet the GitHub integrity level.

• feat: persist transport flags via apm config #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
• #1308 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review Panel for issue #1308 · ● 3.9M · ◷

[Aaryan-Dadu]

I've addressed all the blockers and nearly all the recommendations/nitpicks in the latest commits.