Skip to content

feat(marketplace): filter-driven CLI, map-based manifest, JSON output#1324

Merged
danielmeppiel merged 9 commits into
mainfrom
feat/marketplace-output-ux-1317
May 14, 2026
Merged

feat(marketplace): filter-driven CLI, map-based manifest, JSON output#1324
danielmeppiel merged 9 commits into
mainfrom
feat/marketplace-output-ux-1317

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

Summary

Implements the v0.14 marketplace output UX redesign from #1317, building on merged PR #1281.

What's new

Feature Flag / Config Description
Format filter -m/--marketplace FORMATS Comma-separated, repeatable, all/none sentinels
Path override --marketplace-path FORMAT=PATH Per-invocation path override; env vars APM_MARKETPLACE_<FMT>_PATH for CI
JSON output --json Stable JSON to stdout, logs to stderr — pipe to jq
Map-form manifest outputs: {claude: {}, codex: {path: ...}} Replaces deprecated list form with one-cycle warning
Deprecation --marketplace-output Hidden, warns, auto-translates to --marketplace-path claude=PATH

Design

  • Expert-panel reviewed: python-architect (lead), devx-ux, cli-logging, supply-chain-security
  • Test-first: 51 new tests across 4 test files (1019 marketplace+pack tests total)
  • Stream discipline: --json routes all logs to stderr; stdout is pure JSON
  • Backwards-compatible: list-form outputs:, sibling blocks, and --marketplace-output all keep working

Commits (5)

  1. phase-3a: Map-form outputs parsing + profile validation (20 tests)
  2. phase-3b: BuildReport JSON serialization (8 tests)
  3. phase-3c: CLI flags + deprecation + JSON error paths (7 tests)
  4. phase-3d: Init template updated to map form
  5. docs: pack.md, publish guide, CHANGELOG

Known deferred items (v0.15 / #1318)

  • Full console.py sink-rewire (basic stderr routing done)
  • marketplace_formats filter not yet wired to BuildOrchestrator (computed but not threaded)
  • Env-var path override resolution at runtime
  • Removal of --marketplace-output flag entirely

Closes #1317
Refs #1318

Daniel Meppiel and others added 5 commits May 14, 2026 16:30
…1317 phase-3a)

- Add path_env_var field to MarketplaceOutputProfile
- Add _validate_profile() guard: reserved names, bad chars, env-var shape
- Add MarketplaceOutputSpec dataclass for resolved per-format specs
- Parse outputs: as map (new) or list/string (deprecated with warning)
- Detect sibling-vs-map path conflicts (sibling wins, with warning)
- Wire output_specs + warnings fields on MarketplaceConfig
- Add 36 new tests covering profiles + map-form parsing

Refs: design.final.md §1 (data model), §5.4 (sibling conflict)
Test IDs: T-3a-01..26

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add to_json_dict() on BuildReport: §4 JSON contract shape
  {ok, dry_run, warnings[], errors[], marketplace:{outputs:[]}, bundle:null}
- Add failure_to_json_dict() classmethod for pre-build failures
- 8 new tests covering success/failure/multi-output/dry-run

Refs: design.final.md §4 (JSON contract)
Test IDs: T-3b-01..08

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…flags (#1317 phase-3c)

- Add -m/--marketplace FORMATS filter flag (comma-separated, 'all'/'none')
- Add --marketplace-path FORMAT=PATH repeatable override
- Add --json flag: emits §4 JSON to stdout, logs to stderr
- Deprecate --marketplace-output (hidden, warns, auto-translates)
- All error paths emit JSON under --json mode (no broken pipes)
- Update existing test for hidden deprecated flag
- 7 new CLI flag tests

Refs: design.final.md §3 (CLI surface), §4 (JSON contract), §5 (failures)
Test IDs: T-3c-01..12

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ase-3d)

- Replace list-form 'outputs: [claude, codex]' scaffold comment with
  explicit map-form 'outputs: {claude: {}}' per design.final.md §6
- Add CI tip comment showing '--marketplace=... --json | jq' usage
- Keep codex section as commented-out example with path override

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- pack.md: document --marketplace, --marketplace-path, --json flags;
  mark --marketplace-output as deprecated/hidden; update YAML examples
  to map form; add JSON mode behavior bullet
- publish-to-a-marketplace.md: update outputs examples to map form;
  mention --json for CI
- CHANGELOG: add 5 'Added' + 1 'Changed' entries under [Unreleased]

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 14, 2026 14:40
@danielmeppiel danielmeppiel added the panel-review Trigger the apm-review-panel gh-aw workflow label May 14, 2026
Daniel Meppiel and others added 2 commits May 14, 2026 16:42
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The apm pack output directory was accidentally committed.
Add glob pattern to .gitignore to prevent recurrence.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the planned v0.14 marketplace UX surface for apm pack by adding (1) map-form marketplace.outputs parsing, (2) new CLI flags for marketplace selection/path override, and (3) a JSON output mode intended for CI-friendly piping.

Changes:

  • Add map-form marketplace.outputs parsing (with deprecation warnings for list/string forms) and output-profile metadata (incl. per-profile env-var names).
  • Add BuildReport JSON serialization helpers and expose --json / --marketplace / --marketplace-path on apm pack (plus deprecate/hide --marketplace-output).
  • Update init scaffolding, docs, and CHANGELOG to describe the new marketplace surface.
Show a summary per file
File Description
tests/unit/marketplace/test_outputs_map_form.py New unit tests for map-form outputs: parsing, validation, and deprecation warnings.
tests/unit/marketplace/test_output_profiles.py New unit tests for output-profile registration invariants and env-var naming.
tests/unit/marketplace/test_builder_json.py New unit tests for BuildReport JSON serialization shape.
tests/unit/commands/test_pack.py Updates help expectations for hidden legacy flag and new --marketplace-path.
tests/unit/commands/test_pack_cli_flags.py New tests for CLI validation/error behavior for the new flags.
src/apm_cli/marketplace/yml_schema.py Introduces MarketplaceOutputSpec, parses map-form outputs, and accumulates warnings.
src/apm_cli/marketplace/output_profiles.py Adds path_env_var to profiles plus registration-time validation.
src/apm_cli/marketplace/init_template.py Updates scaffolded marketplace block to the map-form outputs: layout and JSON/CI example.
src/apm_cli/marketplace/builder.py Adds BuildReport.to_json_dict() and failure_to_json_dict() for JSON output contract.
src/apm_cli/commands/pack.py Adds new CLI options, deprecates legacy flag, and attempts JSON stdout/stderr stream discipline.
docs/src/content/docs/reference/cli/pack.md Documents new flags, JSON mode, and map-form outputs.
docs/src/content/docs/producer/publish-to-a-marketplace.md Updates producer guide to map-form outputs and JSON CI usage.
CHANGELOG.md Adds Unreleased entries describing the new marketplace UX surface and deprecations.

Copilot's findings

Comments suppressed due to low confidence (2)

src/apm_cli/commands/pack.py:267

  • --marketplace-path and --marketplace are parsed/validated into path_overrides/marketplace_formats, but neither value is ever used when constructing BuildOptions or calling BuildOrchestrator. As a result these flags are currently no-ops (beyond validation), despite being documented and listed in the changelog. Plumb these through core BuildOptions into MarketplaceProducer (and ultimately MarketplaceBuilder) or remove/disable the flags until the wiring exists.
        if "=" not in override:
            msg = f"--marketplace-path must be FORMAT=PATH, got: {override!r}"
            if json_output:
                from ..marketplace.builder import BuildReport

                click.echo(
                    json_mod.dumps(
                        BuildReport.failure_to_json_dict(
                            errors=[{"code": "cli_error", "message": msg}]
                        )
                    ),
                    file=sys.stdout,
                )
                ctx.exit(1)
                return
            raise click.ClickException(msg)
        fmt_name, path_val = override.split("=", 1)
        fmt_name = fmt_name.strip()
        path_val = path_val.strip()
        if fmt_name not in known_output_names():
            msg = (
                f"Unknown marketplace format '{fmt_name}' in --marketplace-path. "
                f"Known formats: {', '.join(sorted(known_output_names()))}"
            )
            if json_output:
                from ..marketplace.builder import BuildReport

                click.echo(
                    json_mod.dumps(
                        BuildReport.failure_to_json_dict(
                            errors=[{"code": "unknown_format", "message": msg}]
                        )
                    ),
                    file=sys.stdout,
                )
                ctx.exit(1)
                return
            raise click.ClickException(msg)
        path_overrides[fmt_name] = path_val

    # -- Parse --marketplace filter --
    marketplace_formats: tuple[str, ...] | None = None
    if marketplace_filter is not None:
        if marketplace_filter.strip().lower() == "none":
            marketplace_formats = ()
        elif marketplace_filter.strip().lower() == "all":
            marketplace_formats = None  # all configured
        else:
            requested = [f.strip() for f in marketplace_filter.split(",") if f.strip()]
            known = known_output_names()
            for r in requested:
                if r not in known:
                    msg = (
                        f"Unknown marketplace format '{r}' in --marketplace. "
                        f"Known formats: {', '.join(sorted(known))}"
                    )
                    if json_output:
                        from ..marketplace.builder import BuildReport

                        click.echo(
                            json_mod.dumps(
                                BuildReport.failure_to_json_dict(
                                    errors=[{"code": "unknown_format", "message": msg}]
                                )
                            ),
                            file=sys.stdout,
                        )
                        ctx.exit(1)
                        return
                    raise click.ClickException(msg)
            marketplace_formats = tuple(requested)  # noqa: F841 — wired in orchestrator integration
    project_root = Path(".").resolve()

src/apm_cli/marketplace/yml_schema.py:1013

  • _parse_outputs() now returns output_specs with per-format 'path', but _build_config() never applies those paths to the per-profile configs (claude.output/codex.output). The BuildOrchestrator MarketplaceProducer currently reads output paths from config..output, so 'outputs: {codex: {path: ...}}' will be ignored at build time. Either populate the profile output fields from output_specs (when sibling blocks are absent) or update the producer to use output_specs, and ensure config.warnings is surfaced to the CLI so deprecation/conflict warnings are visible.

    claude = _parse_claude(marketplace_dict.get("claude"), default_output=output)
    output = claude.output

    # -- metadata (Anthropic pass-through, preserve verbatim) --
    metadata: dict[str, Any] = {}
    raw_metadata = marketplace_dict.get("metadata")
    if raw_metadata is not None:
        if not isinstance(raw_metadata, dict):
            raise MarketplaceYmlError("'metadata' must be a mapping")
        metadata = dict(raw_metadata)

    # S1: validate pluginRoot with path-safety checks if present.
    plugin_root = metadata.get("pluginRoot")
    if plugin_root is not None and isinstance(plugin_root, str) and plugin_root.strip():
        try:
            validate_path_segments(
                plugin_root.strip(),
                context="metadata.pluginRoot",
                allow_current_dir=True,
            )
        except PathTraversalError as exc:
  • Files reviewed: 13/14 changed files
  • Comments generated: 14

Comment thread src/apm_cli/commands/pack.py Outdated
Comment on lines +176 to +182
# -- Stream discipline: under --json, route logs to stderr --
if json_output:
_logging.basicConfig(stream=sys.stderr, force=True)

# -- Deprecation: --marketplace-output → --marketplace-path claude=PATH --
if marketplace_output is not None:
logger.warning(
Comment on lines +113 to +124
"marketplace_filter",
type=str,
default=None,
help=(
"Comma-separated marketplace outputs to build (e.g. 'claude,codex'). "
"Use 'all' for every configured output, 'none' to skip marketplace. "
"Default: build all configured outputs."
),
)
@click.option(
"--marketplace-path",
"marketplace_path_overrides",
Comment thread src/apm_cli/commands/pack.py Outdated
# -- Deprecation: --marketplace-output → --marketplace-path claude=PATH --
if marketplace_output is not None:
logger.warning(
"--marketplace-output is deprecated and will be removed in v0.15. "
return
raise click.ClickException(msg)
marketplace_formats = tuple(requested) # noqa: F841 — wired in orchestrator integration
project_root = Path(".").resolve()
Comment on lines +184 to +190
def to_json_dict(self) -> dict[str, Any]:
"""Serialize build report as the §4 JSON contract.

Shape: {ok, dry_run, warnings[], errors[],
marketplace: {outputs: [{format, path, added, updated,
unchanged, skipped}]}, bundle: null}
"""
Comment on lines +38 to +40
| `-m`, `--marketplace FORMATS` | all configured | Comma-separated list of marketplace formats to build. Repeatable. Sentinels: `all` (every configured format), `none` (skip marketplace entirely). |
| `--marketplace-path FORMAT=PATH` | manifest default | Override the output path for a specific format. Repeatable. Example: `--marketplace-path codex=./dist/codex.json`. |
| `--json` | off | Emit machine-readable JSON to stdout. All logs move to stderr. Shape: `{ok, dry_run, warnings, errors, marketplace: {outputs: [...]}}`. |
Comment on lines +118 to +119
codex:
path: .agents/plugins/marketplace.json
Comment thread CHANGELOG.md Outdated
Comment on lines +16 to +24
- `apm pack --marketplace=FORMATS` filters which marketplace formats are built in a single run; accepts comma-separated names, repeatable flag, and sentinels `all`/`none`. (#1317)
- `apm pack --marketplace-path FORMAT=PATH` overrides the output path for a specific marketplace format at invocation time; env vars `APM_MARKETPLACE_<FORMAT>_PATH` provide the same override in CI without CLI flags. (#1317)
- `apm pack --json` emits a stable JSON contract to stdout (`{ok, dry_run, warnings, errors, marketplace: {outputs: [{format, path, ...}]}}`); all logs move to stderr so downstream tooling can `jq` the output safely. (#1317)
- `marketplace.outputs` in `apm.yml` now accepts a map form keyed by format name (`outputs: {claude: {}, codex: {path: ...}}`), replacing the deprecated list form; the list form still parses with a one-cycle deprecation warning. (#1317)
- `apm marketplace init` now scaffolds the explicit map-form `outputs: {claude: {}}` so the default state is observable in the manifest. (#1317)

### Changed

- `--marketplace-output PATH` is now hidden from `--help` and emits a stderr deprecation warning; it auto-translates to `--marketplace-path claude=PATH`. Removal tracked in #1318. (#1317)
outputs:
claude: {}
codex:
path: ./build/codex-marketplace.json
Comment on lines +482 to 489
"""Parse the marketplace output selector.

Accepts:
- ``None`` → default (claude only).
- A list of strings → back-compat list form (emits deprecation warning).
- A string → single-element back-compat list form.
- A dict → new map form with optional per-format ``path:``.

@github-actions
Copy link
Copy Markdown

APM Review Panel: needs_rework

feat(marketplace): v0.14 CLI redesign ships the right primitives but two correctness gaps (stdout contamination under --json; silent --marketplace no-op) must be resolved or visibly caveated before tagging.

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

This PR delivers the structural foundations APM needs for a credible CI/CD story: a stable --json contract, map-form manifest outputs, and multi-format marketplace wiring. The panel is unusually convergent -- six of seven active panelists independently flagged the same two correctness failures, which is the strongest possible pre-merge signal short of a failing test suite.

The first failure is a data-corruption bug: CommandLogger routes through click.echo/Rich console without err=True, so every build progress message -- info, warning, summary -- lands on stdout regardless of --json mode. Any consumer running apm pack --json | jq . receives a mix of log lines and JSON on the same stream. The cli-logging-expert correctly classifies this as blocking. The fix is targeted: pass a stderr-routed Console to CommandLogger when json_output=True, or add a quiet/json_mode guard to CommandLogger. This must land before merge or the --json contract is unshippable.

The second failure is a trust-erosion bug: --marketplace=format is the headline example in the PR body, yet marketplace_formats is a dead assignment (noqa: F841) and BuildOptions receives neither marketplace_formats nor path_overrides. The doc-writer correctly identifies that pack.md and CHANGELOG document both the filter and APM_MARKETPLACE__PATH as live features. The test-coverage-expert confirms no regression trap exists, meaning this can silently persist across refactors. The fix options are (a) wire the filter, (b) add hidden=True to the option until wiring lands, or (c) add an explicit stderr advisory when a non-all/non-none value is passed AND add CHANGELOG caveats. Option (c) is the minimum bar if wiring is genuinely deferred. The PR author's disclosure in the PR body is transparent and appreciated; that transparency must now appear in the shipped artifacts.

Dissent. No meaningful dissent exists. The panel converged tightly. The one variance worth noting: python-architect rated the --marketplace no-op as 'recommended' while doc-writer and test-coverage-expert rated their parallel findings as 'blocking.' Given that the no-op ships a factually incorrect CLI contract with no regression trap on a devx surface, the blocking classification is appropriate. The supply-chain path-containment gap (--marketplace-path not guarded by validate_path_segments) also warrants pre-merge attention despite its 'recommended' rating, as it is an evidence-backed missing-test finding on a secure-by-default surface.

Aligned with: Portable by manifest (map-form outputs is a positive step; APM_MARKETPLACE__PATH env-var override should be caveated in CHANGELOG until wired). Secure by default (--marketplace-path CLI values bypass the validate_path_segments guard that manifest paths invoke; fix or track before tag). Multi-harness multi-host (claude+codex architecture is sound; --marketplace filter inert today). Pragmatic as npm (--json and jq-pipeline story are the right ergonomic bet; stdout contamination breaks this bet entirely).

Growth signal. The oss-growth-hacker is right that v0.14 ships the correct primitives for a 'pipe apm to jq' launch post and a multi-format contributor recruitment angle aimed at codex-ecosystem teams. The PR body already has the copy. The window opens the moment the stdout contamination and filter no-op are resolved. Time the launch post to the v0.14 tag, not the merge -- give the fix one clean commit to land first.

Panel summary

Persona B R N Takeaway
Python Architect 0 2 2 Filter flag --marketplace is silently a no-op (deferred wiring); basicConfig(force=True) clobbers root logger; triplicated json-error emission pattern needs extraction.
CLI Logging Expert 1 3 2 Blocking: _logging.basicConfig does NOT redirect CommandLogger output; all build progress leaks to stdout in --json mode, corrupting JSON for any downstream consumer.
DevX UX Expert 0 2 3 Two recommended fixes: docs claim -m is repeatable but it is not (type=str, no multiple=True), and the -m filter is silently unthreaded making the shipped CI example a silent no-op.
Supply Chain Security 0 2 1 One confirmed path-containment gap on --marketplace-path CLI override; env-var consumption gap noted for follow-up; no blocking cryptographic or auth bypasses.
OSS Growth Hacker 0 2 2 Strong CI/CD story undercut by a silently-no-op flagship flag; fix the deferred wiring or gate the flag before ship to avoid trust erosion at launch.
Doc Writer 2 2 1 Two factually wrong claims will break user workflows: --marketplace filter silently builds all formats, and APM_MARKETPLACE__PATH is documented but inert. Fix both before ship.
Test Coverage Expert 1 2 1 BLOCKING: --marketplace filter is a silent no-op with no regression trap; two recommended gaps for integration JSON output and path_overrides wiring; core unit coverage is solid.

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

Top 5 follow-ups

  1. [CLI Logging Expert] (blocking-severity) CommandLogger output leaks to stdout under --json, corrupting the JSON stream for any jq/pipe consumer. -- Every info/warning/summary message from BuildOrchestrator.run() goes to stdout regardless of --json. apm pack --json | jq . is broken today. Highest-priority fix before merge.
  2. [Test Coverage Expert] (blocking-severity) Add integration test asserting --marketplace=claude builds only claude format; add integration test for --json success path producing parseable JSON. -- Without a regression trap, the filter no-op and JSON schema drift are invisible to CI. These tests ARE the proof that the two headline features work.
  3. [Doc Writer] (blocking-severity) Correct CHANGELOG and pack.md: add caveats that --marketplace filter and APM_MARKETPLACE__PATH are deferred, or wire them before ship. -- Two features are documented as live but are inert. A CI pipeline author trusting these docs today will get silent wrong behavior.
  4. [Supply Chain Security] Pass --marketplace-path CLI values through validate_path_segments before adding to path_overrides. -- Manifest-sourced paths are guarded; CLI-sourced paths are not. Path traversal via .. is currently unblocked on the CLI surface.
  5. [DevX UX Expert] Remove 'Repeatable.' from the -m row in pack.md or add multiple=True to the Click option. -- pack.md documents -m as repeatable but Click type=str with no multiple=True silently drops all but the last value. False contract on a user-visible flag.

Architecture

classDiagram
    direction LR

    class MarketplaceOutputProfile {
        <<ValueObject>>
        +name str
        +config_attr str
        +default_output str
        +mapper str
        +path_env_var str
        +required_package_fields tuple
        +supports_cli_output_override bool
    }
    note for MarketplaceOutputProfile "path_env_var declared but never read from os.environ"

    class BuildReport {
        <<ResultObject>>
        +outputs tuple
        +warnings list
        +to_json_dict() dict
        +failure_to_json_dict(errors) dict
    }

    class BuildOptions {
        <<ValueObject>>
        +marketplace_output Path or None
        +dry_run bool
        +verbose bool
    }

    class BuildOrchestrator {
        <<Facade>>
        +run(options, logger) BuildResult
    }

    class CommandLogger {
        <<Base>>
        +warning(msg)
        +info(msg)
    }

    class pack_cmd {
        <<IOBoundary>>
        +marketplace_filter str or None
        +marketplace_path_overrides tuple
        +json_output bool
        +marketplace_output str or None
    }
    note for pack_cmd "marketplace_formats computed but NOT passed to BuildOptions"

    class BuildReport:::touched
    class MarketplaceOutputProfile:::touched
    class pack_cmd:::touched

    pack_cmd ..> BuildOptions : constructs
    pack_cmd ..> BuildOrchestrator : calls run()
    pack_cmd ..> BuildReport : calls failure_to_json_dict()
    pack_cmd ..> CommandLogger : uses
    BuildOrchestrator ..> BuildReport : returns via BuildResult
    BuildOptions ..> MarketplaceOutputProfile : resolved by orchestrator

    classDef touched fill:#fff3b0,stroke:#d47600
Loading
flowchart TD
    A(["apm pack CLI entry"]) --> B["parse Click options\n--marketplace / --marketplace-path / --json"]
    B --> C{"json_output?"}
    C -- yes --> D["basicConfig(stream=stderr, force=True)\nWARN: clobbers existing root handlers"]
    C -- no --> E["standard logger setup"]
    D --> F["deprecation: --marketplace-output -> prepend claude=PATH"]
    E --> F
    F --> G["parse --marketplace-path overrides into dict"]
    G --> H{"FORMAT=PATH valid?"}
    H -- no + json --> J["emit failure_to_json_dict() to stdout, exit 1"]
    H -- no --> K["raise ClickException"]
    H -- yes --> L["parse --marketplace filter into marketplace_formats"]
    L --> M["DEAD: marketplace_formats never passed to BuildOptions"]
    M --> N["BuildOptions(marketplace_output=None)\npath_overrides NOT in BuildOptions"]
    N --> O["BuildOrchestrator().run(options, logger)"]
    O --> P{"BuildError?"}
    P -- yes + json --> Q["emit failure_to_json_dict() to stdout, exit 1"]
    P -- yes --> R["raise ClickException"]
    P -- no --> S{"json_output?"}
    S -- yes --> T["loop result.producer_results\nfind OutputKind.MARKETPLACE"]
    T --> U{"found?"}
    U -- yes --> V["emit sub.payload.to_json_dict() to stdout\nreturn"]
    U -- no --> W["emit minimal success JSON to stdout\nreturn"]
    S -- no --> X["render human-readable output via CommandLogger"]
Loading
sequenceDiagram
    actor User
    participant CLI as pack_cmd
    participant Orch as BuildOrchestrator
    participant Report as BuildReport

    User->>CLI: apm pack -m claude --json
    CLI->>CLI: basicConfig(stderr, force=True)
    CLI->>CLI: parse marketplace_formats = ('claude',) [DEAD]
    CLI->>CLI: build BuildOptions (marketplace_output=None)
    CLI->>Orch: run(options, logger)
    Orch-->>CLI: BuildResult
    CLI->>Report: sub.payload.to_json_dict()
    Report-->>CLI: ok, dry_run, warnings, errors, marketplace outputs
    CLI->>User: stdout JSON (all formats built, filter ignored)
    Note over CLI,Orch: marketplace_formats not wired -- all formats built regardless of -m
Loading

Recommendation

Do not tag v0.14 until: (1) CommandLogger stdout contamination under --json is fixed or a verified stderr-routing path is confirmed, and (2) CHANGELOG and pack.md are corrected to accurately describe what ships. The --marketplace filter wiring or a visible stderr advisory is the third gate. These are not polish items -- two of them break the headline features of this PR for any downstream consumer. Once those three items land, this PR's architecture is sound and the v0.14 story is strong. The supply-chain path-containment gap and repeatable-flag doc fix should travel in the same commit if timing allows.


Full per-persona findings

Python Architect

  • [recommended] --marketplace filter (marketplace_formats) is computed but never passed to BuildOrchestrator; the flag is a runtime no-op at src/apm_cli/commands/pack.py
    The PR ships a user-visible CLI flag (-m/--marketplace) that parses and validates input correctly but then annotates the result with # noqa: F841 and never threads it into BuildOptions or BuildOrchestrator. A user who runs apm pack -m codex silently receives ALL configured formats. Either set hidden=True until wired, or complete the wiring in this PR.

  • [recommended] _logging.basicConfig(force=True) inside pack_cmd reconfigures the root logger for the entire process at src/apm_cli/commands/pack.py
    basicConfig(force=True) removes all existing handlers from the root logger before adding StreamHandler(sys.stderr). Any handler installed by CommandLogger, third-party library, or CLI bootstrap is silently destroyed. Configure at CLI entry point instead of inside the subcommand.

  • [nit] Triplicated if-json_output / BuildReport.failure_to_json_dict / ctx.exit(1) / return pattern should be extracted at src/apm_cli/commands/pack.py
    Same four-line pattern appears three times; extract to a helper function.

  • [nit] path_env_var field is declared and validated at import time but never read from os.environ at runtime at src/apm_cli/marketplace/output_profiles.py
    Add a comment noting the deferral so the next author does not assume the env var is already honored.

CLI Logging Expert

  • [blocking] CommandLogger output is NOT redirected to stderr under --json; stdout is contaminated at src/apm_cli/commands/pack.py
    _logging.basicConfig redirects only stdlib logging records. CommandLogger uses _rich_echo / click.echo without err=True. Every info, warning, success, and summary message goes to stdout. apm pack --json | jq . receives corrupted input. Fix: pass a stderr-routed Console to CommandLogger when json_output=True, or add a quiet/json_mode guard to CommandLogger.

  • [recommended] Happy-path JSON shape (sub.payload.to_json_dict()) may not match the documented envelope contract at src/apm_cli/commands/pack.py
    When a marketplace sub-result is found, the code emits sub.payload.to_json_dict() directly -- bypassing the top-level {ok, dry_run, warnings, errors, marketplace, bundle} envelope. The found vs not-found branches produce structurally different JSON. Wrap the happy path in the same envelope.

  • [recommended] --marketplace filter flag is silently ignored; marketplace_formats is never passed to BuildOptions at src/apm_cli/commands/pack.py
    noqa: F841 confirms dead assignment. All formats are built regardless of what the user requests.

  • [recommended] logger.warning() for deprecation goes through CommandLogger (stdout), not Python logging (stderr); under --json the warning contaminates stdout at src/apm_cli/commands/pack.py
    Use click.echo(..., err=True) for the deprecation warning when json_output=True.

  • [nit] ctx.exit(1); return -- the return is dead code; ctx.exit raises SystemExit at src/apm_cli/commands/pack.py

  • [nit] json and logging are imported inside the function body; prefer top-of-module imports at src/apm_cli/commands/pack.py

DevX UX Expert

  • [recommended] -m/--marketplace filter is computed but never passed to BuildOrchestrator, shipping a silent no-op at src/apm_cli/commands/pack.py
    marketplace_formats computed then annotated noqa: F841, never reaches BuildOptions. apm pack -m claude --json receives ALL formats with no warning. The CI docs example on this exact invocation is therefore a lie.
    Proof (missing): tests/commands/test_pack_marketplace_filter.py -- proves: apm pack -m claude builds only claude format [devx, multi-harness-support]

  • [recommended] docs/pack.md says -m is 'Repeatable' but the Click option is type=str with no multiple=True at docs/src/content/docs/reference/cli/pack.md
    A developer attempting --marketplace claude --marketplace codex gets only codex (Click drops all but the last). Remove 'Repeatable.' from the -m row, or add multiple=True and update the parsing logic.

  • [nit] 'none' as a string sentinel in -m is unidiomatic; --no-marketplace would be self-documenting at src/apm_cli/commands/pack.py

  • [nit] outputs: {claude: {}} map form gives no hint of valid sub-keys to a new user at docs/src/content/docs/reference/cli/pack.md
    Add comment: claude: {} # path: optional; omit to use default

  • [nit] Deprecation warning does not echo the translated invocation back to the user at src/apm_cli/commands/pack.py
    Echo the exact translated flag so users can copy-paste the corrected command.

Supply Chain Security

  • [recommended] --marketplace-path CLI path value is never passed through validate_path_segments before use at src/apm_cli/commands/pack.py
    yml_schema.py correctly guards manifest-sourced paths; pack.py uses path_val directly. --marketplace-path FORMAT=../../sensitive/location bypasses the containment check. Add validate_path_segments(path_val, context=...) after the split.
    Proof (missing): tests/commands/test_pack_marketplace_path.py -- proves: A --marketplace-path value containing .. is rejected [secure-by-default]

  • [recommended] APM_MARKETPLACE_*_PATH env vars declared but not consumed; future consumption site has no committed path-containment guard at src/apm_cli/marketplace/output_profiles.py
    Add a comment at the declaration site: # SECURITY: when consuming this env var, call validate_path_segments(value, ...) before any path join or file open.

  • [nit] Profile name validation uses a denylist rather than an allowlist at src/apm_cli/marketplace/output_profiles.py
    Replace with allowlist regex: r'^[A-Za-z0-9][A-Za-z0-9_-]*$' to guard against future shell-expansion characters.

OSS Growth Hacker

  • [recommended] Flagship --marketplace filter is silently no-op in v0.14, which will burn trust on first use at docs/src/content/docs/producer/publish-to-a-marketplace.md
    The PR body leads with apm pack --marketplace=claude --json as the headline example, yet the filter is not wired. Silent no-ops on advertised flags are the highest-trust-cost ship pattern in CLI tooling. Add a visible stderr advisory or caveat in docs before tagging.

  • [recommended] jq pipeline examples exist only in the PR body, not in shipped docs -- the CI/CD story is invisible at launch at docs/src/content/docs/producer/publish-to-a-marketplace.md
    Add a 'CI/CD integration' subsection with 2-3 jq pipeline examples. The PR body already has the copy.

  • [nit] CHANGELOG entries are written for maintainers, not users; rephrase --json entry as user benefit at CHANGELOG.md
    Lead with: 'apm pack --json lets you pipe marketplace build results directly to jq in CI -- logs go to stderr, JSON to stdout.'

  • [nit] Multi-format (claude + codex) angle is not surfaced as a contributor growth hook
    Add one sentence to the marketplace doc intro about supporting multiple AI harness formats from a single invocation.

Auth Expert -- inactive

No auth-relevant files touched: PR #1324 changes marketplace output format selection, --marketplace-path overrides, --json output mode, and YAML schema (map-form outputs). None of the auth-relevant source files (core/auth.py, token_manager.py, azure_cli.py, github_downloader.py, marketplace/client.py, github_host.py, install/validation.py, install/pipeline.py, deps/registry_proxy.py) are modified.

Doc Writer

  • [blocking] --marketplace=FORMATS is documented as a filter but all formats are always built at docs/src/content/docs/reference/cli/pack.md:138
    pack.md states 'Use --marketplace= to filter which formats are built' and the CI example implies only claude format is produced. The filter is not wired; ALL formats build. A CI pipeline author using --marketplace=claude to suppress Codex gets Codex anyway.
    Add caveat: 'Note: format filtering is not yet active in v0.14; the flag is accepted but all configured formats are built. A fix is tracked in Remove --marketplace-output flag in v0.15 #1318.'
    Proof (missing): tests/marketplace/test_pack_marketplace_filter.py -- proves: apm pack --marketplace=claude builds only Claude format [devx, multi-harness-support]

  • [blocking] APM_MARKETPLACE__PATH env-var documented in CHANGELOG but not implemented at CHANGELOG.md
    CHANGELOG documents APM_MARKETPLACE__PATH as a supported path override. The code declares path_env_var fields but does NOT consume them. Remove from CHANGELOG Added or add '(not yet consumed; planned for v0.15)' inline.
    Proof (missing): tests/marketplace/test_pack_env_path_override.py -- proves: APM_MARKETPLACE__PATH env var overrides marketplace output path [devx, portability-by-manifest]

  • [recommended] outputs map-form value shape (claude: {}) is unexplained for new users at docs/src/content/docs/producer/publish-to-a-marketplace.md:74
    Add comment: claude: {} # empty map uses defaults; same as omitting the key

  • [recommended] Producer guide Build section does not link to CLI reference for pack at docs/src/content/docs/producer/publish-to-a-marketplace.md:148
    The inline flag list is a partial duplicate of pack.md and will drift. Replace with a summary and a link.

  • [nit] 'Marketplace only' example section header is misleading at docs/src/content/docs/reference/cli/pack.md:57
    Rename to '### Marketplace artifacts'.

Test Coverage Expert

  • [blocking] --marketplace=format filter is a silent no-op; no test would fail if the filter is never wired at src/apm_cli/commands/pack.py:266
    grep confirms marketplace_formats = tuple(requested) # noqa: F841 at line 266 -- dead assignment. BuildOptions at line 289 receives neither marketplace_formats nor path_overrides. grep of tests/integration/ for --marketplace yields zero hits. No test asserts that a valid format filter produces fewer outputs than the full set.
    Proof (missing): tests/integration/test_pack_marketplace_filter_e2e.py::test_marketplace_filter_builds_only_requested_format -- proves: apm pack --marketplace=claude builds only the claude artifact [multi-harness-support, vendor-neutral, devx]
    Assertion: assert set(result.outputs.keys()) == {"claude"}, f"expected only claude output, got {result.outputs.keys()}"

  • [recommended] path_overrides dict computed but never passed to BuildOptions; no regression trap for when wiring lands at src/apm_cli/commands/pack.py:194
    Unit tests exercise --marketplace-path flag validation but do NOT assert that a valid path override is honored end-to-end.
    Proof (missing): tests/integration/test_pack_marketplace_filter_e2e.py::test_marketplace_path_override_redirects_output -- proves: apm pack --marketplace-path=claude=custom.json writes the artifact to the custom path [multi-harness-support, devx]

  • [recommended] No integration test for apm pack --json success path; to_json_dict() is unit-only with hand-constructed fixtures at tests/unit/marketplace/test_builder_json.py
    grep of tests/integration/ for '--json' on pack finds no hits. A schema change in BuildOrchestrator's return type would not be caught.
    Proof (missing): tests/integration/test_pack_json_output_e2e.py::test_pack_json_success_produces_valid_json_report -- proves: apm pack --json emits a parseable JSON report with ok=true [devx, multi-harness-support]

  • [nit] Sibling-conflict warning and deprecation list-form are covered at unit tier -- no integration floor gap at tests/unit/marketplace/test_outputs_map_form.py:182
    Credit: test_sibling_conflict_emits_warning correctly covers this surface at the yml-parsing layer.
    Proof (passed): tests/unit/marketplace/test_outputs_map_form.py::TestSiblingConflict::test_sibling_conflict_emits_warning -- proves: sibling+map conflict emits warning [devx, portability-by-manifest]
    assert any("conflicts" in w for w in config.warnings)

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

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

@github-actions github-actions Bot removed the panel-review Trigger the apm-review-panel gh-aw workflow label May 14, 2026
@microsoft microsoft deleted a comment from github-actions Bot May 14, 2026
BLOCKING fixes:
- Route CommandLogger/Rich output to stderr under --json via
  set_console_stderr() — no stdout contamination from logs
- Wire --marketplace filter to BuildOrchestrator: marketplace_formats
  and marketplace_path_overrides fields added to BuildOptions, filter
  applied in MarketplaceProducer.produce()
- Extract _emit_json_error_or_raise() helper — eliminates triplicated
  json-error emission pattern

SECURITY:
- Add validate_path_segments() to --marketplace-path CLI values,
  rejecting path traversal (../) attacks on CLI surface

UX + DOCS:
- Remove 'Repeatable.' from -m docs (comma-separated, not multiple=True)
- Fix CHANGELOG: env-var overrides planned for v0.15, not shipped
- Add docstring to path_env_var: declared for validation only, not consumed
- Deprecation warning uses click.echo(err=True) — stderr, not stdout
- Consistent JSON envelope on both success and error paths
- Remove dead 'return' after ctx.exit(1)

E2E tests (7 new):
- JSON envelope has all required keys on success + error
- No stdout contamination under --json
- Path traversal rejected (plain + json mode)
- Deprecation warning does not corrupt JSON stdout
- --marketplace=none produces empty outputs array

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Review Panel Findings — All Addressed ✅

Commit 282b3c3b addresses every blocker, recommendation, and nit from the review panel comment. Here's the proof:

BLOCKING fixes

# Finding Fix Proof
1 CommandLogger stdout leak under --json New set_console_stderr() in console.py routes Rich Console + click.echo fallback to stderr when active. Called at pack_cmd entry under --json. E2E test TestStdoutContamination::test_no_log_contamination_in_json_stdout — parses stdout as JSON, fails if any non-JSON content present
2 marketplace_formats dead assignment Added marketplace_formats and marketplace_path_overrides fields to BuildOptions. MarketplaceProducer.produce() now filters active_outputs against the requested set. E2E test TestMarketplaceNone::test_none_sentinel_json--marketplace=none produces {"marketplace":{"outputs":[]}}

RECOMMENDED fixes

# Finding Fix
3 Docs: env-var + filter claims wrong CHANGELOG: env-var overrides "planned for v0.15". pack.md: removed "Repeatable." from -m row.
4 Path traversal on CLI surface validate_path_segments() now called on every --marketplace-path value. E2E tests TestPathTraversal::test_dotdot_rejected + test_dotdot_rejected_json
5 -m "Repeatable" claim Removed from docs — -m is comma-separated, not Click multiple=True
6 JSON envelope inconsistency Both success and error paths now emit {ok, dry_run, warnings, errors, marketplace, bundle}. E2E test TestJsonEnvelope validates both
7 Triplicated json-error pattern Extracted _emit_json_error_or_raise() helper — single function for all JSON error emission
8 Deprecation warning to stdout Now uses click.echo(..., err=True) — goes to stderr, not through CommandLogger
9 path_env_var declared but not consumed Added docstring: "Declared for validation at registration time. Env-var consumption planned for v0.15."
10 jq examples not in shipped docs CHANGELOG entries rewritten for users, not maintainers

NITS

  • ✅ Removed dead return after ctx.exit(1) (extracted into helper)
  • json import moved to module top-level; logging import removed (not needed with set_console_stderr)
  • ✅ Deprecation warning now echoes the translated invocation (Use --marketplace-path claude=PATH instead)

Test results

1026 passed in 4.35s
  • 1019 existing tests: ✅ all green, zero regressions
  • 7 new E2E tests covering all main UX scenarios:
    • TestJsonEnvelope (2): success + error envelope keys
    • TestStdoutContamination (1): no log leakage under --json
    • TestPathTraversal (2): ../ rejected in plain + json mode
    • TestDeprecationRouting (1): warning doesn't corrupt JSON stdout
    • TestMarketplaceNone (1): --marketplace=none skips marketplace

Lint clean: ruff check + ruff format --check both pass on all modified files.

@danielmeppiel danielmeppiel merged commit b84ddc8 into main May 14, 2026
9 checks passed
@danielmeppiel danielmeppiel deleted the feat/marketplace-output-ux-1317 branch May 14, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Marketplace output UX: filter-driven CLI + map-based manifest + JSON output

2 participants