APM Review Panel: ship_with_followups

Sanitize marketplace name to kebab-case at output boundary -- correct fix, clean scope, but silent mutation warning and compose()-tier regression trap must be tracked as follow-ups before next release.

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

The fix is correct, well-scoped, and removes a concrete adoption blocker: enterprise repos with dots, underscores, or mixed-case names in apm.yml were silently producing marketplace.json files rejected by Copilot App's kebab-case validation. The implementation is appropriately confined to the output boundary, leaving resolution, registry lookups, and install flows untouched. Both mappers are updated consistently, and the 16 unit tests confirm the function's correctness in isolation. This is a net-positive merge.

Five of seven active panelists independently raised the same issue: when sanitize_marketplace_name() rewrites config.name, neither mapper emits any user-visible signal. The MapperResult.warnings channel is already plumbed from both mappers through builder.py to CLI output -- the existing duplicate-name warning logic proves the path works end-to-end. The omission of a warning is an oversight, not a design decision, and the five-panelist convergence is the highest-signal finding in this panel. Separately, devx-ux-expert and supply-chain-security-expert both flag the all-punctuation fallback to the literal string "marketplace" as a silent fail-open that creates a registry collision risk between distinct marketplace configs. I side with their recommendation to raise a BuildError when sanitization yields an empty result, rather than cli-logging-expert's softer "louder warning" position -- a collision at the registry level is a defect, not a degraded UX, and build-time failure is the correct gate.

The test-coverage-expert finding carries the greatest structural weight by evidence-weighting rules: removing sanitize_marketplace_name() from either mapper's compose() leaves every existing test green, because all integration fixtures already use valid kebab-case names. Evidence outcome=missing on an integration-tier user promise ranks above all recommended-opinion findings in this panel. It is the first tracking issue to open post-merge. The two doc gaps -- a missing CHANGELOG entry and a now-factually-incorrect "only structural transform" claim in publish-to-a-marketplace.md -- are standard hygiene for any behavior-changing fix and must land in the same release cycle.

Dissent. cli-logging-expert recommends a louder warning for the empty-sanitization fallback; devx-ux-expert and supply-chain-security-expert recommend a BuildError. I side with BuildError: silent fallback to the generic name "marketplace" is a registry collision-risk defect, not a degraded-experience issue, and build-time failure is the appropriate gate.

Aligned with: Portable by manifest -- normalizing the name field ensures generated marketplace.json conforms to the registry spec regardless of how the author names their project in apm.yml; Pragmatic as npm -- npm, cargo, and brew all surface warnings when a config value is coerced at publish time, and silent mutation deviates from the "no surprises" contract; Secure by default -- the all-punctuation fallback is a fail-open that creates a registry collision surface, and replacing it with a BuildError hardens the default.

Growth signal. The launch beat for the next release note: "APM now works with any repo naming convention -- dots, underscores, enterprise prefixes -- no renaming required." Enterprise repos with dotted names (e.g. my.company.tools) were a hard no-go before this fix; they now just work. This should be the headline entry in the next CHANGELOG and release announcement to re-engage users who hit the Copilot App validation error and churned.

Panel summary

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

Top 5 follow-ups

1. [Test Coverage Expert] Add compose()-tier regression tests that construct a MarketplaceConfig with name='my.marketplace', call ClaudeMarketplaceMapper.compose() and CodexMarketplaceMapper.compose(), and assert result.document['name']=='my-marketplace'. -- Evidence outcome=missing on an integration-tier user promise: removing sanitize_marketplace_name() from either mapper's compose() passes every existing test today. This is the strongest structural gap in the PR -- a regression trap is required before next release.
2. [CLI Logging Expert] Emit a MapperResult.warnings entry in both ClaudeMarketplaceMapper and CodexMarketplaceMapper when sanitized != config.name; add a distinct warning for the fallback path. CodexMarketplaceMapper also needs to pass the warnings tuple to MapperResult. -- Five panelists converged on this independently -- the highest-convergence signal in the panel. The warnings channel is already wired to CLI output by builder.py. Silent mutation of a user-declared name breaks the "no surprises" contract.
3. [DevX UX Expert] Replace return result or "marketplace" in sanitize_marketplace_name() with a raised BuildError when the sanitized result is empty after stripping non-alphanumeric characters. -- Two panelists (devx-ux, supply-chain-security) independently flagged the all-punctuation fallback as a silent registry collision risk. Fail-open to a generic name is a defect; build-time failure is the correct gate.
4. [Doc Writer] Add a CHANGELOG.md [Unreleased] ### Fixed entry for the kebab-case normalization and correct the "only structural transform" claim in docs/src/content/docs/producer/publish-to-a-marketplace.md (line 119-121). -- The PR changes user-observable output for any user whose project name contains dots, underscores, or uppercase. A CHANGELOG entry is the standard discovery mechanism and the doc claim is now factually incorrect.
5. [Supply Chain Security Expert] Add a lightweight upstream format guard in yml_schema.py requiring that config.name contains at least one alphanumeric character, surfacing the error at config-load time rather than at compose time. -- An all-punctuation name currently satisfies schema validation and flows silently to the sanitizer. Early detection gives the author a cleaner error message and closes the gap that lets malformed names reach the output boundary at all.

Architecture

classDiagram
    direction LR
    class MarketplaceOutputMapper {
        <<Abstract>>
        +uses_remote_metadata bool
        +compose(config, resolved, remote_metadata) MapperResult
    }
    class ClaudeMarketplaceMapper {
        <<ConcreteMapper>>
        +uses_remote_metadata = True
        +compose(config, resolved, remote_metadata) MapperResult
    }
    class CodexMarketplaceMapper {
        <<ConcreteMapper>>
        +compose(config, resolved, remote_metadata) MapperResult
    }
    class MapperResult {
        <<ValueObject>>
        +document dict
        +warnings tuple
        +diagnostics tuple
    }
    class MarketplaceConfig {
        <<ValueObject>>
        +name str
        +packages tuple
    }
    class sanitize_marketplace_name {
        <<PureFunction>>
        +sanitize_marketplace_name(name str) str
    }
    MarketplaceOutputMapper <|-- ClaudeMarketplaceMapper
    MarketplaceOutputMapper <|-- CodexMarketplaceMapper
    ClaudeMarketplaceMapper ..> MapperResult : produces
    CodexMarketplaceMapper ..> MapperResult : produces
    ClaudeMarketplaceMapper ..> MarketplaceConfig : reads
    CodexMarketplaceMapper ..> MarketplaceConfig : reads
    ClaudeMarketplaceMapper ..> sanitize_marketplace_name : output boundary
    CodexMarketplaceMapper ..> sanitize_marketplace_name : output boundary
    class ClaudeMarketplaceMapper:::touched
    class CodexMarketplaceMapper:::touched
    class sanitize_marketplace_name:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

flowchart TD
    A([apm marketplace build]) --> B[MarketplaceBuilder.write_output]
    B --> C[MarketplaceBuilder.compose_output]
    C --> D[MarketplaceBuilder._map_output]
    D --> E{profile.mapper}
    E -- claude --> F[ClaudeMarketplaceMapper.compose]
    E -- codex --> G[CodexMarketplaceMapper.compose]
    F --> H["sanitize_marketplace_name(config.name) NEW"]
    G --> I["sanitize_marketplace_name(config.name) NEW"]
    H --> J[doc.name = sanitized]
    I --> K["doc.name = sanitized / doc.interface.displayName = config.name original"]
    J --> L["MapperResult(doc + warnings + diagnostics)"]
    K --> M["MapperResult(doc only) - warnings channel empty"]
    L --> N["FS atomic_write marketplace.json"]
    M --> N

Recommendation

Approve and merge. The core fix is correct, the implementation is clean, and the enterprise adoption unlock is real. Open a tracking issue immediately for (1) the compose()-tier regression trap -- the most urgent item since the sanitize call can be silently deleted today with all tests still passing -- and (2) the silent mutation warning, which has five-panelist consensus and a clear implementation path through MapperResult.warnings. The CHANGELOG entry and doc correction are lightweight and must land in the same release. The BuildError for empty sanitization and the upstream yml_schema guard are follow-up PRs in the same sprint.

_{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 #2008 · 535.8 AIC · ⌖ 18.3 AIC · ⊞ 5.5K · ◷

APM Review Panel: ship_now

Ship: kebab-case sanitiser closes the silent-rejection cliff for marketplace producers with proven mutation-break coverage and zero blocking findings.

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

All nine panelists converged on ship. The two active architectural voices (python-architect, cli-logging-expert) confirmed the shared-helper extraction is well-motivated and follows the existing collect-then-render diagnostic pattern. Supply-chain-security certified a net-positive: the sanitiser narrows the injection surface on emitted names while the raw name stays internal-only. The test-coverage-expert returned zero findings with six evidence blocks at integration-with-fixtures tier, all outcome=passed, and the mutation-break gate is proven -- removing the sanitize guard fails name-assertion tests; neutering the diagnostic condition fails test_rewrite_emits_warning_diagnostic. This is the empirical-proof bar the maintainer asked for.

No material dissent exists. The two deferred nits are cleanly out of scope: (1) typing BuildDiagnostic.level as Literal touches a pre-existing shared type across the codebase, not this PR's surface; (2) widening the compose-only preview helper to surface diagnostics is a separate refactor since the real write path already renders them. Both are bounded follow-ups with no user-facing regression risk if deferred.

Strategically, this change reinforces APM's "just works" positioning for the producer publish flow. A marketplace.json that silently fails on the Copilot App side is exactly the kind of first-run cliff that kills adoption before a user ever sees value. Closing it with a warn-not-error approach (npm-like ergonomics per devx-ux-expert) is the right trade-off: producers are informed, never blocked.

Aligned with: Pragmatic as npm (warn-not-error mirrors npm normalize-package-name; producers informed, never blocked); Portable by manifest (raw manifest name preserved for internal resolution, only the emitted artifact is sanitised); Multi-harness / multi-host (Claude and Codex mappers apply identical sanitisation via one shared helper); OSS community-driven (real community pain point shipped with full empirical proof); Secure by default (emitted name constrained to [a-z0-9-], closing injection surface on the published artifact).

Growth signal. Closes the "marketplace.json silently rejected by Copilot App" first-run cliff for producers. Amplify in release notes as a "just works" DX win -- producers no longer need to manually kebab-case their config name to get a successful publish.

Panel summary

B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships. All eight in-scope nits were folded into this PR (see "Folded in this run"); two out-of-scope nits are deferred.

Top 2 follow-ups

1. [Python Architect] Type BuildDiagnostic.level as a Literal -- pre-existing shared type used across the codebase; typing it narrows the contract but touches many call-sites. Bounded refactor, no user-facing urgency.
2. [CLI Logging Expert] Surface diagnostics from the compose-only compose_codex_marketplace_json helper -- it returns a 2-tuple and drops the diagnostic list. The real write path already surfaces them; widening the preview helper is a separate refactor with no regression risk if deferred.

Architecture

classDiagram
    direction LR
    class MarketplaceOutputMapper {
      <<AbstractBase>>
      +compose(config, resolved, remote_metadata) MapperResult
    }
    class ClaudeMarketplaceMapper {
      <<ConcreteStrategy>>
      +compose(...) MapperResult
    }
    class CodexMarketplaceMapper {
      <<ConcreteStrategy>>
      +compose(...) MapperResult
    }
    class MapperResult {
      <<ValueObject>>
      +document dict
      +warnings tuple
      +diagnostics tuple~BuildDiagnostic~
    }
    class BuildDiagnostic {
      <<ValueObject>>
      +level str
      +message str
    }
    class MarketplaceBuilder {
      <<Facade>>
      +build() BuildReport
    }
    class BuildReport {
      <<ValueObject>>
      +outputs tuple~MarketplaceOutputReport~
      +diagnostics tuple~BuildDiagnostic~
    }
    class sanitize_marketplace_name {
      <<Pure>>
      +__call__(name) str
    }
    class _sanitized_name_with_diagnostic {
      <<Pure>>
      +__call__(config_name) tuple
    }
    MarketplaceOutputMapper <|-- ClaudeMarketplaceMapper
    MarketplaceOutputMapper <|-- CodexMarketplaceMapper
    ClaudeMarketplaceMapper ..> MapperResult : returns
    CodexMarketplaceMapper ..> MapperResult : returns
    MapperResult *-- BuildDiagnostic : contains
    MarketplaceBuilder *-- MarketplaceOutputMapper : delegates
    MarketplaceBuilder ..> BuildReport : returns
    BuildReport o-- BuildDiagnostic : aggregates
    note for _sanitized_name_with_diagnostic "Shared helper: calls sanitize_marketplace_name,\nemits BuildDiagnostic when rewrite occurs"
    note for MarketplaceBuilder "Collect-then-render: diagnostics\naccumulate across mappers into BuildReport"
    class ClaudeMarketplaceMapper:::touched
    class CodexMarketplaceMapper:::touched
    class sanitize_marketplace_name:::touched
    class _sanitized_name_with_diagnostic:::touched
    classDef touched fill:#fff3b0,stroke:#d47600

flowchart TD
    A["apm.yml: name = 'My.Marketplace_Name'"] --> B["MarketplaceBuilder.build()"]
    B --> C["ClaudeMarketplaceMapper.compose() / CodexMarketplaceMapper.compose()"]
    C --> D["_sanitized_name_with_diagnostic(config.name)"]
    D --> E["sanitize_marketplace_name(name)"]
    E --> F{"sanitized == config_name?"}
    F -- Yes --> G["Return sanitized, diagnostics=[]"]
    F -- No --> H["Emit BuildDiagnostic level=warning\noriginal + sanitized in message"]
    H --> I["Return sanitized, diagnostics=[diag]"]
    G --> J["doc['name'] = sanitized"]
    I --> J
    J --> K{"Mapper type?"}
    K -- Claude --> L["diagnostics = list(name_diagnostics) +\nper-plugin diagnostics"]
    K -- Codex --> M["doc['interface']['displayName'] = config.name\nMapperResult(doc, (), tuple(name_diagnostics))"]
    L --> N["MapperResult(doc, warnings, tuple(diagnostics))"]
    M --> O["[FS] MarketplaceBuilder writes marketplace.json"]
    N --> O
    O --> P["BuildReport.diagnostics aggregates all outputs"]

Recommendation

Zero blocking findings, zero recommended findings, all in-scope nits folded, mutation-break gate proven, lint silent, 1484+22 tests green, and test-coverage-expert certified ship at integration-with-fixtures tier. The two deferred nits are cleanly out of scope with no user-facing risk. Ship.

Folded in this run

• (user-request) e2e integration test proving the full build emits a sanitized kebab-case name (empirical proof) -- resolved in 151fdd5d.
• (shepherd) CHANGELOG [Unreleased] > Fixed entry for the behavior change -- resolved in b7139be8.
• (devx-ux-expert + cli-logging-expert) non-blocking build warning naming original + emitted name on rewrite, via a shared _sanitized_name_with_diagnostic helper wired into both mappers -- resolved in 52ca9b51.
• (doc-writer) docs drift: publish guide + manifest schema now describe the kebab-case normalisation -- resolved in 52ca9b51.
• (python-architect) pre-compile the sanitiser regexes at module scope (codebase idiom) -- resolved in 3a3ba870.
• (devx-ux-expert) warning cited marketplace.yml; retargeted to "your marketplace config" since the recommended location is the apm.yml marketplace: block -- resolved in 3a3ba870.
• (supply-chain-security-expert) documented the intentional many-to-one mapping in the sanitiser docstring -- resolved in 3a3ba870.
• (oss-growth-hacker) CHANGELOG re-led with user value; publish guide now names the rewrite warning -- resolved in 3a3ba870.
• (doc-writer) added a kebab-case pointer on the schema 3.1 name row; split the dense producer sentence -- resolved in 3a3ba870.

Copilot signals reviewed

• src/apm_cli/marketplace/output_mappers.py -- LEGIT (sanitise -> sanitize naming): already resolved by the author at HEAD 0e40142e before this shepherding pass.
• src/apm_cli/marketplace/output_mappers.py -- LEGIT (docstring wording): already resolved by the author at HEAD 0e40142e before this shepherding pass.

Deferred (out-of-scope follow-ups)

These items were surfaced by the panel but cross the stated scope of this PR. Each suggests a separate follow-up.

• (python-architect) Type BuildDiagnostic.level as Literal["warning", "verbose"] -- scope boundary: a pre-existing shared type in diagnostics.py used across the marketplace subsystem, outside this PR's name-sanitisation surface.
• (cli-logging-expert) Widen the compose-only compose_codex_marketplace_json helper to surface diagnostics -- scope boundary: the user-facing write path (write_codex_marketplace_json -> write_output -> compose_output) already surfaces the diagnostic; changing the preview-helper signature is a separate refactor.

Regression-trap evidence (mutation-break gate)

• tests/integration/marketplace/test_name_sanitisation_e2e.py::TestClaudeOutputSanitisesName::test_written_json_name_is_kebab_case -- removed the sanitize_marketplace_name guard from both mappers; the name-assertion tests FAILED as expected; guard restored.
• tests/integration/marketplace/test_name_sanitisation_e2e.py::TestClaudeOutputSanitisesName::test_rewrite_emits_warning_diagnostic -- neutered the if sanitized != config_name diagnostic condition; the diagnostic test FAILED as expected; condition restored.

Lint contract

uv run --extra dev ruff check src/ tests/ and uv run --extra dev ruff format --check src/ tests/ both silent. Pylint R0801 duplication guard: 10.00/10.

CI

All required checks green on 3a3ba870: Lint, Build & Test Shard 1 (Linux), Build & Test Shard 2 (Linux), gate, Coverage Combine (Linux), PR Binary Smoke (Linux), Spec conformance gate, CodeQL (after 0 CI fix iterations).

Mergeability status

Convergence

1 outer iteration; 1 Copilot round. Final panel stance: ship_now.

Ready for maintainer review.

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