fix: make pack check-clean read-only (closes #2727)

[Daniel Meppiel (danielmeppiel)]

fix(pack): make check-clean read-only

TL;DR

apm pack --check-clean now treats the whole pack invocation as read-only, even
when --dry-run is omitted. Marketplace drift is detected against the existing
artifact, which remains unchanged, and the command exits with code 4.

Note

This closes #2727 and removes the need to pair --check-clean with
--dry-run for safety.

Problem (WHY)

• apm pack --check-clean regenerated and overwrote marketplace.json
  before comparing it, so a stale artifact appeared clean and the command
  exited 0.
• The same drift was detected correctly only when callers also passed
  --dry-run, making a release gate depend on a non-obvious safety flag.
• [!] The observed command output and reproduction are recorded in
  #2727.

The gate must preserve the artifact it validates so its result reflects the
working tree that existed when the command started.

Approach (WHAT)

• Derive one effective dry-run mode from --dry-run or --check-clean.
• Pass that mode through CommandLogger, BuildOptions, result rendering, and
  JSON output.
• Add a regression trap that first creates a clean marketplace artifact, then
  changes the manifest and invokes --check-clean without --dry-run.
• Update both command references to state that --check-clean is read-only.

Implementation (HOW)

• src/apm_cli/commands/pack.py - makes check_clean imply effective
  dry-run behavior before any artifact producer runs.
• tests/unit/commands/test_pack_cli_flags.py - proves drift exits 4 and
  preserves the existing marketplace bytes without an explicit --dry-run.
• docs/src/content/docs/reference/cli/pack.md - documents the read-only
  release-gate contract.
• packages/apm-guide/.apm/skills/apm-usage/commands.md - keeps the bundled
  command guidance aligned with the Starlight reference.

Diagrams

Legend: the dashed nodes show the new read-only routing and its no-write
guarantee before the existing drift comparison.

flowchart LR
    C["apm pack --check-clean"] --> D["effective_dry_run = true"]
    D --> O[BuildOrchestrator]
    O --> N["No pack outputs written"]
    N --> G[check_marketplace_drift]
    G --> X{Matches on-disk artifact}
    X -->|yes| E0["Exit 0"]
    X -->|no| E4["Exit 4"]
    classDef new stroke-dasharray: 5 5;
    class D,N new;

Loading

Trade-offs

• Implicit read-only mode. Chose to make --check-clean imply dry-run
  behavior; rejected validating after normal writes because that destroys the
  evidence being checked.
• Whole invocation protection. Chose to suppress every pack output rather
  than special-case marketplace writes, so the command's read-only contract is
  simple and observable.
• Architecture classification: ordinary-fix. The existing pack_cmd
  remains the single owner of CLI flag composition and all producers continue
  to consume the canonical BuildOptions.dry_run; no authority was added or
  split, so no new static boundary guard is required.

Benefits

1. Drift now produces exit code 4 without requiring an extra flag.
2. The checked-in marketplace artifact remains byte-for-byte unchanged.
3. Human output and JSON report the same effective read-only mode.
4. Both user-facing command references describe the corrected behavior.

Validation

Regression trap before the production guard:

FAILED ...test_detects_drift_without_mutating_existing_output
E       assert 0 == 4
mutation-break: regression test failed as expected

Targeted pack and marketplace suites:

95 passed in 1.70s

Test quality contracts:

54 passed in 127.68s (0:02:07)
[+] assertion-quality ratchet clean: AQ001=4, AQ002=12
[+] exact test duplicate ratchet clean: 1141 files, 0 allowed duplicate group(s)

Canonical lint and boundary gates:

All checks passed!
1679 files already formatted
Your code has been rated at 10.00/10
[+] auth-signal lint clean
[+] architecture boundary lint clean
YAML, file-length, and relative-path guards passed

Scenario Evidence

#	Scenario (user promise)	Principle(s)	Test(s) proving it	Type
1	apm pack --check-clean detects drift without changing the existing marketplace artifact	Governed by policy, DevX (pragmatic as npm)	tests/unit/commands/test_pack_cli_flags.py::TestCheckCleanFlag::test_detects_drift_without_mutating_existing_output (regression-trap for #2727)	unit

How to test

• Pack a marketplace project once and save the generated artifact bytes.
• Change a marketplace package version in apm.yml.
• Run apm pack --check-clean --offline without --dry-run.
• Confirm exit code 4 and confirm the artifact bytes are unchanged.

Closes #2727

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[Copilot]

Copilot review overview

Review tier: Lite
Findings: None

What changed in this PR

This PR fixes the apm pack --check-clean release gate so it is truly read-only: it no longer overwrites the existing on-disk marketplace.json (or other pack outputs) before checking drift, and it exits with code 4 when drift is detected (closing #2727).

Changes:

• Make --check-clean imply an effective_dry_run mode for the entire pack invocation (logger, build options, rendering, JSON envelope).
• Add a regression test that proves --check-clean (without --dry-run) detects drift (exit 4) without mutating the existing marketplace output bytes.
• Update both docs references to state that --check-clean is read-only and never writes pack outputs.

File	Description
src/​apm_cli/​commands/​pack.py	Derives and propagates effective_dry_run = dry_run or check_clean so --check-clean cannot mutate pack outputs before drift checks.
tests/​unit/​commands/​test_pack_cli_flags.py	Adds regression coverage ensuring drift is detected and existing marketplace bytes remain unchanged without explicitly passing --dry-run.
docs/​src/​content/​docs/​reference/​cli/​pack.md	Updates CLI reference to document --check-clean as a read-only release gate.
packages/​apm-guide/​.apm/​skills/​apm-usage/​commands.md	Aligns bundled command guidance with the updated read-only --check-clean contract.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: needs_rework

PR #2730 establishes the right read-only contract, but a legacy lockfile mutation and stale release recipe still make that promise unsafe to publish.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

Architecture and security converge on the highest-signal defect: pack_bundle() invokes legacy lockfile migration before honoring dry-run, so a hybrid project can rename apm.lock during apm pack --check-clean. The reported local reproduction observed legacy_exists=false and new_exists=true. The marketplace preservation regression is encouraging, but no passed evidence block was supplied, so it does not rebut this reproduced mutation.

Documentation, DevX, and growth also converge: the canonical release sequence still expects the read-only gate to create artifacts. Clean runners will lack build/*.zip, while reused workspaces could checksum and publish stale artifacts. The guide and CI translations should explicitly run validation first and artifact-producing pack second.

The missing integration assertion for JSON dry_run reporting is load-bearing because it protects a governed-by-policy CLI promise. It should accompany the lockfile fix. Dedicated check-clean rendering is worthwhile polish but ranks below correctness, release safety, and regression coverage.

Dissent. CLI Logging characterized execution as read-only, while Architecture and Supply Chain Security reproduced a legacy lockfile rename. The reproduced filesystem mutation outweighs the narrower marketplace-path assessment.

Aligned with: Secure by default: a validation command must preserve the lockfile that anchors dependency integrity. Governed by policy: check-clean must be observably read-only and defended by an integration assertion for its JSON contract. Pragmatic as npm: the release recipe should separate validation from artifact production with an explicit, copyable command sequence.

Growth signal. The release story is clear once corrected: run a safe read-only validation gate, then run a separate pack build.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	1	0	0	--check-clean still renames a legacy apm.lock during bundle production.
CLI Logging Expert	0	1	0	Check-clean ends with a misleading dry-run write preview after the drift outcome.
DevX UX Expert	0	1	0	The documented CI release flow now needs a separate artifact build.
Supply Chain Security Expert	1	0	0	Check-clean must preserve legacy lockfiles before it is called read-only.
OSS Growth Hacker	1	0	0	Fix the canonical release recipe before promoting check-clean as safely read-only.
Doc Writer	1	0	0	The canonical release guide now relies on a read-only command to create artifacts.
Test Coverage Expert	0	1	0	Check-clean JSON dry_run reporting remains untested without explicit --dry-run.

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

Top 4 follow-ups

1. [Supply Chain Security Expert] (blocking-severity) Prevent legacy lockfile migration across every dry-run bundle and plugin exporter path. -- The current ordering mutates the integrity source of truth despite the advertised read-only contract; add a hybrid-project whole-tree preservation regression.
2. [Doc Writer] (blocking-severity) Split the canonical CI sequence into a read-only gate followed by an artifact-producing pack command. -- The current recipe can leave clean runners without artifacts or allow stale workspace artifacts to be published.
3. [Test Coverage Expert] Assert that check-clean JSON reports dry_run: true without an explicit --dry-run flag. -- This missing integration guard protects a governed-by-policy user promise and would catch silent regression to raw-flag reporting.
4. [CLI Logging Expert] Give implicit check-clean dedicated result rendering. -- Suppressing the trailing write preview would keep the final output consistent with the command's read-only purpose while preserving previews for explicit dry-run.

Architecture

classDiagram
 class PackCommandModule
 class BuildOptions
 class BuildOrchestrator
 class ArtifactProducer
 class BundleProducer
 PackCommandModule ..> BuildOptions : creates
 BuildOptions ..> BuildOrchestrator : configures
 BuildOrchestrator *-- ArtifactProducer : delegates
 ArtifactProducer <|.. BundleProducer

Loading

flowchart TD
 A["apm pack --check-clean"] --> B["effective_dry_run = true"]
 B --> C["BuildOrchestrator"]
 C --> D["BundleProducer"]
 D --> E["migrate_lockfile_if_needed before dry-run guard"]
 E --> F["Legacy lockfile renamed"]

Loading

Recommendation

First eliminate legacy lockfile mutation and add whole-tree regression coverage, then repair the canonical release sequence and JSON contract test. Reassess after those user-facing promises are aligned.

---

Full per-persona findings

Python Architect

• [blocking] --check-clean can still mutate the legacy lockfile at src/apm_cli/bundle/packer.py:71
pack_bundle() calls migrate_lockfile_if_needed() before its dry-run guard. Suggested: Resolve legacy lockfiles without renaming during dry-run and add whole-tree preservation coverage.

CLI Logging Expert

• [recommended] Give check-clean its own result rendering at src/apm_cli/commands/pack.py:575
  Suppress producer previews for implicit check-clean while preserving explicit dry-run previews.

DevX UX Expert

• [recommended] Update the canonical release pipeline to build artifacts separately at docs/src/content/docs/producer/releasing-from-any-ci.md:22.

Supply Chain Security Expert

• [blocking] Check-clean still renames the supply-chain lockfile at src/apm_cli/commands/pack.py:390.

OSS Growth Hacker

• [blocking] The canonical release recipe no longer builds release artifacts at docs/src/content/docs/producer/releasing-from-any-ci.md:22.

Auth Expert -- inactive

Changes are limited to pack/docs/tests and no auth behavior is affected.

Doc Writer

• [blocking] Update the canonical CI sequence because check-clean no longer builds release artifacts at docs/src/content/docs/producer/releasing-from-any-ci.md:22.

Test Coverage Expert

• [recommended] The effective dry-run JSON contract lacks a regression assertion at src/apm_cli/commands/pack.py:534.
  Proof (missing at): tests/integration/test_pack_unified.py::TestCheckCleanJson::test_reports_effective_dry_run_without_explicit_flag -- proves: a check-clean JSON invocation reports that it is read-only.

Performance Expert -- inactive

The touched pack boolean propagation and docs/tests introduce no performance trigger.

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

[Daniel Meppiel (danielmeppiel)]

APM review panel - iteration 2

Recommendation: Fold two remaining authority bypasses before ship.

The first-pass findings are resolved: read-only pack/export flows preserve legacy lockfiles and emit truthful previews, release recipes are executable, JSON behavior is covered, and the new owner has behavioral plus static guardrails. The remaining convergence work is bounded:

1. Route LockFile.installed_paths_for_project through resolve_lockfile_path_for_read(..., read_only=True).
2. Route integration/hook_ownership.py::lockfile_dependency_identities through the same owner.
3. Expand AC37 and its architecture regression to cover those consumers.
4. Refresh the PR Scenario Evidence with the integration-tier proof.

No security, performance, CLI UX, logging, growth, or documentation blocker remains outside those items. This is advisory; the maintainer retains the merge decision.

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_with_followups

Closes #2727 cleanly and provably, but the new lockfile read-only authority needs branch-matrix tests and the pipeline behavior change needs a CHANGELOG migration line.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The panel converges on the core fix: effective_dry_run = dry_run or check_clean is the right minimal shape, and the regression trap proves that --check-clean detects drift without mutating the artifact. Two in-scope threads remain. The new resolve_lockfile_path_for_read authority leaves equivalent fallback logic in installed_paths_for_project and lacks functional branch-matrix coverage. Separately, existing one-command release pipelines need a clear migration note because --check-clean no longer produces artifacts.

Dissent. Python Architect rated the owner/test gap recommended while Test Coverage Expert rated the missing evidence blocking-severity. The missing functional proof deserves the stronger signal because it protects the same read-only contract this PR introduces.

Aligned with: Secure by default: check-clean cannot write regardless of caller intent. Governed by policy: the release gate no longer mutates the artifact it validates. Pragmatic as npm: users no longer need the tribal-knowledge --dry-run pairing.

Growth signal. This is a trust-restoring community fix worth a visible CHANGELOG line.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	2	2	Correct fix; finish canonical owner centralization and functional coverage.
CLI Logging Expert	0	1	1	Explain suppressed bundle output on the human path.
DevX UX Expert	0	2	1	Document pipeline migration and observable no-write behavior.
Supply Chain Security Expert	0	1	0	Integrity fix is sound; migration must be release-visible.
OSS Growth Hacker	0	1	1	Add the trust-restoring fix to CHANGELOG.
Doc Writer	0	2	2	Add migration guidance and verify the apm-action claim.
Test Coverage Expert	1	1	0	Add branch-matrix and bundle-render regression traps.
Performance Expert	0	0	0	No regression; avoiding writes is a net improvement.

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] (blocking-severity) Add fixture-backed branch-matrix tests for resolve_lockfile_path_for_read and the agent-plugin legacy-lockfile read path -- the new owner supports the release-gate fix and currently lacks direct functional evidence.
2. [Supply Chain Security Expert] Add a CHANGELOG migration entry for pipelines that used one call to both gate and produce -- those pipelines now need a separate apm pack call.
3. [Python Architect] Route installed_paths_for_project through the canonical resolver and extend AC37 -- two independent fallback implementations create split authority.
4. [CLI Logging Expert] Emit one explicit read-only notice when mixed bundle and marketplace output is suppressed -- otherwise the no-write behavior is invisible on the human path.
5. [Doc Writer] Verify or qualify the apm-action@v1 split-call claim -- official docs should not assert an external behavior without a known minimum version.

Architecture

flowchart TD
    A["apm pack --check-clean"] --> B["effective_dry_run = true"]
    B --> C["BuildOrchestrator"]
    C --> D["resolve_lockfile_path_for_read(read_only=true)"]
    D --> E["No lockfile migration"]
    C --> F["No bundle or marketplace writes"]
    F --> G["Compare unchanged marketplace artifact"]
    G --> H{"Drift?"}
    H -->|yes| I["Exit 4"]
    H -->|no| J["Exit 0"]

Loading

Recommendation

Fold the five in-scope follow-ups into this PR, then ship the read-only release-gate fix with its guardrails and migration guidance.

---

Full per-persona findings

Python Architect

• [recommended] The new resolver does not absorb installed_paths_for_project's equivalent canonical/legacy fallback decision.
• [recommended] Other nominally read-only commands still migrate legacy lockfiles; audit separately because they are outside this pack-focused change.
• [nit] AC37's literal source substring is formatting-fragile.
• [nit] The render-skip intent needs a comment.

CLI Logging Expert

• [recommended] Mixed bundle and marketplace projects get no bundle-side read-only notice.
• [nit] Explain why producer rendering is suppressed.

DevX UX Expert

• [recommended] Add a CHANGELOG migration entry for the new two-call release shape.
• [recommended] Make the whole-invocation no-write behavior observable in human output.
• [nit] Verify and document the minimum compatible apm-action version.

Supply Chain Security Expert

• [recommended] Call out the release-pipeline behavior change so artifact production cannot fail silently downstream.

OSS Growth Hacker

• [recommended] Add an Unreleased/Fixed entry for [BUG] apm pack --check-clean overwrites marketplace.json before checking drift #2727.
• [nit] Make the reason for two calls self-explanatory in pipeline snippets.

Auth Expert -- inactive

No authentication, token, credential, or host-resolution surface changed.

Doc Writer

• [recommended] Add a migration caution for old single-command pipelines.
• [recommended] Verify the external apm-action@v1 behavior claim or qualify it.
• [nit] Trim dated --check-versions pairing phrasing.
• [nit] The justified docs growth needs no further prose.

Test Coverage Expert

• [blocking] The new lockfile resolver branch matrix and agent-plugin legacy consumer path lack functional evidence.
• [recommended] The mixed bundle human-output suppression path lacks a regression trap.

Performance Expert

No findings.

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

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_with_followups

The core check-clean fix is now converged and CI-green; one direct packer regression trap and a few adjacent migration-doc details remain.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The final-head review confirms that --check-clean is read-only through the CLI, canonical lockfile resolver, and all three bundle exporters. The architecture registry, executable rule, mutation proof, and functional tests defend the new owner. The remaining substantive gap is focused: direct pack_bundle(fmt="apm", dry_run=True) lacks a legacy-lockfile preservation test. The documentation also presents adjacent apm-action v1.10.0 and v1.9.1 minimums without distinguishing their scopes.

Aligned with: Governed by policy: the release gate no longer mutates its evidence. Secure by default: no extra flag is needed for safe behavior. Pragmatic as npm: the documented two-call sequence is explicit and portable.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	2	Canonical owner and dual guardrail are complete.
CLI Logging Expert	0	0	0	The read-only notice is clear and tested.
DevX UX Expert	0	1	2	Reconcile adjacent apm-action version floors.
Supply Chain Security Expert	0	0	1	Keep both split calls on one pinned CLI version.
OSS Growth Hacker	0	0	1	Link the CHANGELOG entry to migration guidance.
Doc Writer	0	1	0	Disambiguate the v1.10.0 and v1.9.1 claims.
Test Coverage Expert	0	1	0	Add the direct legacy-lockfile packer test.
Performance Expert	0	0	1	No performance regression.

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

Top 4 follow-ups

1. [Test Coverage Expert] Add a direct pack_bundle(fmt="apm", dry_run=True) legacy-lockfile preservation test -- the third producer should have the same functional trap as agent-plugin and claude-plugin paths.
2. [DevX UX Expert + Doc Writer] Reconcile apm-action v1.10.0 and v1.9.1 claims -- readers need one unambiguous minimum for the split flow.
3. [Supply Chain Security Expert] State that both split pack calls use one pinned apm-cli version -- gate and production generation must match.
4. [OSS Growth Hacker] Link the CHANGELOG entry to the migration guidance -- the behavior change should be immediately actionable.

Architecture

flowchart TD
    A["apm pack --check-clean"] --> B["effective_dry_run = true"]
    B --> C["BuildOrchestrator"]
    C --> D["resolve_lockfile_path_for_read(read_only=true)"]
    D --> E["No legacy lockfile migration"]
    C --> F["No bundle, marketplace, or plugin-manifest writes"]
    F --> G["Compare unchanged marketplace artifact"]
    G --> H{"Drift?"}
    H -->|yes| I["Exit 4"]
    H -->|no| J["Exit 0"]

Loading

Recommendation

Fold the focused direct-packer test and documentation clarifications, then ship.

---

Full per-persona findings

Python Architect

• [nit] The canonical lockfile owner and dual guardrail are complete.
• [nit] The read-only notice could also name plugin-manifest output.

CLI Logging Expert

No findings.

DevX UX Expert

• [recommended] Reconcile the adjacent apm-action v1.10.0 and v1.9.1 minimums.
• [nit] The CLI reference could repeat the migration callout.
• [nit] Exit-code 0 wording could mention check-clean's no-write case.

Supply Chain Security Expert

• [nit] Both split calls should resolve to the same pinned apm-cli version.

OSS Growth Hacker

• [nit] The CHANGELOG entry could link directly to the migration guide.

Auth Expert -- inactive

No authentication, token, credential, or host-resolution surface changed.

Doc Writer

• [recommended] The two adjacent apm-action minimums read as contradictory.

Test Coverage Expert

• [recommended] The direct pack_bundle legacy-lockfile path lacks functional evidence.

Performance Expert

• [nit] The resolver performs the same bounded stat calls as before; no action needed.

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

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_with_followups

The read-only check-clean contract is converged across production paths; the default Claude/plugin exporter needs the same direct legacy-lockfile regression trap as its two siblings.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

Python Architect and Test Coverage Expert independently found the one remaining asymmetry. The architecture rule statically covers all three lockfile-reading producers, while direct runtime tests cover only the Agent Plugin and legacy APM producer paths. Add the matching export_plugin_bundle(..., dry_run=True) test for the default Claude/plugin path. All other lenses are clean or nit-only, and CI is green.

Aligned with: Governed by policy: the static owner rule already covers every consumer. Secure by default: one final runtime trap will make the no-migration promise symmetric across bundle formats. Multi-harness support: every exporter should carry the same proof.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	1	1	Add the default exporter legacy-lockfile trap.
CLI Logging Expert	0	0	2	Only minor vocabulary/no-op polish remains.
DevX UX Expert	0	0	0	No remaining concerns.
Supply Chain Security Expert	0	0	0	No remaining concerns.
OSS Growth Hacker	0	0	0	Migration link is complete.
Doc Writer	0	0	1	Only an unrelated main-branch nit.
Test Coverage Expert	0	1	0	Default exporter lacks its sibling runtime proof.
Performance Expert	0	0	0	No performance concern.

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

Top 4 follow-ups

1. [Test Coverage Expert + Python Architect] Add tests/unit/test_plugin_exporter.py::test_dry_run_reads_legacy_lockfile_without_migration -- complete the three-producer functional matrix.
2. [CLI Logging Expert] Use the dry-run notice vocabulary for implicit check-clean read-only mode -- keep read-only signaling consistent.
3. [CLI Logging Expert] Avoid the extra read-only notice when no marketplace gate exists -- the existing skip message is sufficient.
4. [Python Architect] Bind check_clean and not dry_run once -- keep the adjacent notice and render-skip conditions aligned.

Architecture

flowchart TD
    A["effective_dry_run"] --> B["resolve_lockfile_path_for_read"]
    B --> C["legacy APM packer"]
    B --> D["Claude/plugin exporter"]
    B --> E["Agent Plugin exporter"]
    C --> F["runtime regression trap"]
    D --> G["missing sibling trap"]
    E --> H["runtime regression trap"]
    B --> I["registered AST owner guard"]

Loading

Recommendation

Fold the missing default-exporter regression trap and adjacent CLI polish, then ship.

---

Full per-persona findings

Python Architect

• [recommended] The default Claude/plugin exporter lacks the legacy-lockfile dry-run test carried by its sibling producers.
• [nit] Bind the repeated check-clean read-only condition once.

CLI Logging Expert

• [nit] Route the notice through dry-run vocabulary.
• [nit] Suppress the extra notice when no marketplace block exists.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

No authentication, token, credential, or host-resolution surface changed.

Doc Writer

The only nit concerns a compile guide change inherited from main and is outside this PR's pack scope.

Test Coverage Expert

• [recommended] export_plugin_bundle has no direct legacy-lockfile dry-run regression trap.

Performance Expert

No findings.

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

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_with_followups

The read-only check-clean fix is architecture-, docs-, security-, UX-, and performance-clean; one explicit combined-flags output test remains.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

Eight panel lenses returned no findings after the prior folds. Test Coverage Expert identified one narrow gap: a bundle-plus-marketplace project has implicit check-clean output coverage and marketplace-only combined-flag coverage, but no test pins the explicit --check-clean --dry-run --offline preview shape or proves the implicit notice is absent. The production condition is correct and CI is green; this is a small in-scope regression trap.

Aligned with: Governed by policy: check-clean cannot mutate the artifact it validates. OSS community driven: #2727 is credited and migration guidance is linked. Pragmatic as npm: the gate now verifies without side effects.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	Canonical owner and producer matrix are complete.
CLI Logging Expert	0	0	0	Notice semantics are converged.
DevX UX Expert	0	0	0	No remaining concerns.
Supply Chain Security Expert	0	0	0	No remaining concerns.
OSS Growth Hacker	0	0	0	Migration story is complete.
Doc Writer	0	0	0	PR-scoped docs are consistent.
Test Coverage Expert	0	1	0	Pin explicit combined-flags preview output.
Performance Expert	0	0	0	No performance concern.

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

Top 1 follow-up

1. [Test Coverage Expert] Add a bundle-plus-marketplace --check-clean --dry-run --offline test -- assert full dry-run preview output and no implicit check-clean notice.

Architecture

flowchart TD
    A["--check-clean"] --> B{"explicit --dry-run?"}
    B -->|no| C["single implicit read-only notice"]
    B -->|yes| D["full dry-run producer previews"]
    C --> E["no pack writes"]
    D --> E

Loading

Recommendation

Fold the last combined-flags regression trap, then ship.

---

Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

No authentication, token, credential, or host-resolution surface changed.

Doc Writer

No findings.

Test Coverage Expert

• [recommended] Explicit combined check-clean and dry-run output lacks a bundle-plus-marketplace regression trap.

Performance Expert

No findings.

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

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_with_followups

The read-only fix is converged except for one observable bundle-only shape: no marketplace block currently suppresses both bundle rendering and the read-only notice.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

CLI Logging Expert and Test Coverage Expert independently found the same line pair. implicit_check_clean_dry_run always suppresses producer rendering, but the notice is conditioned on gate_config is not None. A bundle-only project therefore exits 0 after withholding its bundle without saying why. The existing test currently asserts that silence. All other lenses are clean and CI is green.

Aligned with: Pragmatic as npm: an exit-0 gate should explain suppressed artifacts. Governed by policy: the read-only behavior is correct; only its observability needs the final fold.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	Architecture converged.
CLI Logging Expert	0	1	0	Bundle-only suppression needs its notice.
DevX UX Expert	0	0	0	No remaining concern.
Supply Chain Security Expert	0	0	0	No remaining concern.
OSS Growth Hacker	0	0	0	Migration story complete.
Doc Writer	0	0	0	Docs consistent.
Test Coverage Expert	0	1	0	Correct the bundle-only assertion.
Performance Expert	0	0	0	No performance concern.

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

Top 1 follow-up

1. [CLI Logging Expert + Test Coverage Expert] Emit the read-only notice whenever implicit check-clean suppresses producer rendering, including bundle-only projects, and assert that contract in test_skip_when_no_marketplace_block.

Recommendation

Fold the one-line notice guard and test correction, then ship.

---

Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

• [recommended] Bundle-only implicit check-clean suppresses output without a read-only notice.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

No authentication, token, credential, or host-resolution surface changed.

Doc Writer

No findings.

Test Coverage Expert

• [recommended] The existing no-marketplace test asserts the wrong silent-output contract.

Performance Expert

No findings.

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

[Daniel Meppiel (danielmeppiel)]

APM Review Panel: ship_now

PR #2730 now makes apm pack --check-clean read-only across every pack producer, with symmetric runtime traps, a registered owner guard, migration guidance, and green CI.

cc Sergio Sisternes (@sergio-sisternes-epam) -- a fresh advisory pass is ready for your review.

The terminal panel converged after every in-scope finding was folded. The final bundle-only observability gap is closed at 54ede54d24: implicit check-clean always emits the canonical dry-run notice when it suppresses pack output, including projects without a marketplace block. Reintroducing the old gate_config restriction makes the exact regression test fail. All other panel lenses are clear, the owner-evidence gate is semantically verified, and required CI is green.

Aligned with: Secure by default: check-clean cannot mutate or silently suppress pack output. Governed by policy: one registered owner and mutation-tested architecture rule enforce lockfile reads. Pragmatic as npm: the release gate now verifies without side effects or tribal-knowledge flags.

Growth signal. The CHANGELOG credits #2727 and links directly to the two-call release-pipeline migration.

Panel summary

Persona	B	R	N	Takeaway
Python Architect	0	0	0	Single-owner architecture and dual guardrail are complete.
CLI Logging Expert	0	0	0	Every suppressed-output shape has a tested notice.
DevX UX Expert	0	0	0	CLI, docs, and migration contracts agree.
Supply Chain Security Expert	0	0	0	Release-gate integrity is preserved.
OSS Growth Hacker	0	0	0	Migration story and issue credit are complete.
Doc Writer	0	0	0	PR-scoped documentation is consistent.
Test Coverage Expert	0	0	0	Resolver, producers, notices, and flag interactions are covered.
Performance Expert	0	0	0	Read-only paths avoid writes without hot-path regressions.

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

Architecture

flowchart TD
    A["apm pack --check-clean"] --> B["effective_dry_run = true"]
    B --> C["BuildOrchestrator"]
    C --> D["resolve_lockfile_path_for_read(read_only=true)"]
    D --> E["No legacy lockfile migration"]
    C --> F["No pack output writes"]
    F --> G["Compare unchanged marketplace artifact"]
    G --> H{"Drift?"}
    H -->|yes| I["Exit 4"]
    H -->|no| J["Exit 0"]
    D -. registered owner rule .-> K["Architecture linter"]

Loading

Recommendation

Ship as-is. The panel's final finding is folded and mutation-verified, all other panelists are clear, exact-head tests and lint pass, and required CI is green at 54ede54d24b6bff22d7af06bee79cdc0d0218423.

Folded in this run

• (panel) Centralized every read-only lockfile consumer behind one resolver -- resolved in 8af3e697b9.
• (panel) Registered the lockfile owner in the sharded architecture linter with mutation coverage -- resolved in 8af3e697b9.
• (panel) Added resolver branch-matrix and Agent Plugin legacy-lockfile tests -- resolved in 404fe74fb6.
• (panel) Added an observable read-only notice for suppressed check-clean outputs -- resolved in 2de47561ed.
• (panel) Documented the two-call release migration and action version floor -- resolved in 442c949c24.
• (panel) Updated the frozen architecture rule inventory found by CI -- resolved in 112f670140.
• (panel) Added the direct legacy APM packer legacy-lockfile trap -- resolved in 21d0405aa6.
• (panel) Reconciled action version floors and pinned split-call generator semantics -- resolved in 08b8096d37.
• (panel) Clarified complete pack-output suppression and exit-code wording -- resolved in 43834eb896.
• (panel) Added the default Claude/plugin exporter legacy-lockfile trap -- resolved in b31ed3a57a.
• (panel) Used canonical dry-run notice vocabulary and one named implicit condition -- resolved in 47a7eb5e32.
• (panel) Pinned explicit combined check-clean and dry-run preview output -- resolved in a0915e1118.
• (panel) Explained bundle-only check-clean suppression without a marketplace block -- resolved in 54ede54d24.

Regression-trap evidence (mutation-break gate)

• test_detects_drift_without_mutating_existing_output -- removed effective_dry_run = dry_run or check_clean; test FAILED as expected; guard restored.
• TestResolveLockfilePathForRead::test_read_only_returns_legacy_path_without_migrating -- disabled the if read_only branch; test FAILED as expected; guard restored.
• test_agent_bundle_dry_run_reads_legacy_lockfile_without_migration -- disabled the read-only resolver branch; test FAILED as expected; guard restored.
• TestPackBundle::test_apm_format_dry_run_reads_legacy_lockfile_without_migration -- replaced read_only=dry_run; test FAILED as expected; guard restored.
• TestExportPluginBundle::test_dry_run_reads_legacy_lockfile_without_migration -- replaced read_only=dry_run; test FAILED as expected; guard restored.
• test_reports_suppressed_bundle_output_as_read_only -- replaced dry_run_notice; test FAILED as expected; guard restored.
• test_explicit_dry_run_keeps_full_bundle_and_marketplace_preview -- removed and not dry_run; test FAILED as expected; guard restored.
• test_skip_when_no_marketplace_block -- restored the old gate_config notice restriction; test FAILED as expected; guard restored.
• test_lockfile_read_rule_rejects_disabled_read_only_guard -- disabled the registered owner guard; architecture test FAILED as expected; guard restored.

Lint contract

uv run --frozen --extra dev ruff check src/ tests/ scripts/lint_architecture_boundaries.py scripts/architecture_linter/ and the matching format check passed. Pylint R0801 rated 10.00/10; auth-signal and architecture-boundary lints are clean.

CI

Final CI run 33476724854: all required checks passed after 1 CI fix iteration.

Mergeability status

PR	head SHA	CEO stance	iters	folds	defers	Copilot rounds	CI	mergeable	mergeStateStatus	notes
#2730	54ede54	ship_now	4	13	0	2	green	MERGEABLE	BLOCKED	pending required review

Convergence

4 outer iterations; 2 Copilot rounds with no inline findings. Final panel stance: ship_now.

Ready for maintainer review.

---

Full per-persona findings

Python Architect

No findings.

CLI Logging Expert

No findings.

DevX UX Expert

No findings.

Supply Chain Security Expert

No findings.

OSS Growth Hacker

No findings.

Auth Expert -- inactive

No authentication, token, credential, or host-resolution surface changed.

Doc Writer

No findings.

Test Coverage Expert

No findings after the bundle-only notice trap was folded and mutation-verified.

Performance Expert

No findings.

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