Skip to content

feat(cli): apm experimental - feature-flag registry with list/enable/disable/reset#845

Closed
sergio-sisternes-epam wants to merge 2 commits intomicrosoft:mainfrom
sergio-sisternes-epam:feat/experimental-feature-flags-clean
Closed

feat(cli): apm experimental - feature-flag registry with list/enable/disable/reset#845
sergio-sisternes-epam wants to merge 2 commits intomicrosoft:mainfrom
sergio-sisternes-epam:feat/experimental-feature-flags-clean

Conversation

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator

Summary

Introduces apm experimental — a first-class feature-flag subsystem that lets APM ship opt-in changes behind named flags so early adopters can try new behaviour without branching releases.

Ships with one worked example: verbose-version, which extends apm --version to also print Python version, platform, and install path.

Why

Today we have no safe way to iterate on potentially disruptive changes in main. Contributors either ship full switches (risk breaking users) or carry long-lived branches (rot risk, no real-user signal). A lightweight registry + O(1) lookup gives us a third path: merge the code, gate it behind a flag, collect signal from opt-in users, and graduate to default when proven.

What's in this PR

New command group

apm experimental                       # default: list
apm experimental list [--enabled|--disabled]
apm experimental enable <name>
apm experimental disable <name>
apm experimental reset [<name>] [--yes]
  • Accepts both kebab-case and snake_case on input; displays kebab-case.
  • difflib-powered "Did you mean?" on unknown flag names.
  • reset with no argument clears all overrides (interactive confirmation; --yes skips it for CI).
  • Stale keys from graduated/removed flags are surfaced in list and cleaned up by reset.

Core registry

  • src/apm_cli/core/experimental.py — frozen ExperimentalFlag dataclass, static FLAGS registry, module-level is_enabled(name).
  • Lookup cost: two dict reads on top of the already-cached apm_cli.config._config_cache. No I/O after first load, no extra caches, no singleton. Safe for use in hot paths.
  • is_enabled("typo") raises ValueError — fail loud on developer typos in shipped code.
  • Loader rejects non-bool override values (fails closed to registry default) to prevent truthy-string injection from a hand-edited ~/.apm/config.json.

Storage

Overrides persist under a new experimental key in ~/.apm/config.json:

{
  "default_client": "vscode",
  "experimental": {
    "verbose_version": true
  }
}

Only overrides are written. Unset flags fall through to the registry default (which is always false — asserted by an invariant test).

Worked example: verbose-version

When enabled, apm --version appends Python version, platform, and install path. The gated block is wrapped in try/except Exception: pass so a flag-subsystem bug can never break --version.

Security invariant

Experimental flags MUST NOT gate security-critical behaviour (content scanning, path validation, lockfile integrity, token handling, MCP trust checks). Documented in docs/src/content/docs/reference/experimental.md and CONTRIBUTING.md. The silent-ignore policy for stale keys is safe only because of this invariant.

How to add a flag (for contributors)

See the new "How to add an experimental feature flag" section in CONTRIBUTING.md. TL;DR:

  1. Add an ExperimentalFlag(...) entry to FLAGS in src/apm_cli/core/experimental.py.
  2. At the call site, import is_enabled at function scope (not module top-level — avoids config I/O at import time) and branch on it.
  3. Document the flag in docs/src/content/docs/reference/experimental.md.
  4. Add a test that exercises both flag states.

Testing

  • 71+ new targeted tests across tests/unit/core/test_experimental.py, tests/unit/commands/test_experimental_command.py, and extended tests/unit/commands/test_helpers_version.py.
  • Full unit suite green: uv run pytest tests/unit tests/test_console.py -> 4,842 passed.
  • Covers: registry invariants (name==key, default is False, printable-ASCII descriptions), round-trip enable/disable, stale-key diagnostic, unknown-flag error + difflib suggestions, reset confirmation (accept/decline/--yes), kebab/snake input parity, plain-text fallback when Rich is unavailable, and that apm --version default output is unchanged when the flag is off.

Documentation

  • New: docs/src/content/docs/reference/experimental.md — user-facing reference (commands, storage schema, graduation lifecycle, security boundary).
  • Updated: CONTRIBUTING.md — "How to add an experimental feature flag" recipe.
  • Updated: CHANGELOG.mdUnreleased > Added entry.
  • Updated: docs/src/content/docs/reference/cli-commands.md — new command entry.
  • Skill sync: packages/apm-guide/.apm/skills/apm-usage/commands.md — new command entry.
  • README: deliberately NOT updated at launch — we surface this in README only once we have >=3 meaningful flags shipping under it.

Review panel

Pre-open review via the apm-review-panel skill: python-architect, cli-logging-expert, devx-ux-expert, supply-chain-security-expert, oss-growth-hacker, and apm-ceo. All 10 ratified pre-PR fixes have been applied. CEO verdict: SHIP-WITH-FIXES — all fixes in; deferred follow-ups tracked separately.

Out of scope (deliberate)

  • Project-level flags in apm.yml.
  • Environment-variable overrides (APM_EXPERIMENTAL_*).
  • Remote / rollout-percentage flags.
  • Telemetry on flag usage.

Backward compatibility

Zero impact for existing users. Users with no experimental key in config.json see identical behaviour to today. apm --version output is unchanged unless a user opts in.

…disable/reset

Introduces the `apm experimental` command group and a lightweight feature-flag
registry so APM can ship opt-in behaviour behind named flags, gather signal
from early adopters, and graduate to default when proven.

Ships with one worked example: `verbose-version`, which extends
`apm --version` to also print Python version, platform, and install path.

Highlights:
- O(1) flag lookup on top of the cached config (no I/O after first load)
- `is_enabled("typo")` raises ValueError (fail loud in code paths)
- `apm experimental enable <typo>` surfaces difflib "Did you mean?" suggestion
- Loader rejects non-bool overrides; falls back to registry default
- Security invariant: experimental flags MUST NOT gate security-critical paths
- 71+ targeted tests; full unit suite green (4,842 passed)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 22, 2026 14:19
@sergio-sisternes-epam sergio-sisternes-epam added documentation Improvements or additions to documentation enhancement New feature or request cli labels Apr 22, 2026
@sergio-sisternes-epam sergio-sisternes-epam added the panel-review Trigger the apm-review-panel gh-aw workflow label Apr 22, 2026
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

Adds a first-class experimental feature-flag subsystem to the APM CLI, including a new apm experimental command group and a worked example flag (verbose-version) that augments apm --version output when enabled.

Changes:

  • Introduces a static experimental flag registry (FLAGS) with config-backed overrides and helpers (is_enabled, enable/disable/reset, stale-key detection).
  • Adds apm experimental CLI group with list/enable/disable/reset and associated unit tests.
  • Updates version output behind an experimental gate and adds docs/changelog entries for the new command.

Reviewed changes

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

Show a summary per file
File Description
src/apm_cli/core/experimental.py Implements the experimental flag registry, config integration, and mutation/query helpers.
src/apm_cli/commands/experimental.py New CLI command group to manage experimental flags.
src/apm_cli/commands/_helpers.py Gates additional apm --version details behind verbose_version.
src/apm_cli/cli.py Registers the new experimental command group with the top-level CLI.
tests/unit/core/test_experimental.py Unit coverage for registry behavior, config parsing, overrides, and invariants.
tests/unit/commands/test_experimental_command.py End-to-end CLI tests for list/enable/disable/reset flows and UX.
tests/unit/commands/test_helpers_version.py Tests the verbose_version behavior in apm --version.
docs/src/content/docs/reference/experimental.md New user-facing reference page for apm experimental.
docs/src/content/docs/reference/cli-commands.md Adds apm experimental to the CLI reference index.
packages/apm-guide/.apm/skills/apm-usage/commands.md Syncs the command into the shipped skill docs.
CONTRIBUTING.md Adds contributor guidance for adding new experimental flags.
CHANGELOG.md Adds an Unreleased changelog entry for the new command group.

Comment thread src/apm_cli/commands/experimental.py
Comment thread src/apm_cli/commands/experimental.py
Comment thread src/apm_cli/commands/experimental.py Outdated
Comment thread src/apm_cli/core/experimental.py
Comment thread docs/src/content/docs/reference/experimental.md Outdated
Comment thread tests/unit/commands/test_helpers_version.py Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CHANGELOG.md Outdated
@github-actions
Copy link
Copy Markdown

CLI Logging & Output UX Review — apm experimental

Summary

Clean implementation. The command correctly routes through CommandLogger for all business-logic output, reserves direct _rich_echo for the Rich-unavailable table fallback, and uses the defensive try/except pattern for the --version gating. Five findings, none blocking.


F-1 · "Already enabled/disabled" uses wrong semantic level — progress instead of warning

SUGGESTED

# experimental.py:199
logger.progress(f"{_display_name(normalised)} is already enabled.")
# experimental.py:228
logger.progress(f"{_display_name(normalised)} is already disabled.")

logger.progress() renders as blue [i] — the same styling used for neutral informational messages like "Experimental features let you try new behaviour...". But "already enabled" is a no-op outcome: the user asked to change something and nothing changed. Per the traffic-light rule, that's yellow territory — "should know" the action had no effect.

Fix: Switch to logger.warning():

logger.warning(f"{_display_name(normalised)} is already enabled.")

This gives [!] in yellow — visually distinct from the success path ([*] green) and the error path ([x] red), making the three outcomes instantly distinguishable at a glance.


F-2 · Reset confirmation dialog bypasses CommandLogger — uses raw click.echo

SUGGESTED

# experimental.py:292-294
click.echo(f"This will reset {total} experimental {noun} to {pronoun} {default_word}:")
for line in lines:
    click.echo(line)

Every other output line in this file goes through CommandLogger. These two click.echo() calls are the only exceptions. They produce unstyled plaintext, breaking visual consistency with the rest of the command's output.

Fix: Route through the logger:

logger.progress(f"This will reset {total} experimental {noun} to {pronoun} {default_word}:")
for line in lines:
    logger.progress(line)

This keeps the [i] prefix and blue styling consistent. The confirmation prompt itself (Confirm.ask / click.confirm) can stay raw — prompts are interactive UI, not status messages.


F-3 · CommandLogger (not InstallLogger) is correct here

NOTE — no action needed.

The command imports CommandLogger and instantiates a fresh one per subcommand (lines 150, 185, 215, 242). This is the right choice. InstallLogger is for the validation→resolution→download→summary lifecycle. apm experimental is a metadata/config command — it has no phases, no package count, no diagnostics to collect. Using the base CommandLogger gives the correct semantic level (start/progress/success/error) without the install noise.

The only direct _rich_echo call (line 95-96) is inside the Rich-unavailable fallback in _build_table — an internal rendering concern, not a lifecycle message. Appropriate.


F-4 · --version gating: defensive try/except is the correct pattern

NOTE — no action needed.

# _helpers.py, print_version callback
try:
    from ..core.experimental import is_enabled
    if is_enabled("verbose_version"):
        # ... _rich_echo dim lines ...
except Exception:
    pass  # Never let experimental flag logic break --version

Three things are right here:

  1. Broad except Exception — correct. A config-file parse error, a missing key, or a future bug in the experimental module must never break --version. This is the one place where a blanket catch is justified.
  2. Direct _rich_echo (not logger) — correct. This is a Click --version eager callback, not a command lifecycle. No CommandLogger instance exists in this scope, and creating one just for 3 dim lines would be over-engineering.
  3. color="dim" with aligned labels — correct. The verbose-version lines are supplementary detail, not status messages. Dim styling + 2-space indent visually subordinates them to the primary version string.

F-5 · Table design is clean — passes the "So What?" test

NOTE — no action needed.

The actual table has 3 columns: Flag · Status · Description. The PR summary mentioned "Default" and "Hint" columns, but neither shipped — good editorial judgment. With only one flag today (verbose_version, default always false), a "Default" column would be 100% "off" and a "Hint" column would duplicate post-enable output. Three columns is the right ceiling for a single-flag registry.

The contextual footer tip is smart UX: when nothing is enabled it shows enable <name>, when something is enabled it shows disable <name> to revert. This passes the "include the fix" rule — the user always sees the next action.


Verdict: APPROVE

The output architecture is sound — CommandLogger for command lifecycle, direct _rich_echo only where appropriate, proper defensive wrapping for --version, and a clean table design. F-1 and F-2 are polish items that would tighten the traffic-light consistency; neither is blocking.

Generated by PR Review Panel for issue #845 · ● 13.7M ·

- Fix CHANGELOG placeholder (#TBD -> microsoft#845)
- Use typing.Any in test_helpers_version.py
- Align CONTRIBUTING.md recipe code with its function-scope-import prose
- Correct singular/plural "default(s)" in reset doc example
- Pass symbol="check" to logger.success on enable/disable/reset so they render [+]
- Switch already-enabled/disabled no-op messages to logger.warning ([!]) per traffic-light rule
- Route reset confirmation preamble through CommandLogger (was raw click.echo)
- Verified: removing ignore_unknown_options breaks `reset --yes`; keeping both flags

Addresses Copilot review + PR Review Panel CLI Logging UX findings.

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

Response to Panel Review — All actionable items addressed in 97c6dd6

Thanks for the thorough CLI UX pass. Both SUGGESTED items applied, both NOTE items acknowledged unchanged. Follow-up commit 97c6dd6 on this PR.

F-1 — Applied ✅

logger.progress(...)logger.warning(...) for "already enabled" / "already disabled" at experimental.py:199, 228. No-op outcomes now render [!] (yellow) — visually distinct from the [+] success path and the [x] error path. Verified via smoke test:

$ apm experimental enable verbose-version
[+] Enabled 'verbose-version'.
$ apm experimental enable verbose-version
[!] 'verbose-version' is already enabled.

F-2 — Applied ✅

Reset confirmation preamble at experimental.py:292-294 re-routed through CommandLogger:

logger.progress(f"This will reset {total} experimental {noun} to {pronoun} {default_word}:")
for line in lines:
    logger.progress(line)

The Confirm.ask(...) prompt itself remains raw, matching your guidance that interactive UI ≠ status messages.

F-3, F-4, F-5 — Acknowledged, no action ✅

Your reasoning on CommandLogger (correct choice over InstallLogger), the defensive try/except Exception: pass in the --version callback, and the three-column table design all tracked. Kept as-is.

Adjacent fix applied while in the file

Also added symbol="check" to every logger.success(...) call in experimental.py (flagged by Copilot independently). Without it, success messages were rendering the default sparkles symbol ([*]) instead of the documented [+]. With your F-1 fix stacked on top, the full traffic-light palette is now restored across the command:

Outcome Symbol Colour Method
Success [+] green logger.success(symbol="check")
No-op warning [!] yellow logger.warning(...)
Neutral info [i] blue logger.progress(...)
Error [x] red logger.error(...)

Validation

  • Full unit suite: 5258 passed in 12.69s
  • Targeted: tests/unit/commands/test_experimental_command.py (71 passed) — assertions updated to expect [!] on no-op paths and [+] on success paths

Appreciate the panel verdict. Ready for another pass if anything in the follow-up commit regressed the UX.

@sergio-sisternes-epam
Copy link
Copy Markdown
Collaborator Author

Re-opened as #849 with the branch moved from my fork (sergio-sisternes-epam/apm) to microsoft/apm so other maintainers can push to it directly. Same commits (dd41d5c + 97c6dd6), same diff, same CHANGELOG entry.

All prior review history (Copilot 8 threads resolved + Panel CLI UX review approved) stays here on #845 for audit. Please continue review on #849.

auto-merge was automatically disabled April 22, 2026 17:09

Pull request was closed

danielmeppiel added a commit that referenced this pull request Apr 23, 2026
…disable/reset (#849)

* feat(cli): apm experimental - feature-flag registry with list/enable/disable/reset

Introduces the `apm experimental` command group and a lightweight feature-flag
registry so APM can ship opt-in behaviour behind named flags, gather signal
from early adopters, and graduate to default when proven.

Ships with one worked example: `verbose-version`, which extends
`apm --version` to also print Python version, platform, and install path.

Highlights:
- O(1) flag lookup on top of the cached config (no I/O after first load)
- `is_enabled("typo")` raises ValueError (fail loud in code paths)
- `apm experimental enable <typo>` surfaces difflib "Did you mean?" suggestion
- Loader rejects non-bool overrides; falls back to registry default
- Security invariant: experimental flags MUST NOT gate security-critical paths
- 71+ targeted tests; full unit suite green (4,842 passed)

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

* fix(experimental): address PR #845 review feedback

- Fix CHANGELOG placeholder (#TBD -> #845)
- Use typing.Any in test_helpers_version.py
- Align CONTRIBUTING.md recipe code with its function-scope-import prose
- Correct singular/plural "default(s)" in reset doc example
- Pass symbol="check" to logger.success on enable/disable/reset so they render [+]
- Switch already-enabled/disabled no-op messages to logger.warning ([!]) per traffic-light rule
- Route reset confirmation preamble through CommandLogger (was raw click.echo)
- Verified: removing ignore_unknown_options breaks `reset --yes`; keeping both flags

Addresses Copilot review + PR Review Panel CLI Logging UX findings.

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

* fix(experimental): apply PR #849 review feedback

- Reset now cleans malformed overrides of registered flags (#849, Copilot)
- --verbose works after subcommand name (#849, Panel P-UX-1)
- Promote name-handling helpers to public API (#849, Panel P-ARCH-1)
- Deduplicate flag-name normalisation (#849, Panel P-ARCH-2)
- reset() returns removal count instead of list (#849, Panel P-ARCH-3)
- Demote list preamble to --verbose only (#849, Panel P-LOG-1)
- Add --json output to list (#849, Panel P-UX-2)

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

* docs(experimental): document --json, per-subcommand --verbose, reset fix

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

---------

Co-authored-by: Sergio Sisternes <sergio.sisternes@epam.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Daniel Meppiel <51440732+danielmeppiel@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli documentation Improvements or additions to documentation enhancement New feature or request panel-review Trigger the apm-review-panel gh-aw workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants