APM Review Panel Verdict

Disposition: APPROVE (with two minor pre-merge fixes)

Per-persona findings

Python Architect: This PR adds zero Python code -- all 18 files are shell scripts, JSON, Markdown, bats tests, and git submodule pointers. Applying the "purely procedural" clause from the PR review contract.

Structural placement: devcontainer/ at repo root, isolated from src/apm_cli/. Correct -- this is packaging infrastructure, not CLI logic. No APM Python source is modified.

Submodules: bats-core, bats-support, bats-assert -- all three pinned to specific commit SHAs. No floating-branch references.

Repetition: The package-manager detection cascade (apt-get / apk / dnf) is written three times in install.sh (curl block, python3 block, git block). At 154 lines this is tolerable; flagged as a pragmatic suggestion below.

OO/class diagram (module boundaries -- no Python classes in scope):

classDiagram
    direction LR
    class DevcontainerFeature {
        <<IOBoundary>>
        +id string
        +version string
        +installsAfter list
    }
    class InstallScript {
        <<Pure>>
        +validate_version()
        +check_root()
        +install_uv()
        +ensure_python3()
        +ensure_git()
        +locate_pip()
        +install_apm_cli()
    }
    class PackageManagerDetector {
        <<Pure>>
        +apt_get bool
        +apk bool
        +dnf bool
    }
    class BatsTestSuite {
        <<IOBoundary>>
        +37 unit tests
        +run_with_stubs()
        +make_stub()
    }
    class BatsDeps {
        <<ExternalDep>>
        +bats_core SHA pinned
        +bats_support SHA pinned
        +bats_assert SHA pinned
    }
    DevcontainerFeature *-- InstallScript : executes
    InstallScript *-- PackageManagerDetector : uses inline
    BatsTestSuite ..> InstallScript : stubs and exercises
    BatsTestSuite *-- BatsDeps : loads helpers
    classDef touched fill:#fff3b0,stroke:#d47600
    class DevcontainerFeature:::touched
    class InstallScript:::touched
    class BatsTestSuite:::touched
    class PackageManagerDetector:::touched
    class BatsDeps:::touched

Execution flow diagram:

flowchart TD
    A["devcontainer build\ndevcontainer.json declares feature"] --> B["[EXEC] inject VERSION env var\ninstall.sh:4"]
    B --> C{"validate VERSION\nlatest or X.Y.Z\ninstall.sh:6-11"}
    C -->|invalid| D["[x] exit 1 -- VERSION error"]
    C -->|valid| E{"id -u == 0\ninstall.sh:13-16"}
    E -->|not root| F["[x] exit 1 -- must run as root"]
    E -->|root| G{"uv on PATH\ninstall.sh:399"}
    G -->|present| H["skip -- uv already installed"]
    G -->|absent| I{"curl on PATH\ninstall.sh:403"}
    I -->|absent| J["[EXEC] install curl\napt-get/apk/dnf\ninstall.sh:405-415"]
    I -->|present| K["[NET] curl astral.sh/uv/install.sh to tmp\ninstall.sh:420"]
    J --> K
    K --> L["[EXEC] sh _uv_tmp UV_INSTALL_DIR=/usr/local/bin\ninstall.sh:421"]
    H --> M{"python3 on PATH\ninstall.sh:427"}
    L --> M
    M -->|absent| N["[EXEC] install python3+pip+git\napt-get/apk/dnf\ninstall.sh:429-441"]
    M -->|present| O{"python3 >= 3.10\ninstall.sh:460-466"}
    N --> O
    O -->|fail| P["[x] exit 1 -- Python version"]
    O -->|ok| Q{"git on PATH\ninstall.sh:444"}
    Q -->|absent| R["[EXEC] install git\napt-get/apk/dnf\ninstall.sh:445-456"]
    Q -->|present| S["locate pip3/pip/ensurepip\ninstall.sh:470-485"]
    R --> S
    S --> T["[NET] pip install apm-cli PKG_SPEC\ninstall.sh:497-508"]
    T --> U{"externally-managed-environment\ninstall.sh:500"}
    U -->|detected| V["[NET] pip install --break-system-packages\ninstall.sh:502"]
    U -->|other error| W["[x] exit 1"]
    V --> X{"apk present, no bash\ninstall.sh:511"}
    U -->|success| X
    X -->|yes| Y["[EXEC] apk add bash\ninstall.sh:512"]
    X -->|no| Z{"apm on PATH\ninstall.sh:516"}
    Y --> Z
    Z -->|yes| AA["[+] APM version installed\ninstall.sh:517"]
    Z -->|no| AB["[!] warn not on PATH\ninstall.sh:519-521"]

Design patterns

• Used in this PR: Guard clause (early exit) -- root check (install.sh:13-16), VERSION validation (install.sh:6-11), Python version boundary (install.sh:460-466); Idempotent command -- uv-skip-if-present (install.sh:399) prevents re-installing on repeated container builds.
• Pragmatic suggestion: Extract package-manager detection to a detect_pkg_mgr() shell function -- the apt-get/apk/dnf cascade appears three times (curl, python3, git install blocks). At current size it is acceptable; worth extracting if the script grows beyond ~200 lines.

CLI Logging Expert: No APM Python output infrastructure (CommandLogger, _rich_*, DiagnosticCollector) is touched. All user-facing output is from install.sh.

Output quality against APM STATUS_SYMBOLS conventions:

• [x] for all error exits: correct
• [+] for success: correct ([+] APM 0.9.0 installed at /usr/local/bin/apm)
• [!] for warn-not-fail PATH case: correct
• Error messages include next actions ("Add 'USER root' to your Dockerfile" -- exemplary)
• apt-get -qq and apk --no-cache keep install output quiet -- signal-to-noise OK

One minor note: progress lines (echo "Installing uv...", echo "Installing APM CLI (version: ${VERSION})...") do not use STATUS_SYMBOLS. Not a blocker; shell feature scripts conventionally omit them. No _rich_* or CommandLogger anti-patterns introduced.

DevX UX Expert: Feature follows the devcontainer spec exactly -- developers familiar with ghcr.io/devcontainers/features/python will immediately understand the usage pattern. version: "latest" vs semver matches npm/pip mental models precisely.

Error messages are strong: every failure includes a concrete recovery action. The warn-not-fail PATH case is correct UX (install succeeded; the PATH issue is advisory). The sync-local-devcontainer.sh workaround for the VS Code path restriction is well-documented.

Critical gap: devcontainer/README.md Option B presents the OCI reference as a usable install path:

{
  "features": {
    "ghcr.io/<org>/<collection>/apm:1": {}
  }
}

<org>/<collection> is a placeholder. No CI publish workflow is included in this PR. A developer reading the README and copying this snippet will get a resolution failure with no useful error. This must be addressed before merge -- either mark Option B as "coming soon" or add the publish workflow.

Unit test submodule init step is documented in README section 5 -- acceptable discoverability.

Supply Chain Security Expert: No critical gaps. Full threat-surface analysis:

1. curl-then-exec uv installer (curl -LsSf (astral.sh/redacted) > "$_uv_tmp" && sh "$_uv_tmp"): Two-step save-then-exec is safer than a pipe but the downloaded installer carries no hash or signature verification. Executes as root. TLS to astral.sh is the sole integrity anchor. This is the official Astral-recommended install pattern and an accepted ecosystem norm at container build time. Not a blocker. Gap worth tracking: the script fetches the latest installer rather than a pinned version, making builds non-reproducible across uv installer releases.

2. PyPI install (pip install apm-cli[==X.Y.Z]): Standard PyPI delivery; no --hash pinning. Consistent with APM's existing distribution surface. Pinned-version path (==X.Y.Z) reduces malicious-update window. No regression.

3. Git submodules (bats-core d9faff0d, bats-assert 697471b7, bats-support 0954abb9): All SHA-pinned. Test-only; not part of the installed feature. Good practice.

4. PEP 668 --break-system-packages: Intentional; scoped to the container (ephemeral); documented in README. Acceptable.

5. Root execution: Required by devcontainer spec; script verifies id -u == 0 before proceeding. Correct.

6. Temp file cleanup: trap 'rm -f "$_uv_tmp"' EXIT INT TERM covers all exit paths. Good.

Auth Expert: Not activated -- PR touches only devcontainer/ packaging files (shell scripts, JSON manifest, bats tests); no changes to auth.py, token_manager.py, azure_cli.py, or any host-classification or credential-resolution code.

OSS Growth Hacker: High-leverage adoption move. Dev Container Features are indexed on containers.dev/features -- a passive discovery channel for developers setting up AI development environments. Being listed there gives APM organic reach from developers who have never heard of it. The one-line devcontainer.json addition is the steepest friction reduction APM has shipped for Codespaces / VS Code Dev Containers users.

Two blocking gaps for growth:

1. No CHANGELOG entry -- required by repo convention; also the raw material for the launch beat.
2. OCI reference is a placeholder (see DevX finding) -- makes the README misleading and undercuts trust on first read.

One near-term gap: the main README.md does not mention devcontainer as an install path. The Codespaces funnel is completely invisible to visitors landing on the repo home page.

Side-channel to CEO: Merge this quickly -- it signals responsiveness to an external contributor and unlocks a distribution channel. The publish workflow (not in this PR) is the single biggest growth multiplier; it should ship as the immediate follow-up. Story angle when shipping: "Install APM in any Codespace or dev container with one line" -- retweetable, reinforces the reproducible-AI-dev-environments narrative, and is technically accurate once the publish workflow lands.

CEO arbitration

Specialists are in agreement; no conflicts to resolve. The feature is well-implemented, well-tested (37 stub-isolated bats unit tests plus a 6-distro integration matrix), and architecturally clean. Two pre-merge fixes are required: a CHANGELOG.md entry under [Unreleased] (every code-changing PR per repo convention) and a correction to the README Option B OCI reference so it does not present an unresolvable placeholder as a working install path. The security posture is acceptable -- the uv curl-then-exec is an ecosystem norm; a follow-up issue to pin the uv installer version is the right scope for that gap. This is a strategically important PR from an external contributor: merging promptly (with the two small fixes) is the right call.

Required actions before merge

1. Add a CHANGELOG.md entry under ## [Unreleased] in the Added section, e.g.: Added: Dev Container Feature -- packages \apm-cli` as a reusable devcontainer feature with multi-distro support and 37 bats unit tests (Devcontainer feature #861).`
2. Update devcontainer/README.md Option B (lines ~127-148) to make clear the OCI reference (ghcr.io/<org>/<collection>/apm:1) is a placeholder pending a publish workflow. Mark it explicitly as "coming soon" or replace the snippet with instructions for local-path consumption only. As written, it is an unresolvable reference that will confuse users.

Optional follow-ups

• Add a CI workflow using devcontainers/action@v1 (publish mode) to push the feature to GHCR on tag push -- this is what enables real discoverability on containers.dev and is the Why do we need a GitHub token? #1 growth multiplier from this PR.
• Update the main README.md installation section to surface the devcontainer feature as an install option (targets the Codespaces / VS Code funnel that the repo home page currently misses entirely).
• Open a follow-up issue to pin the uv installer to a specific version rather than always fetching the latest installer script (reproducibility + supply-chain posture).
• Consider adding a shellcheck CI step for install.sh and sync-local-devcontainer.sh to catch POSIX compliance regressions (the bats test already covers the local keyword, but shellcheck would catch more).

Generated by PR Review Panel for issue #861 · ● 880.3K · ◷

APM Review Panel Verdict

Disposition: REQUEST_CHANGES (two lightweight pre-merge items -- no code changes required)

Per-persona findings

Python Architect: This PR contains zero Python code changes. All 18 changed files are POSIX sh, JSON, Bash (bats tests), and Markdown. The architecture is a textbook Dev Container Feature shape: a declarative manifest + a procedural install script + a comprehensive test suite. No OO structural concerns. Two minor shell observations: python3 -c "..." is called twice to get major/minor (could be one invocation -- harmless in container context), and the trap covers EXIT INT TERM but not HUP (very minor POSIX gap). The bats stub-isolation approach is excellent -- 37 tests cover all branch paths without Docker or network. This is the right architecture for this scope.

OO / class diagram (module boundaries):

classDiagram
    direction LR
    class DevContainerFeature {
        <<ExternalProtocol>>
        +id: str = "apm"
        +version: str = "1.0.0"
        +options: dict
        +installsAfter: list
    }
    class InstallScript {
        <<IOBoundary>>
        +apt_update_once() void
        +validate_version() void
        +install_uv() void
        +ensure_python3() void
        +ensure_git() void
        +locate_pip() void
        +install_apm() void
        +ensure_bash_alpine() void
        +verify_apm_path() void
    }
    class PackageManagers {
        <<Adapter>>
        +apt_get: cmd
        +apk: cmd
        +dnf: cmd
    }
    class ExternalSources {
        <<ExternalDependency>>
        +astral_sh_uv_install: url
        +pypi_apm_cli: pkg
    }
    class BatsTestSuite {
        <<TestFixture>>
        +setup() STUB_BIN
        +run_with_stubs() void
        +make_stub() void
        +make_pkg_mgr_stub() void
    }
    DevContainerFeature ..> InstallScript : executes via devcontainer CLI
    InstallScript ..> PackageManagers : dispatches per platform
    InstallScript ..> ExternalSources : fetches
    BatsTestSuite ..> InstallScript : stubs and exercises
    class InstallScript:::touched
    class DevContainerFeature:::touched
    class BatsTestSuite:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

Execution flow diagram:

flowchart TD
    A["devcontainer CLI build"] --> B["[EXEC] install.sh\nVERSION from feature env"]
    B --> C{VERSION = 'latest'\nor semver X.Y.Z?}
    C -- "no" --> D["[x] exit 1\n'VERSION must be latest or semver'"]
    C -- "yes" --> E{id -u == 0?}
    E -- "no" --> F["[x] exit 1\n'must run as root'"]
    E -- "yes" --> G{uv on PATH?}
    G -- "yes" --> J["skip uv install"]
    G -- "no" --> H{curl on PATH?}
    H -- "no" --> H2["[NET] install curl\napt-get/apk/dnf"]
    H -- "yes" --> I
    H2 --> I["[NET] curl astral.sh/uv/install.sh to mktemp\ntrap rm tmpfile EXIT/INT/TERM\n[EXEC] sh tmpfile UV_INSTALL_DIR=/usr/local/bin"]
    I --> J
    J --> K{python3 on PATH?}
    K -- "no" --> L["[FS] install python3+pip+git\napt-get/apk/dnf"]
    K -- "yes" --> M
    L --> M{git on PATH?}
    M -- "no" --> N["[FS] install git\napt-get/apk/dnf"]
    M -- "yes" --> O["assert python3 >= 3.10\nsys.version_info.major/minor"]
    N --> O
    O -- "< 3.10" --> P["[x] exit 1\n'apm-cli requires Python 3.10+'"]
    O -- ">= 3.10" --> Q["locate pip:\npip3 > pip > python3 -m ensurepip"]
    Q -- "none found" --> QF["[x] exit 1\n'pip not available'"]
    Q -- "found" --> R["install_apm()\n$PIP_CMD install apm-cli\[==VERSION\] 2>&1"]
    R -- "success" --> S
    R -- "externally-managed-environment" --> T["[NET] retry:\n--break-system-packages"]
    T -- "success" --> S
    T -- "fail" --> U["[x] exit 1"]
    R -- "other error" --> U
    S["[FS] apk: add bash if missing"] --> V{apm on PATH?}
    V -- "yes" --> W["[+] APM version installed at PATH"]
    V -- "no" --> X["[!] warn: not in PATH\nno exit 1"]

Design patterns

• Used in this PR: Adapter -- apt_update_once() and the if/elif/else chains wrap platform-specific package managers (apt-get, apk, dnf) behind a uniform call interface; install_apm() adapts the PEP 668 retry protocol. Correct for shell scope.
• Used in this PR: Template Method (inline) -- install_apm() defines the primary attempt -> error detection -> retry sequence. Appropriate without subclasses in a single-context sh script.
• Pragmatic suggestion: none -- 154 lines of POSIX sh is the simplest correct design at this scope.

CLI Logging Expert: No changes to Python CLI logging infrastructure (CommandLogger, DiagnosticCollector, _rich_* helpers). The install.sh output correctly uses the repo's ASCII bracket notation: [x] for errors (each with a concrete next action), [+] for success, [!] for the non-fatal PATH warning. One minor inconsistency: progress echo lines like echo "Installing uv..." and echo "Python 3 not found -- installing via system package manager..." have no leading status symbol; prefixing with [*] (action) or [i] (info) would be more uniform with APM conventions. Not blocking in a devcontainer install.sh. No CommandLogger violations.

DevX UX Expert: The devcontainer feature mental model (one JSON declaration, installsAfter ordering, single version option) is immediately familiar to anyone who has used the python or node devcontainer features. Error messages are good: every failure includes a concrete next action ("Add USER root to your Dockerfile", "Use a base image with Python 3.10+"). The sync-local-devcontainer.sh helper is an excellent ergonomic touch for contributors.

Critical UX gap: Section 4 "Option B" of the README uses ghcr.io/<org>/<collection>/apm:1 as the consumption reference. This feature is not yet published to GHCR. A user who reads the README today and tries Option B will get an OCI pull failure with no indication of why. This must be addressed before merge -- either with an explicit "not yet published" note and a link to the tracking issue, or by adding the publish workflow to this PR. The code and tests are solid; this is a documentation gap that determines first-impression UX.

Supply Chain Security Expert: Two new external download surfaces are introduced in the container build path:

1. uv installer: curl astral.sh/uv/install.sh -> mktemp -> sh (medium risk, accepted): HTTPS protects in-transit integrity; downloading to a temp file first is better than curl | sh direct. However, there is no checksum verification of the installer script content and no pinned uv version. If astral.sh were compromised, the installer would execute arbitrary code as root inside the container. This risk is broadly accepted in the ecosystem (rustup, bun, nix use the same pattern). Recommended: add a comment to install.sh explicitly acknowledging this trade-off (pattern: "# The uv installer is fetched over HTTPS without checksum verification -- accepted risk per (astral.sh/redacted) ..."). Not blocking.

2. pip install apm-cli without hash pinning (low risk): Standard for devcontainer features; no --require-hashes. Not blocking.

3. Git submodules for bats (low risk): Git submodule SHAs are stored in the object tree, so git submodule update --init --recursive always fetches the pinned SHA. Correctly pinned.

4. PEP 668 --break-system-packages retry (accepted): Appropriate for container environments; clearly documented.

No path traversal surface (install.sh does not interact with APM's path_security.py). No auth token handling. Overall supply chain posture is appropriate for the devcontainer feature ecosystem.

Auth Expert: Not activated -- all 18 changed files are in devcontainer/ (shell install script, feature manifest, README, test scripts); no APM auth source files (auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, install/validation.py, registry_proxy.py) were modified.

OSS Growth Hacker: High-conviction growth beat. "APM works out-of-the-box in any Dev Container" is a compelling one-liner that opens a new acquisition channel: VS Code Remote Containers users. The friction reduction (from a custom postCreateCommand to a one-line JSON declaration) is the kind of delta that drives organic word-of-mouth. Two notes to CEO: (1) A CHANGELOG.md entry is missing -- this is the raw material for the release narrative; without it the feature ships quietly. (2) The OCI reference placeholder in Option B means the story can't be told externally until the publish workflow lands. Track "devcontainer feature published to GHCR" as the prerequisite milestone for the launch beat (blog post / social: "zero-config APM for every dev environment in one line"). This is a good PR -- unblock it fast so the publish step can follow.

CEO arbitration

Specialists are in agreement; no disagreements to arbitrate. This is a well-scoped, well-tested greenfield addition that sits squarely within APM's mission. Two pre-merge items are required: the missing CHANGELOG entry (repo convention, non-negotiable) and the README Option B clarification (first-impression UX, cannot let users hit a silent OCI pull failure). The supply chain risk from the uv curl-install pattern is accepted ecosystem practice and does not block. The publish-to-GHCR workflow is the natural follow-up issue -- it does not need to land in this PR, but it must be tracked explicitly. Once the two required actions are addressed, this is approved.

Required actions before merge

1. Add CHANGELOG.md entry under [Unreleased] > Added: per repo convention, every merged PR that ships code requires a changelog entry. Suggested text: - Added Dev Container Feature (devcontainer/src/apm/) -- installs APM CLI in any devcontainer.json with a one-line declaration; includes 37 bats unit tests and a 6-scenario integration matrix (#861).

2. Clarify the unpublished OCI reference in devcontainer/README.md section 4 Option B: the placeholder ghcr.io/<org>/<collection>/apm:1 is not a real published reference. A user who tries it today will get an OCI pull failure. Add a clear "Not yet published -- coming soon" note (or link to the tracking issue for the publish workflow) so the first-impression UX does not silently fail.

Optional follow-ups

• Add a GitHub Actions workflow to publish the devcontainer feature to ghcr.io/microsoft/apm/apm:1 (the publish pipeline is the prerequisite for Option B to be usable and for the growth beat to land).
• Add a comment in install.sh acknowledging the accepted supply chain trade-off for the uv curl-to-sh install pattern (e.g. citing the risk and why it is accepted for container contexts).
• Minor: consolidate the two python3 -c "..." invocations that read version_info.major and version_info.minor into one invocation for clarity.
• Minor: add HUP to the trap signal list in install.sh for full POSIX coverage.
• Consider adding [*] or [i] status symbol prefixes to the progress echo lines in install.sh for consistency with APM's ASCII status symbol conventions.

Generated by PR Review Panel for issue #861 · ● 1.1M · ◷

@coakenfold let me think about the publishing pipeline and how to get the artifact published. If you have any suggestions or proposed steps, please post here.

@danielmeppiel sounds good. It's not clear to me if I need to do anything else. Please let me know if something needs to change. Thx for the review & consideration.

Hey @coakenfold -- with your maintainer_can_modify permission I pushed one follow-up commit (62cf31d3) to fold in the maintainer-side concerns so this can land cleanly and users get a working ghcr.io/microsoft/apm/apm-cli:1 reference on day one. Happy to revert any of it if you'd rather take a different angle.

What changed in that commit:

1. Renamed feature id from apm -> apm-cli. The apm name collides with the well-known APM = Application Performance Monitoring meaning (Datadog/Elastic/NewRelic/OpenTel devcontainer features all ship under apm). apm-cli is unambiguous and matches the PyPI package name. Mechanical rename: src/apm/ -> src/apm-cli/, test/apm/ -> test/apm-cli/, manifest id, scenarios.json keys, bats INSTALL_SH path, sync-local helper. Final OCI ref: ghcr.io/microsoft/apm/apm-cli.

2. CHANGELOG.md entry under [Unreleased] / Added.

3. README.md -- replaced the 3 placeholder ghcr.io/<org>/<collection>/apm:1 refs with the real ghcr.io/microsoft/apm/apm-cli:1, added a tag-selection table (:1 vs :1.0 vs :1.0.0 vs :latest), and reordered section 4 so the GHCR quick-start comes first and local-development is last (matches the convention from every devcontainers/features/* README).

4. New .github/workflows/devcontainer-feature-test.yml -- pull_request + merge_group gate runs the bats unit tests + a 4-scenario integration matrix (default-ubuntu-24, default-alpine-3, pinned-version, with-python-feature). A nightly cron extends to 6 scenarios with default-debian-12 + default-fedora so we catch base-image drift without slowing down PR throughput. Single aggregation job Devcontainer Feature - Test Result so branch protection doesn't break when matrix legs are added/removed.

5. New .github/workflows/devcontainer-feature-publish.yml -- push: main path-filtered on devcontainer/src/**, uses devcontainers/action@v1 with disable-repo-tagging: true (avoids the action pushing its own git tags alongside our v* apm-cli release tags), permissions: { contents: read, packages: write }, concurrency.cancel-in-progress: false (no partial manifest pushes if two publishes race), and workflow_dispatch for manual recovery.

Validated locally: 37/37 bats unit tests pass, both new workflows pass actionlint. Two reviewer checkpoints (DevX UX + OSS Growth) both returned SHIP.

Post-merge ceremony I'll handle on the maintainer side: flip the GHCR package visibility to Public + connect to the repo (without this, users get silent unauthorized pulls), add Devcontainer Feature - Test Result to branch protection, and file the containers.dev community-index PR.

Thanks again for the patient back-and-forth on this -- it's a really clean feature.