Skip to content

refactor(mode): replace config-based mode setting with runtime detection#196

Open
ayeshurun wants to merge 22 commits intomicrosoft:mainfrom
ayeshurun:copilot/remove-mode-settings
Open

refactor(mode): replace config-based mode setting with runtime detection#196
ayeshurun wants to merge 22 commits intomicrosoft:mainfrom
ayeshurun:copilot/remove-mode-settings

Conversation

@ayeshurun
Copy link
Copy Markdown
Collaborator

@ayeshurun ayeshurun commented Mar 22, 2026

📥 Pull Request

✨ Description of new changes

The CLI mode (interactive/command_line) was persisted in ~/.config/fab/config.json and required fab config set mode interactive to enter REPL. Mode is now determined purely at runtime - no config entry, no explicit switching.

What changed

  • Runtime detection - Context singleton exposes set_runtime_mode()/get_runtime_mode(). InteractiveCLI sets FAB_MODE_INTERACTIVE on start and resets it on exit. Everything else reads get_runtime_mode() instead of the config file.

  • Config migration - init_defaults() silently removes the legacy mode key (and fab_mode) from existing config.json on first run.

  • Backward compat - config get mode and config set mode <value> now print a deprecation warning and return cleanly instead of erroring.

  • Context persistence - _should_use_context_file() now calls get_runtime_mode() instead of fab_state_config.get_config(FAB_MODE). Context files are still skipped in REPL (in-memory context is sufficient).

  • main.py - removed the post-login block that auto-started REPL when mode=interactive was stored in config.

  • Docs - modes.md rewritten to reflect the new runtime model; mode row removed from settings.md.

Entry points now

Invocation Mode
fab REPL (interactive)
fab <command> Command-line

Original prompt

I want to get rid of the mode settings, where I will only have the command line and REPL modes, but not through settings. If I'm in REPL mode I want know that I'm in the context because there are some behaviors depend on that (today it is called interactive mode). I want you to also update the docs and support backward compatibility.

ayeshurun and others added 11 commits March 17, 2026 12:14
This script benchmarks the startup performance of the CLI by measuring module import times, CLI invocation times, and heavy dependency loading. It allows comparisons against a baseline branch or tag.
Co-authored-by: ayeshurun <98805507+ayeshurun@users.noreply.github.com>
- Move runtime mode into Context class (singleton instance methods)
- Graduated deprecation: 'config set mode interactive' still launches REPL with warning
- Graduated deprecation: 'config get mode' returns runtime mode with warning
- Add regression tests: runtime mode (6), config deprecation (7), init defaults (4),
  MAP removal (6), session-per-request (3), context persistence mock updates (10)
- Consolidate test files into test_fab_context.py, test_config.py, test_fab_state_config.py

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolved conflicts in main.py and test_fab_state_config.py.
Re-removed ItemType.MAP, _shared_session/_get_session, and MAP references
in command_support.yaml, fab_item.py, and test conftest that upstream
re-introduced.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ayeshurun ayeshurun requested a review from a team as a code owner March 22, 2026 10:36
Copy link
Copy Markdown
Collaborator Author

@ayeshurun ayeshurun left a comment

Choose a reason for hiding this comment

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

@copilot Address all comments

Alon Yeshurun and others added 2 commits March 29, 2026 09:55
…ntions

- Use mock_repl fixture in TestConfig instead of inline with-patch
- Add _success suffix to all TestRuntimeMode test names
- Fix test_init_defaults_no_mode_key_succeeds -> _success

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove duplicate mode deprecation tests from TestConfig (kept in
  TestConfigModeDeprecated which has better coverage)
- Remove mock_repl fixture from TestConfig (no longer needed)
- Drop mock_questionary_print from tests that only need mock_print_warning
  and mock_repl
- Add actual assertions to previously no-op tests (mock_print_done,
  mock_questionary_print)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 29, 2026 09:20
Copy link
Copy Markdown

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

Refactors Fabric CLI mode handling to remove the persisted mode setting and instead determine REPL vs command-line behavior at runtime, while keeping backward compatibility for config get/set mode and updating docs/tests accordingly.

Changes:

  • Introduces a runtime-mode flag on the Context singleton and updates REPL startup/exit to toggle it.
  • Removes mode from persisted config keys/defaults and adds a config migration to delete legacy mode.
  • Deprecates config get/set mode (warn + no-op / runtime value), updates docs and test suites.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/fabric_cli/core/fab_context.py Adds runtime mode storage/accessors; switches context persistence gating to runtime mode.
src/fabric_cli/core/fab_interactive.py Sets runtime mode when entering REPL and resets on exit.
src/fabric_cli/core/fab_constant.py Removes mode from config-valid-values and defaults.
src/fabric_cli/core/fab_state_config.py Adds migration logic to remove legacy mode from existing config.
src/fabric_cli/commands/config/fab_config_set.py Deprecates config set mode and optionally launches REPL for interactive.
src/fabric_cli/commands/config/fab_config_get.py Deprecates config get mode and returns runtime mode.
src/fabric_cli/main.py Removes post-login auto-REPL behavior based on stored config mode.
src/fabric_cli/parsers/fab_config_parser.py Updates examples away from mode.
tests/conftest.py Resets Context runtime mode between tests to prevent leakage.
tests/test_core/test_fab_context.py Adds unit tests for runtime mode behavior and config removal assertions.
tests/test_core/test_context_persistence.py Updates persistence tests to rely on runtime mode instead of config mode.
tests/test_core/test_fab_state_config.py Adds migration tests for removing legacy mode in init_defaults().
tests/test_core/test_fab_config_completers.py Updates completer tests to avoid mode.
tests/test_commands/test_config.py Updates command tests for deprecation behavior and non-mode config set/get.
docs/essentials/modes.md Rewrites docs to describe runtime-detected REPL vs command-line.
docs/essentials/settings.md Removes mode from persisted settings documentation.
.github/instructions/test.instructions.md Updates documented test naming convention/examples.
.changes/unreleased/optimization-20260322-103653.yaml Adds changelog entry for the optimization/refactor.
Comments suppressed due to low confidence (1)

tests/test_core/test_fab_state_config.py:79

  • There’s a redundant local import json inside test_init_defaults_no_write_when_unchanged even though the module now imports json at the top. Removing the inner import will reduce noise and keep imports consistent.
    def test_init_defaults_no_write_when_unchanged(self, tmp_path, monkeypatch):
        """Test that init_defaults skips writing when config already has all defaults."""
        import json

        from fabric_cli.core import fab_constant

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings March 29, 2026 09:31
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

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

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

- Fix init_defaults() to set changed=True after deleting mode key and
  also clean up legacy fab_mode key
- Update test_context_persistence comments to reflect that REPL mode
  (not disabled persistence) is why file ops are skipped
- Use tmp_path instead of tempfile.mkdtemp() in _create_temp_config
  for automatic cleanup
- Rename double-underscore test names to single underscore in test_core
- Update PR description: Context singleton (not fab_constant) exposes
  runtime mode helpers

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

4 participants