fix: P3/P4/P5 — lazy config init, exception narrowing, CHANGELOG#121
Merged
fix: P3/P4/P5 — lazy config init, exception narrowing, CHANGELOG#121
Conversation
P3 — Lazy config initialisation: - config.py: `_config_instance` is now None at module level; `get_config()` creates the singleton on first call instead of at import time. Eliminates the `brew --version` subprocess that fired on every `import versiontracker.*`. - conftest.py: `reset_config` fixture now resets the module-level `_config_instance` (was targeting a non-existent class attribute and had no effect for the lifetime of the test suite). P4 (remainder) — Exception narrowing in __main__.py: - Plugin file load: `except (FileNotFoundError, ImportError, ValueError, OSError)` - `_initialize_plugins`: `except (ImportError, FileNotFoundError, OSError)` - Outermost `versiontracker_main` catch remains broad `Exception` (justified). P5 (remainder) — CHANGELOG and docs: - Added [Unreleased] entry covering all user-visible changes from P0–P4 (Homebrew command fix, progress flag fix, --output-file flag, lazy init, exception narrowing, README maturity label). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideImplements lazy initialisation of the global Config singleton, fixes the test fixture to reset the correct config instance, narrows plugin-related exception handling in main.py, and updates the changelog to document recent fixes and features. Sequence diagram for lazy Config initialisation and test resetsequenceDiagram
actor Pytest
participant ResetFixture as reset_config_fixture
participant ConfigModule as versiontracker_config
participant TestFunc as test_using_get_config
participant Config as Config_instance
Pytest->>ResetFixture: start test
ResetFixture->>ConfigModule: import versiontracker.config as _cfg_mod
ResetFixture->>ConfigModule: store original _config_instance
ResetFixture->>ConfigModule: set _config_instance = None
Pytest->>TestFunc: run test
TestFunc->>ConfigModule: get_config()
alt _config_instance is None
ConfigModule->>ConfigModule: check _config_instance is None
ConfigModule->>Config: create Config()
ConfigModule->>ConfigModule: assign _config_instance = Config()
end
ConfigModule-->>TestFunc: return _config_instance
Pytest->>ResetFixture: finish test
ResetFixture->>ConfigModule: restore _config_instance = original
Sequence diagram for narrowed plugin loading exceptions in mainsequenceDiagram
actor UserCLI as user
participant Main as versiontracker_main
participant PluginActions as _handle_plugin_actions
participant PluginInit as _initialize_plugins
participant PluginManager as plugin_manager
user->>Main: invoke versiontracker with options
Main->>PluginActions: _handle_plugin_actions(options)
alt load plugin from file
PluginActions->>PluginManager: load_plugin_from_file(plugin_path)
alt load succeeds
PluginManager-->>PluginActions: plugin loaded
PluginActions-->>Main: return 0
else FileNotFoundError or ImportError or ValueError or OSError
PluginManager-->>PluginActions: raise specific error
PluginActions-->>Main: return 1 (print failure message)
end
end
Main->>PluginInit: _initialize_plugins()
PluginInit->>PluginInit: from versiontracker.plugins import load_plugins
alt load_plugins succeeds
PluginInit->>PluginInit: load_plugins()
PluginInit-->>Main: return
else ImportError or FileNotFoundError or OSError
PluginInit-->>Main: log debug and skip plugin loading
end
Updated class diagram for Config singleton and reset fixtureclassDiagram
class Config {
}
class VersiontrackerConfigModule {
Config _config_instance
get_config() Config
}
class ConftestResetConfigFixture {
reset_config() None
}
VersiontrackerConfigModule ..> Config : creates
ConftestResetConfigFixture ..> VersiontrackerConfigModule : resets _config_instance
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🔒 Security Analysis ReportSecurity Analysis ReportGenerated: Tue Mar 31 19:53:48 UTC 2026 Bandit Security ScanSafety Check ResultsPip-Audit Results |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The lazy initialization of
_config_instanceinget_config()is not thread-safe; if the config can be accessed from multiple threads, consider guarding initialization with a lock or using a thread-safe pattern to avoid creating multiple instances. - The narrowed exception tuples in
__main__for plugin loading are now hard-coded in multiple places; consider centralizing the expected plugin-loading error types (e.g., in the plugin manager or a shared helper) so that future changes to error handling don’t require updating several catch lists.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The lazy initialization of `_config_instance` in `get_config()` is not thread-safe; if the config can be accessed from multiple threads, consider guarding initialization with a lock or using a thread-safe pattern to avoid creating multiple instances.
- The narrowed exception tuples in `__main__` for plugin loading are now hard-coded in multiple places; consider centralizing the expected plugin-loading error types (e.g., in the plugin manager or a shared helper) so that future changes to error handling don’t require updating several catch lists.
## Individual Comments
### Comment 1
<location path="conftest.py" line_range="60-63" />
<code_context>
+ """
+ import versiontracker.config as _cfg_mod
+
+ original = _cfg_mod._config_instance
+ _cfg_mod._config_instance = None
yield
- if hasattr(Config, "_instance"):
- Config._instance = None
+ _cfg_mod._config_instance = original
</code_context>
<issue_to_address>
**issue (testing):** Restoring the original singleton after each test can reintroduce shared mutable state across tests.
Because `original` may be a mutated, non-`None` instance, restoring it after `yield` can break test isolation by leaking state into subsequent tests. The previous behavior (resetting to `None` before and after each test) avoided this. To preserve per-test isolation and prevent cross-test bleed-through, you can simplify the fixture to:
```python
import versiontracker.config as _cfg_mod
@pytest.fixture(autouse=True)
def reset_config() -> Generator[None, None, None]:
_cfg_mod._config_instance = None
yield
_cfg_mod._config_instance = None
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+60
to
+63
| original = _cfg_mod._config_instance | ||
| _cfg_mod._config_instance = None | ||
| yield | ||
| if hasattr(Config, "_instance"): | ||
| Config._instance = None | ||
| _cfg_mod._config_instance = original |
Contributor
There was a problem hiding this comment.
issue (testing): Restoring the original singleton after each test can reintroduce shared mutable state across tests.
Because original may be a mutated, non-None instance, restoring it after yield can break test isolation by leaking state into subsequent tests. The previous behavior (resetting to None before and after each test) avoided this. To preserve per-test isolation and prevent cross-test bleed-through, you can simplify the fixture to:
import versiontracker.config as _cfg_mod
@pytest.fixture(autouse=True)
def reset_config() -> Generator[None, None, None]:
_cfg_mod._config_instance = None
yield
_cfg_mod._config_instance = None
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 762f4e0bfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| print(f"Plugin loaded from: {plugin_path}") | ||
| return 0 | ||
| except Exception as e: | ||
| except (FileNotFoundError, ImportError, ValueError, OSError) as e: |
There was a problem hiding this comment.
plugin_manager.load_plugin_from_file() wraps virtually all load failures into PluginError (see versiontracker/plugins/__init__.py), but this handler now only catches built-in exceptions. As a result, common --load-plugin failures (missing file, bad plugin class, init failure) now raise uncaught PluginError and abort the CLI with a traceback instead of returning the intended user-facing error and exit code 1 from _handle_plugin_actions.
Useful? React with 👍 / 👎.
docdyhr
added a commit
that referenced
this pull request
Mar 31, 2026
- P1: integration test for --no-progress done (PR #122) - P2: integration test for --export --output-file done (PR #122) - P3: lazy config init done (PR #121) - P5: CHANGELOG updated + PROJECT_REVIEW.md removed (PR #121) - Update objective note: all P0-P5 complete Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
docdyhr
added a commit
that referenced
this pull request
Mar 31, 2026
…put-file (#122) * docs: update TODO.md to reflect PR #117/#118 completions Mark P0, P1, P2, P4, P5 as done with per-item checkboxes. Add PR #117 and #118 to Recent Completions. Promote P3 to next priority (last blocker before v1.0). Correct test count to 2,158 passing / 15 skipped. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: add deferred integration tests for --no-progress and --export --output-file Covers the two deferred items from P1/P2 stabilisation: - test_no_progress_suppresses_progress: verifies that passing --no-progress (options.no_progress=True) through handle_configure_from_options() sets the canonical config.no_progress=True and that config.show_progress returns False. - test_export_output_file_writes_file: verifies that passing --export json --output-file PATH writes the export to a file and does not echo the raw JSON export data to stdout. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: tick off deferred P1/P2/P3/P5 TODO items - P1: integration test for --no-progress done (PR #122) - P2: integration test for --export --output-file done (PR #122) - P3: lazy config init done (PR #121) - P5: CHANGELOG updated + PROJECT_REVIEW.md removed (PR #121) - Update objective note: all P0-P5 complete Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Config()singleton no longer created at import time;get_config()initialises on first call, eliminating thebrew --versionsubprocess that fired on everyimport versiontracker.*conftest.pyreset_configfixture now resets the correct module-level_config_instance(was targeting a non-existent class attribute — had silently no effect)__main__.py: plugin loading catches narrowed to specific types; outermostversiontracker_mainboundary retains broadExceptionwith justification[Unreleased]entry covering all user-visible changes from P0–P4; removedPROJECT_REVIEW.mdfrom repo rootTest plan
pytest tests/test_config.py tests/test_main.py tests/test_integration.py— 34 passed🤖 Generated with Claude Code
Summary by Sourcery
Defer configuration singleton creation to first access, narrow plugin-loading exception handling, and update unreleased changelog entries accordingly.
Bug Fixes:
Documentation: