[codex] stabilize module install and init state#535
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a metadata-only module availability classifier, repo-scoped discovery and preserve-merge semantics for init, install-time enable/repair of disabled modules, enhanced missing-command diagnostics, associated OpenSpec docs/tests, and bumps package/module versions to 0.46.7. Changes
Sequence DiagramsequenceDiagram
autonumber
participant User as "CLI (specfact)"
participant CLI as "Missing-Command Handler"
participant Classifier as "Module Availability Classifier"
participant Discover as "Module Discovery"
participant Packages as "Package/State Merge"
participant State as "Persisted State (modules.json)"
rect rgba(200,200,255,0.5)
User->>CLI: invoke command for group X
CLI->>Classifier: classify_module_availability(module_id?, command_name?, base_path?)
Classifier-->>CLI: ModuleAvailability(status, module_id, package_dir, reason, recovery_command)
alt status == DISABLED or SKIPPED or SHADOWED
CLI-->>User: print targeted remediation (enable/repair/skip reason/shadow info)
else status == ABSENT
CLI-->>User: print generic install/init guidance
end
end
rect rgba(200,255,200,0.5)
User->>CLI: run init --repo <path> --profile <id>
CLI->>Discover: discover_all_modules_for_project(base_path=<path>)
Discover-->>Packages: discovered_modules
CLI->>Packages: get_discovered_modules_for_state(enable_ids=[profile ids], base_path=<path>, preserve_existing=true)
Packages->>State: read_modules_state()
State-->>Packages: existing_state
Packages->>Packages: merge(discovered, existing_state, preserve_existing=true)
Packages-->>State: write_modules_state(merged_module_list)
State-->>CLI: success
CLI-->>User: output (enabled modules + preserved unrelated state)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
djm81
added
bug
![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: c92dbff0c9
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/specfact_cli/modules/module_registry/src/commands.py (1)
208-239:⚠️ Potential issue | 🟠 MajorUse canonical availability here, not
requested_namelookup.Line 232 looks up
discovered_by_namewithrequested_name, but that map is keyed byentry.metadata.name. For canonical manifests such asnold-ai/specfact-codebase, bothspecfact module install specfact-codebaseandspecfact module install nold-ai/specfact-codebasemiss the existing local copy and fall through to bundled/marketplace install. Even when the lookup does hit, this branch still treats every discovered local copy as “already available”, so disabled/skipped/shadowed cases bypass the new repair and recovery behavior. Resolve the canonical manifest id first and drive this short-circuit from the shared availability classifier.As per coding guidelines,
openspec/**/*.md: Treat as specification source of truth: proposal/tasks/spec deltas vs. code behavior, CHANGE_ORDER consistency, and scenario coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 15-17: Expand the 0.46.5 entry so the changelog documents all
significant changes instead of one compressed "Fixed" bullet: keep the existing
Fixed line for "Module install and init" (the current bullet text) and add
separate bullets under "Changed" and/or "Added" to explicitly mention the
diagnostics behavior changes and the new reality-test coverage, using clear
short titles like "Changed: diagnostics behavior" and "Added: reality-test
coverage" so downstream users can trace the updates.
- Line 17: The markdown bullet starting with "**Module install and init**" is
over 120 characters; reflow that single-line bullet into multiple wrapped lines
(preserving the bold header and sentence content) so no line exceeds 120 chars,
breaking at word boundaries and keeping continuation lines as plain wrapped text
under the same bullet; ensure the final wrapped lines read naturally and keep
the original meaning.
In `@src/specfact_cli/modules/module_registry/src/commands.py`:
- Around line 198-205: The helper _enable_if_disabled currently calls
get_discovered_modules_for_state without passing the selected repo/base path, so
modules that exist only under a non-default repo (e.g., invoked via module
install --scope project --repo /other/repo) are not rediscovered and the state
stays unchanged; update _enable_if_disabled(module_id: str) to accept and thread
a base_path (or repo) through to get_discovered_modules_for_state (i.e., call
get_discovered_modules_for_state(enable_ids=[module_id], disable_ids=[],
preserve_existing=True, base_path=base_path)) and ensure callers pass the
selected repo/base path when invoking _enable_if_disabled so the disabled-state
repair actually discovers modules in the target repository before writing state
and returning True.
In `@src/specfact_cli/registry/module_availability.py`:
- Around line 14-18: The availability classifier imports two private helpers
(_check_core_compatibility and _validate_module_dependencies) from
module_packages, creating tight coupling; extract these helpers into a new
shared module (e.g., module_checks.py) with public names
(check_core_compatibility, validate_module_dependencies), update
module_availability to import the new public functions, and adjust
module_packages to either import and re-export the shared functions or call them
from module_checks to preserve behavior; ensure all references to the
underscore-prefixed names in module_availability, module_packages, and tests are
updated to the new public names and that module_checks has clear public API
contracts and importable symbols.
- Around line 88-95: _add a clear docstring on the _skip_reason function stating
it performs metadata-only classification (core compatibility and dependency
checks) and intentionally omits deeper schema and integrity validations for
performance; mention that schema/integrity checks are performed elsewhere for
requested install/command paths. Reference the function name _skip_reason and
the types DiscoveredModule/enabled_map in the docstring so reviewers can find
the intended behavior quickly. Ensure the docstring is concise and placed
immediately above the _skip_reason definition.
- Around line 22-30: The ModuleAvailabilityStatus enum defines an unused value
AMBIGUOUS; update the enum or its documentation to remove ambiguity: either
delete the AMBIGUOUS member from ModuleAvailabilityStatus so the enum only
contains the actual returned states, or retain AMBIGUOUS but add an explicit
comment on ModuleAvailabilityStatus explaining it is reserved for future use
(and why) and add a TODO or contract note near classify_module_availability to
indicate why it currently doesn't return AMBIGUOUS; reference
ModuleAvailabilityStatus and classify_module_availability when making the
change.
In `@src/specfact_cli/registry/module_packages.py`:
- Around line 1427-1428: Add an icontract precondition to the function that
declares the parameters base_path and preserve_existing to enforce semantics:
require that if preserve_existing is True then base_path is not None (e.g.
`@icontract.require`(lambda preserve_existing, base_path: not preserve_existing or
base_path is not None, "When preserve_existing is True, base_path must be
provided")). Ensure icontract is imported and update the function docstring to
state this contract.
In `@tests/unit/modules/module_registry/test_commands.py`:
- Around line 119-126: The mock uses a lambda with an "or" side-effect trick
(enabled.append(... ) or [...]) which is terse but odd; replace the lambda with
a clearly named helper function (e.g., mock_list_modules or _mock_list_modules)
that accepts the same keyword args (enable_ids, disable_ids, preserve_existing),
performs enabled.append(list(enable_ids)), and then returns the list of module
dicts, and use that function in place of the lambda to make the side-effect
explicit and readable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c43fa3df-7f4a-4bb7-a7fe-fa9d2d498181
📒 Files selected for processing (28)
CHANGELOG.mddocs/core-cli/init.mddocs/module-system/installing-modules.mdopenspec/changes/marketplace-07-module-install-state-consistency/.openspec.yamlopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdpyproject.tomlsetup.pysrc/__init__.pysrc/specfact_cli/__init__.pysrc/specfact_cli/cli.pysrc/specfact_cli/modules/init/module-package.yamlsrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/modules/module_registry/module-package.yamlsrc/specfact_cli/modules/module_registry/src/commands.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/registry/module_packages.pytests/unit/modules/init/test_first_run_selection.pytests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/specfact_cli/test_module_not_found_error.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Compatibility (Python 3.11)
- GitHub Check: Linting (ruff, pylint, safe-write guard)
- GitHub Check: Tests (Python 3.12)
- GitHub Check: Socket Security: Pull Request Alerts
🧰 Additional context used
📓 Path-based instructions (20)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/*.py: Maintain minimum 80% test coverage, with 100% coverage for critical paths in Python code
Use clear naming and self-documenting code, preferring clear names over comments
Ensure each function/class has a single clear purpose (Single Responsibility Principle)
Extract common patterns to avoid code duplication (DRY principle)
Apply SOLID object-oriented design principles in Python code
Use type hints everywhere in Python code and enable basedpyright strict mode
Use Pydantic models for data validation and serialization in Python
Use async/await for I/O operations in Python code
Use context managers for resource management in Python
Use dataclasses for simple data containers in Python
Enforce maximum line length of 120 characters in Python code
Use 4 spaces for indentation in Python code (no tabs)
Use 2 blank lines between classes and 1 blank line between methods in Python
Organize imports in order: Standard library → Third party → Local in Python files
Use snake_case for variables and functions in Python
Use PascalCase for class names in Python
Use UPPER_SNAKE_CASE for constants in Python
Use leading underscore (_) for private methods in Python classes
Use snake_case for Python file names
Enable basedpyright strict mode with strict type checking configuration in Python
Use Google-style docstrings for functions and classes in Python
Include comprehensive exception handling with specific exception types in Python code
Use logging with structured context (extra parameters) instead of print statements
Use retry logic with tenacity decorators (@retry) for operations that might fail
Use Pydantic BaseSettings for environment-based configuration in Python
Validate user input using Pydantic validators in Python models
Use@lru_cacheand Redis-based caching for expensive calculations in Python
Run code formatting with Black (120 character line length) and isort in Python
Run type checking with basedpyright on all Python files
Run linting with ruff and pylint on all Pyth...
Files:
src/__init__.pysrc/specfact_cli/cli.pytests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/modules/module_registry/test_commands.pytests/unit/modules/init/test_first_run_selection.pysrc/specfact_cli/__init__.pytests/unit/specfact_cli/registry/test_module_availability.pysetup.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_packages.pysrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/registry/module_discovery.pytests/unit/specfact_cli/test_module_not_found_error.pysrc/specfact_cli/modules/module_registry/src/commands.py
{src/__init__.py,pyproject.toml,setup.py}
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
{src/__init__.py,pyproject.toml,setup.py}: Update src/init.py first as primary source of truth for package version, then pyproject.toml and setup.py
Maintain version synchronization across src/init.py, pyproject.toml, and setup.py
Files:
src/__init__.pypyproject.tomlsetup.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
src/**/*.py: All code changes must be followed by running the full test suite using the smart test system.
All Python files in src/ and tools/ directories must have corresponding test files in tests/ directory. If you modify src/common/logger_setup.py, you MUST have tests/unit/common/test_logger_setup.py. NO EXCEPTIONS - even small changes require tests.
All new Python runtime code files must have corresponding test files created BEFORE committing the code. NO EXCEPTIONS - no code without tests.
Test Coverage Validation: Run hatch run smart-test-unit for modified files, hatch run smart-test-folder for modified directories, and hatch run smart-test-full before committing. ALL TESTS MUST PASS.
All components must support TEST_MODE=true environment variable with test-specific behavior defined as: if os.environ.get('TEST_MODE') == 'true': # test-specific behavior
Use src/common/logger_setup.py for all logging via: from common.logger_setup import get_logger; logger = get_logger(name)
Use src/common/redis_client.py with fallback for Redis operations via: from common.redis_client import get_redis_client; redis_client = get_redis_client()
Type checking must pass with no errors using: mypy .
Test coverage must meet or exceed 80% total coverage. New code must have corresponding tests. Modified code must maintain or improve coverage. Critical paths must have 100% coverage.
Use Pydantic v2 validation for all context and data schemas.Add/update contracts on new or modified public APIs, stateful classes and adapters using
icontractdecorators andbeartyperuntime type checks
src/**/*.py: Meaningful Naming — identifiers reveal intent; avoid abbreviations. Identifiers insrc/must usesnake_case(modules/functions),PascalCase(classes),UPPER_SNAKE_CASE(constants). Avoid single-letter names outside short loop variables.
KISS — keep functions and modules small and single-purpose. Maximum function length: 120 lines (Phase A error threshold). Maximum cyclomati...
Files:
src/__init__.pysrc/specfact_cli/cli.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_packages.pysrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
@(src|tests)/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Linting must pass with no errors using: pylint src tests
Files:
src/__init__.pysrc/specfact_cli/cli.pytests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/modules/module_registry/test_commands.pytests/unit/modules/init/test_first_run_selection.pysrc/specfact_cli/__init__.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_packages.pysrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/registry/module_discovery.pytests/unit/specfact_cli/test_module_not_found_error.pysrc/specfact_cli/modules/module_registry/src/commands.py
{pyproject.toml,setup.py,src/__init__.py}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Manually update version numbers in pyproject.toml, setup.py, and src/init.py when making a formal version change
Files:
src/__init__.pypyproject.tomlsetup.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{py,pyi}: After any code changes, follow these steps in order: (1) Apply linting and formatting to ensure code quality:hatch run format, (2) Type checking:hatch run type-check(basedpyright), (3) Contract-first approach: Runhatch run contract-testfor contract validation, (4) Run full test suite:hatch test --cover -v, (5) Verify all tests pass and contracts are satisfied, (6) Fix any issues and repeat steps until all tests pass
All public APIs must have@icontractdecorators and@beartypetype checking
Use Pydantic models for all data structures with data validation
Only write high-value comments if at all. Avoid talking to the user through comments
Files:
src/__init__.pysrc/specfact_cli/cli.pytests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/modules/module_registry/test_commands.pytests/unit/modules/init/test_first_run_selection.pysrc/specfact_cli/__init__.pytests/unit/specfact_cli/registry/test_module_availability.pysetup.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_packages.pysrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/registry/module_discovery.pytests/unit/specfact_cli/test_module_not_found_error.pysrc/specfact_cli/modules/module_registry/src/commands.py
pyproject.toml
📄 CodeRabbit inference engine (.cursorrules)
When updating the version in
pyproject.toml, ensure it's newer than the latest PyPI version. The CI/CD pipeline will automatically publish to PyPI only if the new version is greater than the published version
Files:
pyproject.toml
**/*.{md,mdc}
📄 CodeRabbit inference engine (.cursor/rules/markdown-rules.mdc)
**/*.{md,mdc}: Do not use more than one consecutive blank line anywhere in the document (MD012: No Multiple Consecutive Blank Lines)
Fenced code blocks should be surrounded by blank lines (MD031: Fenced Code Blocks)
Lists should be surrounded by blank lines (MD032: Lists)
Files must end with a single empty line (MD047: Files Must End With Single Newline)
Lines should not have trailing spaces (MD009: No Trailing Spaces)
Use asterisks (**) for strong emphasis, not underscores (__) (MD050: Strong Style)
Fenced code blocks must have a language specified (MD040: Fenced Code Language)
Headers should increment by one level at a time (MD001: Header Increment)
Headers should be surrounded by blank lines (MD022: Headers Should Be Surrounded By Blank Lines)
Only one top-level header (H1) is allowed per document (MD025: Single H1 Header)
Use consistent list markers, preferring dashes (-) for unordered lists (MD004: List Style)
Nested unordered list items should be indented consistently, typically by 2 spaces (MD007: Unordered List Indentation)
Use exactly one space after the list marker (e.g., -, *, +, 1.) (MD030: Spaces After List Markers)
Use incrementing numbers for ordered lists (MD029: Ordered List Item Prefix)
Enclose bare URLs in angle brackets or format them as links (MD034: Bare URLs)
Don't use spaces immediately inside code spans (MD038: Spaces Inside Code Spans)
Use consistent indentation (usually 2 or 4 spaces) throughout markdown files
Keep line length under 120 characters in markdown files
Use reference-style links for better readability in markdown files
Use a trailing slash for directory paths in markdown files
Ensure proper escaping of special characters in markdown files
Files:
docs/core-cli/init.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdCHANGELOG.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.mddocs/module-system/installing-modules.md
docs/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Update architecture documentation in docs/ for architecture changes, state machine documentation for FSM modifications, interface documentation for API changes, and configuration guides for configuration changes. DO NOT create internal docs in specfact-cli repo folder that should not be visible to end users; use the respective internal repository instead.
Files:
docs/core-cli/init.mddocs/module-system/installing-modules.md
⚙️ CodeRabbit configuration file
docs/**/*.md: User-facing accuracy: CLI examples match current behavior; preserve Jekyll front matter;
call out when README/docs index need sync.
Files:
docs/core-cli/init.mddocs/module-system/installing-modules.md
**/*.md
📄 CodeRabbit inference engine (.cursorrules)
Avoid markdown linting errors (refer to markdown-rules)
Files:
docs/core-cli/init.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdCHANGELOG.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.mddocs/module-system/installing-modules.md
openspec/changes/**/*.md
📄 CodeRabbit inference engine (.cursorrules)
For
/opsx:archive(Archive change): Include module signing and cleanup in final tasks. Agents MUST runopenspec archive <change-id>from repo root (no manualmvunderopenspec/changes/archive/)
Files:
openspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.md
openspec/**/{proposal.md,tasks.md,design.md,spec.md}
📄 CodeRabbit inference engine (.cursor/rules/automatic-openspec-workflow.mdc)
openspec/**/{proposal.md,tasks.md,design.md,spec.md}: Applyopenspec/config.yamlproject context and per-artifact rules (for proposal, specs, design, tasks) when creating or updating any OpenSpec change artifact in the specfact-cli codebase
After implementation, validate the change withopenspec validate <change-id> --strict; fix validation errors in proposal, specs, design, or tasks and re-validate until passing before considering the change complete
Files:
openspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.md
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Treat as specification source of truth: proposal/tasks/spec deltas vs. code behavior,
CHANGE_ORDER consistency, and scenario coverage. Surface drift between OpenSpec and
implementation.
Files:
openspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.md
CHANGELOG.md
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
Include new version entries at the top of CHANGELOG.md when updating versions
Update CHANGELOG.md with all code changes as part of version control requirements.
Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change
Files:
CHANGELOG.md
**/*cli*.{py,pyi}
📄 CodeRabbit inference engine (.cursorrules)
Commands should follow typer patterns with rich console output
Files:
src/specfact_cli/cli.py
src/specfact_cli/**/*.py
⚙️ CodeRabbit configuration file
src/specfact_cli/**/*.py: Focus on modular CLI architecture: lazy module loading, registry/bootstrap patterns, and
dependency direction. Flag breaking changes to public APIs, Pydantic models, and resource
bundling. Verify@icontract+@beartypeon public surfaces; prefer centralized logging
(get_bridge_logger) over print().
Files:
src/specfact_cli/cli.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_packages.pysrc/specfact_cli/modules/init/src/commands.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/test_*.py: Write tests first in test-driven development (TDD) using the Red-Green-Refactor cycle
Ensure each test is independent and repeatable with no shared state between tests
Organize Python imports in tests using unittest.mock for Mock and patch
Use setup_method() for test initialization and Arrange-Act-Assert pattern in test files
Use@pytest.mark.asynciodecorator for async test functions in Python
Organize test files in structure: tests/unit/, tests/integration/, tests/e2e/ by module
Files:
tests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/modules/module_registry/test_commands.pytests/unit/modules/init/test_first_run_selection.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/specfact_cli/test_module_not_found_error.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Tests must be meaningful and test actual functionality, cover both success and failure cases, be independent and repeatable, and have clear, descriptive names. NO EXCEPTIONS - no placeholder or empty tests.
tests/**/*.py: Trim low-value unit tests when a contract covers the same assertion (type/shape/raises on negative checks)
Delete tests that only assert input validation, datatype/shape enforcement, or raises on negative conditions now guarded by contracts and runtime typing
Convert repeated edge-case permutations into one Hypothesis property with contracts acting as oraclesSecret redaction via
LoggerSetup.redact_secretsmust be covered by unit tests
Files:
tests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/modules/module_registry/test_commands.pytests/unit/modules/init/test_first_run_selection.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/specfact_cli/test_module_not_found_error.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first testing: meaningful scenarios, not redundant assertions already covered by
contracts. Flag flakiness, environment coupling, and missing coverage for changed behavior.
Files:
tests/unit/specfact_cli/registry/test_init_module_state.pytests/unit/modules/module_registry/test_commands.pytests/unit/modules/init/test_first_run_selection.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/specfact_cli/test_module_not_found_error.py
**/*.yaml
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
YAML files must pass linting using: hatch run yaml-lint with relaxed policy.
Files:
src/specfact_cli/modules/init/module-package.yamlsrc/specfact_cli/modules/module_registry/module-package.yaml
**/*.{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Validate YAML configuration files locally using
hatch run yaml-lintbefore committing
**/*.{yml,yaml}: Format all YAML and workflow files usinghatch run yaml-fix-allbefore committing
Use Prettier to fix whitespace, indentation, and final newline across YAML files
Use yamllint with the repo .yamllint configuration (line-length 140, trailing spaces and final newline enforced) to lint non-workflow YAML files
Files:
src/specfact_cli/modules/init/module-package.yamlsrc/specfact_cli/modules/module_registry/module-package.yaml
🧠 Learnings (22)
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to openspec/changes/**/*.md : For `/opsx:archive` (Archive change): Include module signing and cleanup in final tasks. Agents MUST run `openspec archive <change-id>` from repo root (no manual `mv` under `openspec/changes/archive/`)
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/.openspec.yamldocs/core-cli/init.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mdopenspec/changes/marketplace-07-module-install-state-consistency/proposal.md
📚 Learning: 2026-04-10T22:41:54.436Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/automatic-openspec-workflow.mdc:0-0
Timestamp: 2026-04-10T22:41:54.436Z
Learning: Before implementing code changes, run `openspec validate <change-id> --strict` to validate the OpenSpec change and optionally run `/wf-validate-change <change-id>` for breaking-change and dependency analysis; do not proceed to implementation until validation passes or user explicitly overrides
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/.openspec.yamlopenspec/changes/marketplace-07-module-install-state-consistency/tasks.md
📚 Learning: 2026-04-10T22:42:04.372Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/session_startup_instructions.mdc:0-0
Timestamp: 2026-04-10T22:42:04.372Z
Learning: For OpenSpec changes when a sibling `specfact-cli-internal/` checkout exists, read internal wiki guidance in `docs/agent-rules/40-openspec-and-tdd.md` and consult `wiki/hot.md`, `wiki/graph.md`, and relevant `wiki/concepts/*.md` files
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/.openspec.yamldocs/core-cli/init.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mddocs/module-system/installing-modules.md
📚 Learning: 2026-04-10T22:41:54.436Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/automatic-openspec-workflow.mdc:0-0
Timestamp: 2026-04-10T22:41:54.436Z
Learning: When implementation is complete and merged, run `openspec archive <change-id>` from the repository root to merge delta specs into openspec/specs/ and move the change under openspec/changes/archive/; do not manually move folders or use --skip-specs or --no-validate without explicit user confirmation
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/.openspec.yamlopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.md
📚 Learning: 2026-04-10T22:41:54.436Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/automatic-openspec-workflow.mdc:0-0
Timestamp: 2026-04-10T22:41:54.436Z
Learning: Applies to openspec/**/{proposal.md,tasks.md,design.md,spec.md} : Apply `openspec/config.yaml` project context and per-artifact rules (for proposal, specs, design, tasks) when creating or updating any OpenSpec change artifact in the specfact-cli codebase
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/.openspec.yamldocs/core-cli/init.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.md
📚 Learning: 2026-03-25T21:33:15.296Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/testing-and-build-guide.mdc:0-0
Timestamp: 2026-03-25T21:33:15.296Z
Learning: Applies to {pyproject.toml,setup.py,src/__init__.py} : Manually update version numbers in pyproject.toml, setup.py, and src/__init__.py when making a formal version change
Applied to files:
src/__init__.pypyproject.tomlsrc/specfact_cli/__init__.pysetup.py
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to {src/__init__.py,pyproject.toml,setup.py} : Update src/__init__.py first as primary source of truth for package version, then pyproject.toml and setup.py
Applied to files:
src/__init__.pypyproject.toml
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to {src/__init__.py,pyproject.toml,setup.py} : Maintain version synchronization across src/__init__.py, pyproject.toml, and setup.py
Applied to files:
src/__init__.pypyproject.toml
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to @(pyproject.toml|setup.py|src/__init__.py|src/specfact_cli/__init__.py) : Maintain synchronized versions across pyproject.toml, setup.py, src/__init__.py, and src/specfact_cli/__init__.py.
Applied to files:
src/__init__.pypyproject.tomlsrc/specfact_cli/__init__.pysetup.py
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to pyproject.toml : When updating the version in `pyproject.toml`, ensure it's newer than the latest PyPI version. The CI/CD pipeline will automatically publish to PyPI only if the new version is greater than the published version
Applied to files:
pyproject.toml
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to docs/**/*.md : Update architecture documentation in docs/ for architecture changes, state machine documentation for FSM modifications, interface documentation for API changes, and configuration guides for configuration changes. DO NOT create internal docs in specfact-cli repo folder that should not be visible to end users; use the respective internal repository instead.
Applied to files:
docs/core-cli/init.mdopenspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/specs/init-module-state/spec.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.mdopenspec/changes/marketplace-07-module-install-state-consistency/design.mddocs/module-system/installing-modules.md
📚 Learning: 2026-04-10T22:41:54.436Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/automatic-openspec-workflow.mdc:0-0
Timestamp: 2026-04-10T22:41:54.436Z
Learning: If `openspec` command is not found or if specfact-cli workspace or openspec/ directory is not accessible, do not modify application code unless the user explicitly confirms to proceed without OpenSpec
Applied to files:
docs/core-cli/init.mdsrc/specfact_cli/modules/init/module-package.yaml
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: All development work MUST use git worktrees per AGENTS.md Git Worktree Policy. Never create branches with `git checkout -b` in the primary checkout. Create worktree from origin/dev: `git worktree add ../specfact-cli-worktrees/<type>/<slug> -b <branch-name> origin/dev` where allowed types are: `feature/`, `bugfix/`, `hotfix/`, `chore/`. Forbidden in worktrees: `dev`, `main`. After creating worktree: `cd ../specfact-cli-worktrees/<type>/<slug>`. Bootstrap Hatch in worktree: `hatch env create`. Run pre-flight checks: `hatch run smart-test-status` and `hatch run contract-test-status`. Do all implementation work from the worktree, never from primary checkout. After PR merge: cleanup with `git worktree remove`, `git branch -d`, `git worktree prune`
Applied to files:
docs/core-cli/init.mdsrc/specfact_cli/modules/init/module-package.yaml
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: For `/opsx:ff` (Fast-Forward Change Creation): OPSX provides change scaffolding and artifact templates. AGENTS.md requires explicitly adding: Worktree creation task as Step 1 in tasks.md (not just 'create branch'), TDD_EVIDENCE.md tracking task in section 2 (Tests), Documentation research task per `openspec/config.yaml`, Module signing quality gate if applicable, Worktree cleanup steps in final section
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.md
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: When creating implementation plans, task lists, or OpenSpec tasks.md, ALWAYS explicitly verify and include: (1) Worktree creation from `origin/dev` (not `git checkout -b`), (2) `hatch env create` in the worktree, (3) Pre-flight checks (`hatch run smart-test-status`, `hatch run contract-test-status`), (4) Worktree cleanup steps post-merge, (5) Self-check: 'Have I followed AGENTS.md Git Worktree Policy section?'
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.md
📚 Learning: 2026-04-10T22:41:54.436Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/automatic-openspec-workflow.mdc:0-0
Timestamp: 2026-04-10T22:41:54.436Z
Learning: Applies to openspec/**/{proposal.md,tasks.md,design.md,spec.md} : After implementation, validate the change with `openspec validate <change-id> --strict`; fix validation errors in proposal, specs, design, or tasks and re-validate until passing before considering the change complete
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.md
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Before executing ANY workflow command (`/opsx:ff`, `/opsx:apply`, `/opsx:continue`, etc.), perform the Pre-Execution Checklist: (1) Git Worktree - create if task creates branches/modifies code, (2) TDD Evidence - create `TDD_EVIDENCE.md` if behavior changes, (3) Documentation - include documentation research if changes affect user-facing behavior, (4) Module Signing - include signature verification if changes modify bundled modules, (5) Confirmation - state clearly that pre-execution checklist is complete and AGENTS.md compliance is confirmed
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.md
📚 Learning: 2026-04-10T22:41:54.436Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/automatic-openspec-workflow.mdc:0-0
Timestamp: 2026-04-10T22:41:54.436Z
Learning: Applies to {src,tools}/**/*.{js,ts,jsx,tsx,py},**/*.test.{js,ts,jsx,tsx,py},**/*.spec.{js,ts,jsx,tsx,py} : Do not add, modify, or delete any application code in src/, tools/, tests, or significant docs until an OpenSpec change (new or delta) is created and validated, unless the user explicitly opts out with 'skip openspec', 'direct implementation', 'simple fix', or 'just fix it'
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.md
📚 Learning: 2026-04-10T22:42:21.860Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-10T22:42:21.860Z
Learning: Perform `spec -> tests -> failing evidence -> code -> passing evidence` in that order for behavior changes
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.mdopenspec/changes/marketplace-07-module-install-state-consistency/TDD_EVIDENCE.md
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: For `/opsx:apply` (Implementation): OPSX provides task iteration and progress tracking. AGENTS.md requires verification before each task: Confirm you are IN a worktree (not primary checkout) before modifying code, Record failing test evidence in `TDD_EVIDENCE.md` BEFORE implementing, Record passing test evidence AFTER implementation, Run quality gates from worktree (format, type-check, contract-test), GPG-signed commits (`git commit -S`), PR to `dev` branch (never direct push)
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/tasks.md
📚 Learning: 2026-03-25T21:33:15.296Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/testing-and-build-guide.mdc:0-0
Timestamp: 2026-03-25T21:33:15.296Z
Learning: Applies to CHANGELOG.md : Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change
Applied to files:
CHANGELOG.md
📚 Learning: 2026-04-10T22:42:21.860Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-10T22:42:21.860Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
openspec/changes/marketplace-07-module-install-state-consistency/specs/module-installation/spec.mdsrc/specfact_cli/modules/module_registry/module-package.yaml
🪛 LanguageTool
openspec/changes/marketplace-07-module-install-state-consistency/design.md
[style] ~19-~19: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ses when they can be derived locally. - Make `specfact init --repo ... --profile ......
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~46-~46: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... specfact module enable <id> command. If skipped for integrity, compatibility, s...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~75-~75: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...h and merge state deterministically. 5. Update user-facing docs for module install/lis...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context nold-ai/specfact-cli-modules
[::nold-ai/specfact-cli-modules::] docs/guides/installing-modules.md: references specfact module install usage and repo/scope flags; documentation consumer of the CLI behaviors fixed by this PR (install/enable/repair messaging and repo-scoped --repo behavior).
[::nold-ai/specfact-cli-modules::] docs/reference/commands.md: documents specfact init --profile/--install and specfact module install CLIs; these docs are consumers of the UX/diagnostic changes (absent/disabled/skipped/shadowed messaging) introduced by the PR.
[::nold-ai/specfact-cli-modules::] docs/guides/marketplace.md and docs/getting-started/choose-your-modules.md: reference marketplace module ids and init/profile workflows; relevant to profile-driven enabling and discovery semantics changed by the PR.
[::nold-ai/specfact-cli-modules::] docs/team-and-enterprise/multi-repo.md and similar team docs (many files): show examples using specfact init --repo and profile flags; these content files depend on the repo-scoped discovery/preserve-existing behavior fixed here.
Summary assessment:
- No code-level consumers (no executable code) were found in nold-ai/specfact-cli-modules — this repository primarily contains documentation that references the CLI commands and expected behaviors changed by the PR.
- The repository’s docs are aligned with the PR scope (install/init/profile/repo behaviors); reviewers should confirm documentation examples/diagnostic text remain accurate after the PR’s messaging and recovery-command changes.
🔇 Additional comments (27)
src/specfact_cli/modules/module_registry/module-package.yaml (1)
2-2: Version bump and integrity refresh look consistent.The manifest update at Line 2 and checksum refresh at Line 20 align with the release scope.
Also applies to: 20-20
openspec/changes/marketplace-07-module-install-state-consistency/.openspec.yaml (1)
1-2: OpenSpec registration metadata is clean and traceable.
schemaand concretecreateddate are correctly declared for the new change entry.src/__init__.py (1)
6-6: Version source update is consistent.At Line 6, the package version aligns with the coordinated release bump.
src/specfact_cli/__init__.py (1)
48-48: Public package version export stays aligned.The update at Line 48 keeps runtime-reported version in sync with release metadata.
setup.py (1)
10-10: Setup package version is correctly synchronized.At Line 10, the version matches the coordinated
0.46.5release metadata.pyproject.toml (1)
7-7: Version 0.46.5 correctly supersedes PyPI release 0.46.4—release is gated for publish.Line 7 confirms the local version is ahead of the current PyPI publication, enabling the CI/CD pipeline to proceed with publication.
src/specfact_cli/modules/init/module-package.yaml (1)
20-20: Clarify when module signatures are required: checksum-only acceptable on PR branches per policy, but signature required for main.At Line 20, the init module uses checksum-only integrity. This is acceptable on non-main branches (where the policy defers signing to CI and requires only version bumps), but strict verification (checksum + signature) is required on main.
However, note two structural concerns:
Tier classification: The module is
tier: community, not "core" (thecategory: corefield refers to command bundling, not module tier).Architectural inconsistency: The upgrade module (also
tier: community) has a signature in its manifest, while init and module_registry do not—all three changed in the same commit. The branch-aware policy (git-branch-module-signature-flag.sh) defers signature verification to CI on PR branches and requires strict checks on main. Confirm whether bundled community-tier modules should have uniform signing expectations.The version was bumped (0.1.31), satisfying the version-increment requirement from the signing policy, but signature alignment across bundled modules may need review before main merge.
docs/core-cli/init.md (1)
55-56: LGTM! Documentation accurately reflects repo-scoped discovery and state preservation.The added paragraph correctly documents the
--profileand--installenable semantics along with the repo-aware discovery behavior. This aligns with the implementation inget_discovered_modules_for_state(..., preserve_existing=True)shown in the relevant context snippets.src/specfact_cli/cli.py (2)
69-69: LGTM! Import introduces metadata-only availability classification.This import enables the CLI to differentiate module availability states (disabled, skipped, shadowed, absent) without eagerly loading module command code—a key architectural decision for lazy module loading and deterministic diagnostics.
128-149: Well-structured availability-based diagnostic routing.The branching logic correctly prioritizes DISABLED → SKIPPED → SHADOWED before falling through to the "not installed" path. This aligns with the OpenSpec design decision to differentiate lifecycle states in missing-command UX.
Minor observation: The
availability.module_id or module_idfallback pattern appears in all three branches. This is safe given the classifier always populatesmodule_idfor non-ABSENT statuses, but the consistency is good for defensive coding.tests/unit/modules/init/test_first_run_selection.py (1)
166-200: Excellent coverage of repo-scoped discovery contract.This test validates the critical architectural invariant from the OpenSpec
init-module-statespec:get_discovered_modules_for_statemust receivebase_pathfrom--repo,preserve_existing=True, and the profile-resolvedenable_ids. The captured kwargs pattern cleanly verifies the contract without coupling to implementation details.The assertion at line 195 using
tmp_path.resolve()correctly handles path normalization, matching how the implementation resolves the repo path.tests/unit/specfact_cli/registry/test_init_module_state.py (1)
72-86: Targeted test for merge-based state preservation.This test directly validates the OpenSpec
init-module-staterequirement that repo-scoped refresh must preserve lifecycle state for modules outside the discovery view. The explicit dict assertions at lines 84-85 verify both:
- Discovered modules appear with default
enabled: True- Pre-existing unseen modules retain their stored state (
enabled: False,version: 9.9.9)The mock at line 78-80 cleanly isolates discovery to a controlled base_path scope.
tests/unit/modules/module_registry/test_commands.py (1)
97-146: Comprehensive test for disabled-module enable-on-install flow.This test validates the OpenSpec
module-installationspec requirement thatspecfact module installmust repair/enable disabled modules rather than reporting only "already installed." The test captures:
enable_idspassed to discovery (line 138)- Exact state payload written to
modules.jsonincluding unrelated modules (lines 139-144)- User-facing "enabled" feedback in stdout (line 145)
openspec/changes/marketplace-07-module-install-state-consistency/specs/user-module-root/spec.md (1)
1-29: Well-structured scope precedence specification.The three scenarios comprehensively cover the project-before-user shadowing semantics:
- Install satisfaction evaluated against target scope (user), with runtime governance warning
- Missing-command diagnostics identifying the controlling project origin while acknowledging shadowed user copy
- User-scope fallback when no project copy exists
These requirements directly map to the
ModuleAvailabilityStatus.SHADOWEDhandling incli.pyand the_shadowed_availabilityhelper inmodule_availability.py. The GIVEN/WHEN/THEN structure makes the scenarios testable.tests/unit/specfact_cli/test_module_not_found_error.py (2)
23-34: Clean fixture for mocking availability classification.The
absent_modulefixture correctly patchesclassify_module_availabilityat the call site (specfact_cli.cli) rather than the definition site, ensuring the mock takes effect. The lambda extractsmodule_idfrom kwargs to construct a realisticModuleAvailabilitywith dynamic recovery command.
80-102: Validates disabled-module messaging contract.This test directly validates the OpenSpec requirement that missing-command UX must distinguish disabled modules from uninstalled ones. The assertions at lines 100-102 verify:
- Positive: "is installed but disabled" appears
- Positive: Recovery command
specfact module enable ...appears- Negative: "is not installed" does not appear
The
normalized_outputon line 98 handles Rich console formatting for reliable assertions.openspec/changes/marketplace-07-module-install-state-consistency/tasks.md (1)
1-45: Comprehensive task tracking with TDD evidence requirements.The tasks document aligns with the PR implementation:
- Section 2.4 preserves the lazy-loading architectural constraint: classifier tests must not eagerly import module command code
- Section 4.2 directly maps to the
preserve_existing=Trueparameter- Section 6.2 includes
openspec validate --strictas a quality gateAll tasks are marked complete (
[x]), and TDD evidence tasks (1.7, 6.4) ensure behavioral changes have before/after validation.src/specfact_cli/modules/init/src/commands.py (1)
483-492: Good merge-based, repo-aware state refresh.Passing
base_path=repo_pathwithpreserve_existing=True, then threading the selected module ids into the refresh, fixes the earlier state-clobbering pattern without duplicating discovery logic.Also applies to: 726-744
src/specfact_cli/registry/module_discovery.py (1)
166-215: Good consolidation of project-aware discovery.The new wrapper keeps workspace-root selection repo-aware without duplicating precedence and shadowing logic across callers.
openspec/changes/marketplace-07-module-install-state-consistency/proposal.md (1)
1-39: Well-structured proposal with clear traceability.The proposal correctly identifies the root cause (mixing discovery results with state writes) and enumerates concrete behavioral changes that align with the implementation in
module_availability.pyandmodule_packages.py. The GitHub issue linkage (#533bug →#534user story →#353parent feature) provides solid traceability for OpenSpec validation.src/specfact_cli/registry/module_packages.py (2)
1445-1458: Solid merge-based state preservation implementation.The logic correctly extends newly discovered modules with persisted state rows that weren't in the current discovery view. The
sorted(state.items())ensures deterministic ordering, and the defensivestr()/bool()casts handle potential schema drift gracefully. This aligns with the design spec's "merge-based" refresh requirement that preserves user-disabled choices.
198-203: Clean conditional routing for repo-scoped discovery.The lazy import of
discover_all_modules_for_projectinside the function body maintains the file's pattern of avoiding eager module loading. The routing logic correctly delegates to the project-scoped discovery whenbase_pathis provided, implementing the design spec's "repo-aware" requirement without breaking existing callers.openspec/changes/marketplace-07-module-install-state-consistency/design.md (2)
1-84: Design decisions align well with implementation.The five decisions map directly to the code changes:
- Availability classification helper →
ModuleAvailabilityStatusenum andclassify_module_availability()inmodule_availability.py- Canonicalize module identity →
_module_id_matches()and_module_id_tail()functions- Install no-op as lifecycle reconciliation → referenced in PR objectives for
commands.py- Repo-aware merge-based init →
get_discovered_modules_for_state(base_path=..., preserve_existing=True)inmodule_packages.py- Shadowing behavior surfaced →
_shadowed_availability()withshadowed_byfieldThe open questions (auto-enable vs explicit enable, list/origin exposure) are appropriately flagged for implementation-time decisions with test documentation.
62-68: Risk mitigations are practical and well-scoped.The four risks identified (classification duplication, stale state preservation, enable-on-install surprise, startup integrity cost) each have proportionate mitigations. Particularly noteworthy: the mitigation for stale entries ("preserve only rows with explicit disabled state") aligns with the
preserve_existingimplementation that only carries forward rows not in the current discovery view.src/specfact_cli/registry/module_availability.py (3)
160-187: Exemplary contract-first public API design.The
classify_module_availabilityfunction demonstrates proper contract usage:
@beartypefor runtime type enforcement@requirewith clear precondition ("module_id or command_name required")@ensurevalidating the return type invariantThe logic flow (discover → match → shadowing check → disabled check → skip check → available) correctly implements the six-state classification without importing module command code, keeping CLI startup fast.
107-113: Shadowing detection correctly scoped to the documented confusion case.The
_shadowed_duplicatefunction only flags project-scope shadowing user/marketplace modules, which matches the design spec's focus: "When the selected command or module is unavailable because a project copy shadows a user copy, diagnostics should identify both origins." Other shadow combinations (e.g., user shadowing custom) are less confusing and correctly excluded.
46-56: Module ID canonicalization handles bare name resolution correctly.The
_module_id_tailand_module_id_matchesfunctions implement the design spec's identity canonicalization: a request likespecfact-codebasecorrectly matches the discoverednold-ai/specfact-codebaseby comparing final path segments. Thestrip()calls provide defensive handling for whitespace in user input.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/commands/test_backlog_config.py (1)
89-97:⚠️ Potential issue | 🟠 MajorIsolate precedence tests from ambient
SPECFACT_*environment variables.These tests validate fallback resolution (config/inferred), but they don’t clear relevant env vars first. If CI/local env already sets
SPECFACT_GITHUB_*orSPECFACT_ADO_*, assertions can become nondeterministic.Proposed fix
- def test_github_uses_config_when_args_none(self) -> None: + def test_github_uses_config_when_args_none(self, monkeypatch: pytest.MonkeyPatch) -> None: """When repo_owner/repo_name not passed, values from config are used.""" + monkeypatch.delenv("SPECFACT_GITHUB_REPO_OWNER", raising=False) + monkeypatch.delenv("SPECFACT_GITHUB_REPO_NAME", raising=False) + monkeypatch.delenv("SPECFACT_GITHUB_TOKEN", raising=False) with patch( "specfact_backlog.backlog.commands._load_backlog_config", return_value={"github": {"repo_owner": "myorg", "repo_name": "myrepo"}}, ): kwargs = _build_adapter_kwargs(_AdapterContext(adapter="github")) assert kwargs.get("repo_owner") == "myorg" assert kwargs.get("repo_name") == "myrepo" - def test_ado_uses_config_when_args_none(self) -> None: + def test_ado_uses_config_when_args_none(self, monkeypatch: pytest.MonkeyPatch) -> None: """When ado_org/ado_project not passed, values from config are used.""" + monkeypatch.delenv("SPECFACT_ADO_ORG", raising=False) + monkeypatch.delenv("SPECFACT_ADO_PROJECT", raising=False) + monkeypatch.delenv("SPECFACT_ADO_TEAM", raising=False) + monkeypatch.delenv("SPECFACT_ADO_TOKEN", raising=False) with patch( "specfact_backlog.backlog.commands._load_backlog_config", return_value={ "ado": {"org": "myorg", "project": "MyProject", "team": "My Team"}, }, ): kwargs = _build_adapter_kwargs(_AdapterContext(adapter="ado")) assert kwargs.get("org") == "myorg" assert kwargs.get("project") == "MyProject" assert kwargs.get("team") == "My Team" - def test_ado_uses_inferred_when_args_none(self) -> None: + def test_ado_uses_inferred_when_args_none(self, monkeypatch: pytest.MonkeyPatch) -> None: """When ado_org/ado_project not passed, inferred from git is used.""" + monkeypatch.delenv("SPECFACT_ADO_ORG", raising=False) + monkeypatch.delenv("SPECFACT_ADO_PROJECT", raising=False) + monkeypatch.delenv("SPECFACT_ADO_TEAM", raising=False) + monkeypatch.delenv("SPECFACT_ADO_TOKEN", raising=False) with ( patch( "specfact_backlog.backlog.commands._load_backlog_config", return_value={}, ),As per coding guidelines "Ensure each test is independent and repeatable with no shared state between tests" and "Tests must be meaningful and test actual functionality... be independent and repeatable".
Also applies to: 111-123, 210-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/commands/test_backlog_config.py` around lines 89 - 97, The tests (e.g., test_github_uses_config_when_args_none and the other mentioned cases) can be influenced by existing SPECFACT_* env vars; before calling _build_adapter_kwargs with an _AdapterContext(adapter="github") ensure any relevant environment variables (like SPECFACT_GITHUB_* and SPECFACT_ADO_*) are cleared or isolated so the config fallback is deterministic—use the test fixture (monkeypatch.delenv or patch.dict on os.environ) to remove SPECFACT_GITHUB_REPO_OWNER, SPECFACT_GITHUB_REPO_NAME and any SPECFACT_ADO_* entries at the start of these tests and restore/limit scope to the test only, then assert repo_owner/repo_name come from the mocked _load_backlog_config as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/commands/test_backlog_config.py`:
- Around line 89-97: The tests (e.g., test_github_uses_config_when_args_none and
the other mentioned cases) can be influenced by existing SPECFACT_* env vars;
before calling _build_adapter_kwargs with an _AdapterContext(adapter="github")
ensure any relevant environment variables (like SPECFACT_GITHUB_* and
SPECFACT_ADO_*) are cleared or isolated so the config fallback is
deterministic—use the test fixture (monkeypatch.delenv or patch.dict on
os.environ) to remove SPECFACT_GITHUB_REPO_OWNER, SPECFACT_GITHUB_REPO_NAME and
any SPECFACT_ADO_* entries at the start of these tests and restore/limit scope
to the test only, then assert repo_owner/repo_name come from the mocked
_load_backlog_config as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 473d9566-922a-4616-b520-eb9b5b930673
📒 Files selected for processing (2)
tests/unit/commands/test_backlog_config.pytests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests (Python 3.12)
🧰 Additional context used
📓 Path-based instructions (5)
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/test_*.py: Write tests first in test-driven development (TDD) using the Red-Green-Refactor cycle
Ensure each test is independent and repeatable with no shared state between tests
Organize Python imports in tests using unittest.mock for Mock and patch
Use setup_method() for test initialization and Arrange-Act-Assert pattern in test files
Use@pytest.mark.asynciodecorator for async test functions in Python
Organize test files in structure: tests/unit/, tests/integration/, tests/e2e/ by module
Files:
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.pytests/unit/commands/test_backlog_config.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/*.py: Maintain minimum 80% test coverage, with 100% coverage for critical paths in Python code
Use clear naming and self-documenting code, preferring clear names over comments
Ensure each function/class has a single clear purpose (Single Responsibility Principle)
Extract common patterns to avoid code duplication (DRY principle)
Apply SOLID object-oriented design principles in Python code
Use type hints everywhere in Python code and enable basedpyright strict mode
Use Pydantic models for data validation and serialization in Python
Use async/await for I/O operations in Python code
Use context managers for resource management in Python
Use dataclasses for simple data containers in Python
Enforce maximum line length of 120 characters in Python code
Use 4 spaces for indentation in Python code (no tabs)
Use 2 blank lines between classes and 1 blank line between methods in Python
Organize imports in order: Standard library → Third party → Local in Python files
Use snake_case for variables and functions in Python
Use PascalCase for class names in Python
Use UPPER_SNAKE_CASE for constants in Python
Use leading underscore (_) for private methods in Python classes
Use snake_case for Python file names
Enable basedpyright strict mode with strict type checking configuration in Python
Use Google-style docstrings for functions and classes in Python
Include comprehensive exception handling with specific exception types in Python code
Use logging with structured context (extra parameters) instead of print statements
Use retry logic with tenacity decorators (@retry) for operations that might fail
Use Pydantic BaseSettings for environment-based configuration in Python
Validate user input using Pydantic validators in Python models
Use@lru_cacheand Redis-based caching for expensive calculations in Python
Run code formatting with Black (120 character line length) and isort in Python
Run type checking with basedpyright on all Python files
Run linting with ruff and pylint on all Pyth...
Files:
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.pytests/unit/commands/test_backlog_config.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Tests must be meaningful and test actual functionality, cover both success and failure cases, be independent and repeatable, and have clear, descriptive names. NO EXCEPTIONS - no placeholder or empty tests.
tests/**/*.py: Trim low-value unit tests when a contract covers the same assertion (type/shape/raises on negative checks)
Delete tests that only assert input validation, datatype/shape enforcement, or raises on negative conditions now guarded by contracts and runtime typing
Convert repeated edge-case permutations into one Hypothesis property with contracts acting as oraclesSecret redaction via
LoggerSetup.redact_secretsmust be covered by unit tests
Files:
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.pytests/unit/commands/test_backlog_config.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first testing: meaningful scenarios, not redundant assertions already covered by
contracts. Flag flakiness, environment coupling, and missing coverage for changed behavior.
Files:
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.pytests/unit/commands/test_backlog_config.py
@(src|tests)/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Linting must pass with no errors using: pylint src tests
Files:
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.pytests/unit/commands/test_backlog_config.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{py,pyi}: After any code changes, follow these steps in order: (1) Apply linting and formatting to ensure code quality:hatch run format, (2) Type checking:hatch run type-check(basedpyright), (3) Contract-first approach: Runhatch run contract-testfor contract validation, (4) Run full test suite:hatch test --cover -v, (5) Verify all tests pass and contracts are satisfied, (6) Fix any issues and repeat steps until all tests pass
All public APIs must have@icontractdecorators and@beartypetype checking
Use Pydantic models for all data structures with data validation
Only write high-value comments if at all. Avoid talking to the user through comments
Files:
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.pytests/unit/commands/test_backlog_config.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to openspec/changes/**/*.md : For `/opsx:archive` (Archive change): Include module signing and cleanup in final tasks. Agents MUST run `openspec archive <change-id>` from repo root (no manual `mv` under `openspec/changes/archive/`)
🔀 Multi-repo context nold-ai/specfact-cli-modules
nold-ai/specfact-cli-modules
-
Docs referencing specfact init/profile/--install and module install CLI flows (potential consumers of CLI UX/diagnostic changes; may need wording updates to match new messaging/recovery commands):
- docs/guides/installing-modules.md — multiple examples and guidance for specfact module install and module enable behavior. [::nold-ai/specfact-cli-modules::]
- docs/getting-started/installation.md — profile/install examples and guidance for init/install flows. [::nold-ai/specfact-cli-modules::]
- docs/getting-started/choose-your-modules.md — marketplace install examples and profile/init guidance. [::nold-ai/specfact-cli-modules::]
- docs/guides/marketplace.md — multiple specfact module install examples and init profile usage. [::nold-ai/specfact-cli-modules::]
- docs/guides/workflows.md — references specfact init --profile usage. [::nold-ai/specfact-cli-modules::]
- docs/reference/commands.md — documents specfact init and specfact module install CLI signatures. [::nold-ai/specfact-cli-modules::]
- docs/team-and-enterprise/multi-repo.md — examples using specfact init --repo (relevant to repo-scoped discovery/preserve_existing behavior). [::nold-ai/specfact-cli-modules::]
- docs/guides/ai-ide-workflow.md and many bundle overview docs (backlog, codebase, code-review, spec, govern) — reference specfact init ide and recommend re-running init after upgrades/installs (relevant to init behavior and discovery of bundle resources). [::nold-ai/specfact-cli-modules::]
-
Tests/docs review harness referencing CLI commands:
- tests/unit/docs/test_docs_review.py contains assertions expecting strings like "specfact init ide" (may need update if user-facing help text changes). [::nold-ai/specfact-cli-modules::]
Summary assessment: This repository contains documentation (and documentation tests) that reference the CLI commands and UX changed by the PR (init/profile, --repo behavior, module install/enable messages). No executable code consumers were found. Reviewers should verify doc examples and recovery/enable guidance (e.g., exact enable command strings) remain accurate after the PR's messaging and recovery-command changes. [::nold-ai/specfact-cli-modules::]
🔇 Additional comments (3)
tests/unit/specfact_cli/registry/test_init_module_lifecycle_ux.py (2)
62-62: Good signature sync for discovery monkeypatches.Adding
base_pathandpreserve_existingto both stubs correctly tracks the updated discovery API and avoids false failures in these Typer CLI UX tests.Also applies to: 86-86
60-65: Consider extracting a shared discovery stub to reduce signature drift risk.These two identical lambdas duplicate the same integration seam. A shared helper will make future API changes in init/discovery less brittle across UX tests.
[ suggest_optional_refactor ]
As per coding guidelines, "Extract common patterns to avoid code duplication (DRY principle)".Refactor sketch
+def _stub_discovered_modules_for_state( + enable_ids=None, disable_ids=None, base_path=None, preserve_existing=False +): + return [{"id": "sync", "version": "0.1.0", "enabled": True}] + monkeypatch.setattr( "specfact_cli.modules.init.src.commands.get_discovered_modules_for_state", - lambda enable_ids=None, disable_ids=None, base_path=None, preserve_existing=False: [ - {"id": "sync", "version": "0.1.0", "enabled": True}, - ], + _stub_discovered_modules_for_state, )Also applies to: 84-89
tests/unit/commands/test_backlog_config.py (1)
19-23: Good adapter-contract migration in tests.Switching these paths to
_AdapterContext(...)keeps the tests aligned with the new CLI adapter boundary and validates precedence behavior through the same context object shape used by the command layer.Also applies to: 80-85, 133-137
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
CHANGELOG.md (2)
21-26:⚠️ Potential issue | 🟡 MinorExpand 0.46.5 to explicitly capture all significant change types.
Line 21-Line 26 compresses multiple release-level behaviors into a single Fixed bullet. Please add explicit Changed/Added bullets for diagnostics behavior updates and new reality-test coverage so downstream users can trace impact accurately.
Suggested changelog structure update
## [0.46.5] - 2026-04-28 ### Fixed - **Module install and init**: repair module availability, install, upgrade, uninstall, and profile-init state handling so repeated runs across repos and envs stay consistent. + +### Changed + +- **Module diagnostics behavior**: improved missing-command classification and recovery guidance for absent, disabled, skipped, shadowed, and ambiguous module states. + +### Added + +- **Reality-test coverage**: added lifecycle reality tests for install, upgrade, uninstall, and `init --profile` flows against real project fixtures.Based on learnings: "Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 21 - 26, Update the [0.46.5] entry so that changes are split into explicit categories instead of a single "Fixed" bullet: keep the existing "Fixed" note about module install/init, add a "Changed" bullet describing diagnostics behavior updates, and add an "Added" bullet noting new reality-test coverage; target the header "[0.46.5]" and replace the single Fixed list item with three separate bullets (Added, Changed, Fixed) that each succinctly capture the distinct changes.
25-25:⚠️ Potential issue | 🟡 MinorWrap the overlong bullet for markdown line-length compliance.
Line 25 exceeds the 120-character guideline and should be wrapped at word boundaries.
Suggested line wrap
-- **Module install and init**: repair module availability, install, upgrade, uninstall, and profile-init state handling so repeated runs across repos and envs stay consistent. +- **Module install and init**: repair module availability, install, upgrade, + uninstall, and profile-init state handling so repeated runs across repos and + envs stay consistent.As per coding guidelines: "Keep line length under 120 characters in markdown files"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 25, The long bullet in CHANGELOG.md ("**Module install and init**: repair module availability, install, upgrade, uninstall, and profile-init state handling so repeated runs across repos and envs stay consistent.") exceeds 120 chars; break it into multiple lines at word boundaries (e.g., split after "install and init" or between comma-separated phrases) so each Markdown line is ≤120 chars while preserving the exact wording and bold label "**Module install and init**".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/specfact_cli/modules/module_registry/src/commands.py`:
- Around line 445-452: Normalize the repository path once and reuse it for both
discovery and install-root construction: compute a single normalized project
root (e.g., normalized_repo or repo_root) by resolving the workspace root
(nearest .git) and use that value when calling
discover_all_modules_for_project(normalized_repo) and when building target_root
and the _InstallOneParams(repo=...) argument so discovery and install point at
the same root; update references to repo in discover_all_modules_for_project,
target_root construction, and the _InstallOneParams call to use this normalized
variable.
In `@src/specfact_cli/registry/module_availability.py`:
- Around line 51-77: _module_id_matches currently treats "namespace/name" the
same as short ids by using tail matching; change it so fully-qualified ids
(those containing '/') require exact equality with discovered_id. Specifically,
in _module_id_matches, after normalizing requested (requested_clean),
short-circuit to return requested_clean == discovered_id if '/' in
requested_clean (or otherwise only allow exact match for qualified ids);
otherwise preserve the existing behavior that also checks
_module_id_tail(requested_clean) == _module_id_tail(discovered_id). Keep
_entry_matches and _availability_matches logic unchanged so they benefit from
the stricter matching.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 21-26: Update the [0.46.5] entry so that changes are split into
explicit categories instead of a single "Fixed" bullet: keep the existing
"Fixed" note about module install/init, add a "Changed" bullet describing
diagnostics behavior updates, and add an "Added" bullet noting new reality-test
coverage; target the header "[0.46.5]" and replace the single Fixed list item
with three separate bullets (Added, Changed, Fixed) that each succinctly capture
the distinct changes.
- Line 25: The long bullet in CHANGELOG.md ("**Module install and init**: repair
module availability, install, upgrade, uninstall, and profile-init state
handling so repeated runs across repos and envs stay consistent.") exceeds 120
chars; break it into multiple lines at word boundaries (e.g., split after
"install and init" or between comma-separated phrases) so each Markdown line is
≤120 chars while preserving the exact wording and bold label "**Module install
and init**".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a9ef6a67-11c4-4a4d-b7ba-784f3154d69a
📒 Files selected for processing (11)
CHANGELOG.mdpyproject.tomlsetup.pysrc/__init__.pysrc/specfact_cli/__init__.pysrc/specfact_cli/modules/module_registry/module-package.yamlsrc/specfact_cli/modules/module_registry/src/commands.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/registry/module_discovery.pytests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Tests (Python 3.12)
- GitHub Check: Type Checking (basedpyright)
- GitHub Check: Linting (ruff, pylint, safe-write guard)
- GitHub Check: Contract-First CI
- GitHub Check: Compatibility (Python 3.11)
- GitHub Check: Docs Review
🧰 Additional context used
📓 Path-based instructions (15)
{src/__init__.py,pyproject.toml,setup.py}
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
{src/__init__.py,pyproject.toml,setup.py}: Update src/init.py first as primary source of truth for package version, then pyproject.toml and setup.py
Maintain version synchronization across src/init.py, pyproject.toml, and setup.py
Files:
pyproject.tomlsetup.pysrc/__init__.py
{pyproject.toml,setup.py,src/__init__.py}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Manually update version numbers in pyproject.toml, setup.py, and src/init.py when making a formal version change
Files:
pyproject.tomlsetup.pysrc/__init__.py
pyproject.toml
📄 CodeRabbit inference engine (.cursorrules)
When updating the version in
pyproject.toml, ensure it's newer than the latest PyPI version. The CI/CD pipeline will automatically publish to PyPI only if the new version is greater than the published version
Files:
pyproject.toml
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/*.py: Maintain minimum 80% test coverage, with 100% coverage for critical paths in Python code
Use clear naming and self-documenting code, preferring clear names over comments
Ensure each function/class has a single clear purpose (Single Responsibility Principle)
Extract common patterns to avoid code duplication (DRY principle)
Apply SOLID object-oriented design principles in Python code
Use type hints everywhere in Python code and enable basedpyright strict mode
Use Pydantic models for data validation and serialization in Python
Use async/await for I/O operations in Python code
Use context managers for resource management in Python
Use dataclasses for simple data containers in Python
Enforce maximum line length of 120 characters in Python code
Use 4 spaces for indentation in Python code (no tabs)
Use 2 blank lines between classes and 1 blank line between methods in Python
Organize imports in order: Standard library → Third party → Local in Python files
Use snake_case for variables and functions in Python
Use PascalCase for class names in Python
Use UPPER_SNAKE_CASE for constants in Python
Use leading underscore (_) for private methods in Python classes
Use snake_case for Python file names
Enable basedpyright strict mode with strict type checking configuration in Python
Use Google-style docstrings for functions and classes in Python
Include comprehensive exception handling with specific exception types in Python code
Use logging with structured context (extra parameters) instead of print statements
Use retry logic with tenacity decorators (@retry) for operations that might fail
Use Pydantic BaseSettings for environment-based configuration in Python
Validate user input using Pydantic validators in Python models
Use@lru_cacheand Redis-based caching for expensive calculations in Python
Run code formatting with Black (120 character line length) and isort in Python
Run type checking with basedpyright on all Python files
Run linting with ruff and pylint on all Pyth...
Files:
setup.pysrc/__init__.pytests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{py,pyi}: After any code changes, follow these steps in order: (1) Apply linting and formatting to ensure code quality:hatch run format, (2) Type checking:hatch run type-check(basedpyright), (3) Contract-first approach: Runhatch run contract-testfor contract validation, (4) Run full test suite:hatch test --cover -v, (5) Verify all tests pass and contracts are satisfied, (6) Fix any issues and repeat steps until all tests pass
All public APIs must have@icontractdecorators and@beartypetype checking
Use Pydantic models for all data structures with data validation
Only write high-value comments if at all. Avoid talking to the user through comments
Files:
setup.pysrc/__init__.pytests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
src/**/*.py: All code changes must be followed by running the full test suite using the smart test system.
All Python files in src/ and tools/ directories must have corresponding test files in tests/ directory. If you modify src/common/logger_setup.py, you MUST have tests/unit/common/test_logger_setup.py. NO EXCEPTIONS - even small changes require tests.
All new Python runtime code files must have corresponding test files created BEFORE committing the code. NO EXCEPTIONS - no code without tests.
Test Coverage Validation: Run hatch run smart-test-unit for modified files, hatch run smart-test-folder for modified directories, and hatch run smart-test-full before committing. ALL TESTS MUST PASS.
All components must support TEST_MODE=true environment variable with test-specific behavior defined as: if os.environ.get('TEST_MODE') == 'true': # test-specific behavior
Use src/common/logger_setup.py for all logging via: from common.logger_setup import get_logger; logger = get_logger(name)
Use src/common/redis_client.py with fallback for Redis operations via: from common.redis_client import get_redis_client; redis_client = get_redis_client()
Type checking must pass with no errors using: mypy .
Test coverage must meet or exceed 80% total coverage. New code must have corresponding tests. Modified code must maintain or improve coverage. Critical paths must have 100% coverage.
Use Pydantic v2 validation for all context and data schemas.Add/update contracts on new or modified public APIs, stateful classes and adapters using
icontractdecorators andbeartyperuntime type checks
src/**/*.py: Meaningful Naming — identifiers reveal intent; avoid abbreviations. Identifiers insrc/must usesnake_case(modules/functions),PascalCase(classes),UPPER_SNAKE_CASE(constants). Avoid single-letter names outside short loop variables.
KISS — keep functions and modules small and single-purpose. Maximum function length: 120 lines (Phase A error threshold). Maximum cyclomati...
Files:
src/__init__.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
@(src|tests)/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Linting must pass with no errors using: pylint src tests
Files:
src/__init__.pytests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
**/*.yaml
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
YAML files must pass linting using: hatch run yaml-lint with relaxed policy.
Files:
src/specfact_cli/modules/module_registry/module-package.yaml
**/*.{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Validate YAML configuration files locally using
hatch run yaml-lintbefore committing
**/*.{yml,yaml}: Format all YAML and workflow files usinghatch run yaml-fix-allbefore committing
Use Prettier to fix whitespace, indentation, and final newline across YAML files
Use yamllint with the repo .yamllint configuration (line-length 140, trailing spaces and final newline enforced) to lint non-workflow YAML files
Files:
src/specfact_cli/modules/module_registry/module-package.yaml
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/test_*.py: Write tests first in test-driven development (TDD) using the Red-Green-Refactor cycle
Ensure each test is independent and repeatable with no shared state between tests
Organize Python imports in tests using unittest.mock for Mock and patch
Use setup_method() for test initialization and Arrange-Act-Assert pattern in test files
Use@pytest.mark.asynciodecorator for async test functions in Python
Organize test files in structure: tests/unit/, tests/integration/, tests/e2e/ by module
Files:
tests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Tests must be meaningful and test actual functionality, cover both success and failure cases, be independent and repeatable, and have clear, descriptive names. NO EXCEPTIONS - no placeholder or empty tests.
tests/**/*.py: Trim low-value unit tests when a contract covers the same assertion (type/shape/raises on negative checks)
Delete tests that only assert input validation, datatype/shape enforcement, or raises on negative conditions now guarded by contracts and runtime typing
Convert repeated edge-case permutations into one Hypothesis property with contracts acting as oraclesSecret redaction via
LoggerSetup.redact_secretsmust be covered by unit tests
Files:
tests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first testing: meaningful scenarios, not redundant assertions already covered by
contracts. Flag flakiness, environment coupling, and missing coverage for changed behavior.
Files:
tests/unit/modules/module_registry/test_commands.pytests/unit/specfact_cli/registry/test_module_availability.py
**/*.{md,mdc}
📄 CodeRabbit inference engine (.cursor/rules/markdown-rules.mdc)
**/*.{md,mdc}: Do not use more than one consecutive blank line anywhere in the document (MD012: No Multiple Consecutive Blank Lines)
Fenced code blocks should be surrounded by blank lines (MD031: Fenced Code Blocks)
Lists should be surrounded by blank lines (MD032: Lists)
Files must end with a single empty line (MD047: Files Must End With Single Newline)
Lines should not have trailing spaces (MD009: No Trailing Spaces)
Use asterisks (**) for strong emphasis, not underscores (__) (MD050: Strong Style)
Fenced code blocks must have a language specified (MD040: Fenced Code Language)
Headers should increment by one level at a time (MD001: Header Increment)
Headers should be surrounded by blank lines (MD022: Headers Should Be Surrounded By Blank Lines)
Only one top-level header (H1) is allowed per document (MD025: Single H1 Header)
Use consistent list markers, preferring dashes (-) for unordered lists (MD004: List Style)
Nested unordered list items should be indented consistently, typically by 2 spaces (MD007: Unordered List Indentation)
Use exactly one space after the list marker (e.g., -, *, +, 1.) (MD030: Spaces After List Markers)
Use incrementing numbers for ordered lists (MD029: Ordered List Item Prefix)
Enclose bare URLs in angle brackets or format them as links (MD034: Bare URLs)
Don't use spaces immediately inside code spans (MD038: Spaces Inside Code Spans)
Use consistent indentation (usually 2 or 4 spaces) throughout markdown files
Keep line length under 120 characters in markdown files
Use reference-style links for better readability in markdown files
Use a trailing slash for directory paths in markdown files
Ensure proper escaping of special characters in markdown files
Files:
CHANGELOG.md
CHANGELOG.md
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
Include new version entries at the top of CHANGELOG.md when updating versions
Update CHANGELOG.md with all code changes as part of version control requirements.
Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change
Files:
CHANGELOG.md
**/*.md
📄 CodeRabbit inference engine (.cursorrules)
Avoid markdown linting errors (refer to markdown-rules)
Files:
CHANGELOG.md
src/specfact_cli/**/*.py
⚙️ CodeRabbit configuration file
src/specfact_cli/**/*.py: Focus on modular CLI architecture: lazy module loading, registry/bootstrap patterns, and
dependency direction. Flag breaking changes to public APIs, Pydantic models, and resource
bundling. Verify@icontract+@beartypeon public surfaces; prefer centralized logging
(get_bridge_logger) over print().
Files:
src/specfact_cli/registry/module_availability.pysrc/specfact_cli/__init__.pysrc/specfact_cli/registry/module_discovery.pysrc/specfact_cli/modules/module_registry/src/commands.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to openspec/changes/**/*.md : For `/opsx:archive` (Archive change): Include module signing and cleanup in final tasks. Agents MUST run `openspec archive <change-id>` from repo root (no manual `mv` under `openspec/changes/archive/`)
📚 Learning: 2026-03-25T21:33:15.296Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/testing-and-build-guide.mdc:0-0
Timestamp: 2026-03-25T21:33:15.296Z
Learning: Applies to {pyproject.toml,setup.py,src/__init__.py} : Manually update version numbers in pyproject.toml, setup.py, and src/__init__.py when making a formal version change
Applied to files:
pyproject.tomlsetup.pysrc/__init__.pysrc/specfact_cli/__init__.py
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to pyproject.toml : When updating the version in `pyproject.toml`, ensure it's newer than the latest PyPI version. The CI/CD pipeline will automatically publish to PyPI only if the new version is greater than the published version
Applied to files:
pyproject.toml
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to @(pyproject.toml|setup.py|src/__init__.py|src/specfact_cli/__init__.py) : Maintain synchronized versions across pyproject.toml, setup.py, src/__init__.py, and src/specfact_cli/__init__.py.
Applied to files:
pyproject.tomlsetup.pysrc/__init__.pysrc/specfact_cli/__init__.pysrc/specfact_cli/modules/module_registry/src/commands.py
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to {src/__init__.py,pyproject.toml,setup.py} : Update src/__init__.py first as primary source of truth for package version, then pyproject.toml and setup.py
Applied to files:
pyproject.tomlsetup.pysrc/__init__.pysrc/specfact_cli/__init__.py
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to {src/__init__.py,pyproject.toml,setup.py} : Maintain version synchronization across src/__init__.py, pyproject.toml, and setup.py
Applied to files:
pyproject.tomlsrc/__init__.py
📚 Learning: 2026-04-10T22:42:21.860Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-10T22:42:21.860Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
src/specfact_cli/modules/module_registry/module-package.yaml
📚 Learning: 2026-03-25T21:33:15.296Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/testing-and-build-guide.mdc:0-0
Timestamp: 2026-03-25T21:33:15.296Z
Learning: Applies to CHANGELOG.md : Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to CHANGELOG.md : Update CHANGELOG.md with all code changes as part of version control requirements.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to openspec/changes/**/*.md : For `/opsx:archive` (Archive change): Include module signing and cleanup in final tasks. Agents MUST run `openspec archive <change-id>` from repo root (no manual `mv` under `openspec/changes/archive/`)
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to docs/**/*.md : Update architecture documentation in docs/ for architecture changes, state machine documentation for FSM modifications, interface documentation for API changes, and configuration guides for configuration changes. DO NOT create internal docs in specfact-cli repo folder that should not be visible to end users; use the respective internal repository instead.
Applied to files:
src/specfact_cli/__init__.py
🔀 Multi-repo context nold-ai/specfact-cli-modules
Linked repositories findings
nold-ai/specfact-cli-modules
-
Documentation references specfact init --profile / --install and repo-scoped init usage in many places (examples that may need wording or recovery-command updates):
- docs/guides/workflows.md: lines showing "specfact init --profile solo-developer" and "specfact init ide --repo . --ide cursor" [::nold-ai/specfact-cli-modules::]
- docs/guides/marketplace.md: multiple profile/install examples (e.g., lines with "specfact init --profile solo-developer", "specfact init --install backlog,codebase") [::nold-ai/specfact-cli-modules::]
- docs/getting-started/installation.md: numerous first-run/init examples and guidance including "specfact init --profile ..." and "specfact init ide" [::nold-ai/specfact-cli-modules::]
- docs/getting-started/choose-your-modules.md: profile/install examples and module install guidance (multiple occurrences) [::nold-ai/specfact-cli-modules::]
- docs/reference/commands.md & docs/reference/module-categories.md:
specfact initandspecfact module installCLI usage examples (may cite exact enable/repair commands) [::nold-ai/specfact-cli-modules::]
-
Documentation references specfact module install and module enable/disable usage and repo/scope flags (may need to align messages with new classifier/repair behavior):
- docs/guides/installing-modules.md: full install examples and notes including "--scope project --repo /path/to/repo" and "If a module is already available locally ... install is skipped with a clear message." (relevant to changed install/enable semantics) [::nold-ai/specfact-cli-modules::]
- docs/reference/dependency-resolution.md and docs/guides/module-marketplace.md: multiple install examples and flags (relevant to availability messaging) [::nold-ai/specfact-cli-modules::]
- docs/guides/installing-modules.md: examples showing "specfact module enable backlog" (recovery guidance string appears in docs) [::nold-ai/specfact-cli-modules::]
-
docs/team-and-enterprise/multi-repo.md and other team docs include recommendations to run specfact init --profile per-repo (relevant to repo-scoped discovery/preserve_existing behavior) [::nold-ai/specfact-cli-modules::]
-
Tests that assert documentation/help strings:
- tests/unit/docs/test_docs_review.py contains an assertion expecting "specfact init ide" in combined docs output (this test may need adjustment if user-facing help text or examples change) [::nold-ai/specfact-cli-modules::]
Summary assessment: This repository contains documentation and a docs-review test that reference the CLI commands and example recovery/enable guidance changed by the PR (init/profile/--repo behavior, module install/enable messaging). Reviewers should verify documentation examples and the docs-test remain accurate with the PR's updated messaging and exact enable/repair command strings. [::nold-ai/specfact-cli-modules::]
🔇 Additional comments (9)
src/specfact_cli/__init__.py (1)
48-48: Version source stays aligned at the CLI package boundary.
__version__is consistent with the other updated version sources in this PR.Based on learnings: "Maintain synchronized versions across pyproject.toml, setup.py, src/init.py, and src/specfact_cli/init.py."
src/__init__.py (1)
6-6: Primary version source is correctly bumped.This keeps the root package version aligned with the rest of the release metadata.
As per coding guidelines: "Update src/init.py first as primary source of truth for package version, then pyproject.toml and setup.py."
setup.py (1)
10-10: Setup metadata version remains in lockstep with project version sources.Good alignment for build/publish consistency.
Based on learnings: "Maintain synchronized versions across pyproject.toml, setup.py, src/init.py, and src/specfact_cli/init.py."
pyproject.toml (1)
7-7: Version gate satisfied—no action required.Local versions are synchronized across all sources (pyproject.toml, src/init.py, src/specfact_cli/init.py, setup.py), and 0.46.6 is ahead of the published PyPI version (0.46.4), ensuring release automation will proceed correctly.
src/specfact_cli/modules/module_registry/module-package.yaml (1)
19-20: This manifest revision is correct; signatures are applied post-merge by the sign-modules workflow.The bundled module signing architecture uses a staged approach: PRs verify checksums and version bumps, while signatures are added post-merge by
sign-modules.yml. The version bumps are present (init0.1.31, module_registry0.1.22), satisfying the requirement to enforce version bumps for manifests. Unsigned manifests during PR review are expected and by design—the workflow appliesVERIFY_MODULES_PR(omitting--require-signature) for pull requests and reserves strict signature verification for protected-branch pushes.> Likely an incorrect or invalid review comment.CHANGELOG.md (1)
13-19: 0.46.6 entry placement and scope look correct.Top-of-file placement and concise Fixed classification are aligned with changelog ordering and structure expectations.
tests/unit/modules/module_registry/test_commands.py (1)
97-145: Good regression coverage for the repair path.These cases pin both the preserve-merge state write and the repo-aware
base_paththreading, which are the two easiest places for this flow to regress.Also applies to: 148-195
tests/unit/specfact_cli/registry/test_module_availability.py (1)
109-144: Nice shadowing regression.This locks in the exact
package_dir/shadowed_bycontract, which is the detail most likely to drift when discovery ordering changes.src/specfact_cli/registry/module_discovery.py (1)
176-242: Good consolidation of the project-aware discovery variants.Routing all three entrypoints through
_discover_modules()keeps root priority and duplicate handling in one place, which should make the repo-aware behavior much harder to regress.
djm81
force-pushed
the
bugfix/module-install-state-consistency
branch
from
f1ebc20 to
b52c9c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/specfact_cli/registry/test_module_availability.py (1)
1-168: 🧹 Nitpick | 🔵 TrivialSolid regression coverage for the availability classifier.
These five tests nail the critical state transitions—disabled/enabled repair, explicit
module_idprecedence over shared bundle groups, namespace isolation, compatibility skip, and project-vs-user shadowing. The monkeypatch approach keeps tests fast and deterministic without touching real filesystem state.A few architectural notes:
- Missing trailing newline at line 168 (PEP 8 / linters will flag this).
- Consider adding
@pytest.mark.unitor similar markers per coding guidelines to supporthatch run smart-test-unitfiltering.📝 Suggested fix for trailing newline
assert availability.status is ModuleAvailabilityStatus.SHADOWED assert availability.shadowed_by == project_entry.package_dir assert availability.package_dir == user_entry.package_dir +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/specfact_cli/registry/test_module_availability.py` around lines 1 - 168, The file is missing a trailing newline at EOF—add a final newline character to the end of tests/unit/specfact_cli/registry/test_module_availability.py; optionally, to support unit test filtering, import pytest and either add `@pytest.mark.unit` to the existing test functions (e.g., test_classify_installed_disabled_module_reports_disabled, test_classify_prioritizes_requested_module_id_over_shared_command_group, test_classify_fully_qualified_id_does_not_tail_match_other_namespace, test_classify_skipped_module_reports_compatibility_reason, test_project_scope_shadow_reports_shadowing_and_available_project_copy) or set module-level pytestmark = [pytest.mark.unit].
♻️ Duplicate comments (3)
CHANGELOG.md (1)
27-27:⚠️ Potential issue | 🟡 MinorWrap the 0.46.6 fixed bullet to satisfy markdown max line length.
This line is over the 120-character markdown limit and should be wrapped for lint compliance.
✂️ Proposed wrap
-- **PR follow-up**: fix project-scope module re-enable to honor `--repo` and restore reachable shadowed-module diagnostics during availability checks. +- **PR follow-up**: fix project-scope module re-enable to honor `--repo` + and restore reachable shadowed-module diagnostics during availability + checks.As per coding guidelines: "Keep line length under 120 characters in markdown files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 27, The CHANGELOG.md bullet "- **PR follow-up**: fix project-scope module re-enable to honor `--repo` and restore reachable shadowed-module diagnostics during availability checks." exceeds 120 chars; wrap this single bullet into multiple lines (keeping the leading "- **PR follow-up**:" marker and markdown formatting) so each line is under 120 characters—break after a natural clause (e.g., after "honor `--repo`," or "and restore reachable shadowed-module diagnostics") to preserve readability and lint compliance.src/specfact_cli/registry/module_availability.py (2)
14-18: 🧹 Nitpick | 🔵 TrivialPrivate helper imports create coupling—acceptable but note for future.
Importing
_check_core_compatibilityand_validate_module_dependenciescouples this classifier tomodule_packagesinternals. As noted in prior review, if these helpers evolve independently, consider promoting them to a sharedmodule_checks.pywith public contracts.For now, the coupling is acceptable given the helpers are stable and the alternative (duplicating logic) is worse.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/specfact_cli/registry/module_availability.py` around lines 14 - 18, The file imports private helpers (_check_core_compatibility and _validate_module_dependencies) from module_packages which creates tight coupling; to address this, extract these helpers into a new shared module (e.g., module_checks.py) with public function names, move the implementations from module_packages to that module, update references in this file to import the public functions from module_checks, and keep merge_module_state import as-is if it remains module_packages-specific; ensure all call sites use the new public symbols and run tests to confirm no behavior change.
22-30: 🧹 Nitpick | 🔵 Trivial
AMBIGUOUSenum value still unused—consider documenting or removing.As noted in prior review,
classify_module_availabilityhas five return paths (ABSENT, SHADOWED, DISABLED, SKIPPED, AVAILABLE) but never returnsAMBIGUOUS. This violates DRY principles and reduces contract clarity.If
AMBIGUOUSis reserved for future multi-match scenarios (e.g., when multiple modules provide the same command), add a comment documenting this reservation. Otherwise, remove it to keep the enum honest about actual states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/specfact_cli/registry/module_availability.py` around lines 22 - 30, ModuleAvailabilityStatus currently defines AMBIGUOUS but classify_module_availability never returns it; either remove AMBIGUOUS from the enum or explicitly document its intended future use. Update the enum declaration (ModuleAvailabilityStatus) to drop the AMBIGUOUS member if it’s unused, or add a brief doc comment above AMBIGUOUS explaining the reserved scenario (e.g., "reserved for future multi-provider/multi-match resolution when multiple modules claim the same command") and update any tests/docs to reflect the change; ensure classify_module_availability and any callers remain consistent with the revised enum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/pre_commit_code_review.py`:
- Around line 163-175: The conditional uses an empty if/pass then else block
around checking env.get("SPECFACT_MODULES_REPO"); change it to an inverted
conditional for clarity by checking the negative condition (e.g., if not
env.get("SPECFACT_MODULES_REPO", "").strip():) and then run the
discover_specfact_modules_repo() logic to set env["SPECFACT_MODULES_REPO"],
leaving the subsequent env.setdefault(...) calls unchanged; reference symbols:
repo_root, env, "SPECFACT_MODULES_REPO", discover_specfact_modules_repo(), and
_repo_root().
In `@src/specfact_cli/registry/module_availability.py`:
- Line 191: The file ends without a trailing newline; add a single newline
character at EOF so the final line "return
_available_or_skipped_availability(primary, enabled_map)" is followed by a
newline to satisfy PEP 8 and POSIX expectations (ensure the file terminates with
a newline).
In
`@tests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.py`:
- Line 41: The line assigning local_version is too long; split the file read and
parse steps to keep each line under 120 chars. For example, first read the
pyproject content into a variable (e.g., pyproject_text = (repo_root /
"pyproject.toml").read_text(encoding="utf-8")), then call
tomllib.loads(pyproject_text)["project"]["version"].strip() to set
local_version; update references to local_version accordingly.
- Around line 14-15: Replace the duplicated TOML parsing helper in the test by
importing and using the production parser: remove the local
_project_version_from_pyproject_bytes definition and import
read_project_version_from_pyproject_bytes from the script module that defines
it, then update any test usages to call
read_project_version_from_pyproject_bytes(content) so the test and production
code share the same parsing logic; also add the import at the top of the test
file and remove the unused tomllib import.
- Around line 49-53: Add a unit test that patches read_local_version to "1.0.0",
pyproject_version_at_git_revision to "0.9.0", and fetch_latest_pypi_version to
"0.8.0" and then calls main(["--skip-when-version-unchanged-vs", "HEAD"])
asserting it returns 0 and that fetch_latest_pypi_version was called once; use
patch.object on the module functions (read_local_version,
pyproject_version_at_git_revision, fetch_latest_pypi_version) and the test
fixture name used in other tests (e.g., mod) to mirror the provided example so
the differing-version path exercises lines 224–229 and verifies PyPI is queried.
In `@tests/unit/scripts/test_pre_commit_code_review.py`:
- Around line 341-354: Extend the
test_build_review_subprocess_env_preserves_explicit_xdg_paths to also set
explicit environment values for SEMGREP_VERSION_CACHE_PATH and SEMGREP_LOG_FILE
before calling build_review_subprocess_env, then assert that
env["SEMGREP_VERSION_CACHE_PATH"] and env["SEMGREP_LOG_FILE"] equal the explicit
values; this verifies that the setdefault-based defaults do not override
caller-provided SEMGREP_* paths.
---
Outside diff comments:
In `@tests/unit/specfact_cli/registry/test_module_availability.py`:
- Around line 1-168: The file is missing a trailing newline at EOF—add a final
newline character to the end of
tests/unit/specfact_cli/registry/test_module_availability.py; optionally, to
support unit test filtering, import pytest and either add `@pytest.mark.unit` to
the existing test functions (e.g.,
test_classify_installed_disabled_module_reports_disabled,
test_classify_prioritizes_requested_module_id_over_shared_command_group,
test_classify_fully_qualified_id_does_not_tail_match_other_namespace,
test_classify_skipped_module_reports_compatibility_reason,
test_project_scope_shadow_reports_shadowing_and_available_project_copy) or set
module-level pytestmark = [pytest.mark.unit].
---
Duplicate comments:
In `@CHANGELOG.md`:
- Line 27: The CHANGELOG.md bullet "- **PR follow-up**: fix project-scope module
re-enable to honor `--repo` and restore reachable shadowed-module diagnostics
during availability checks." exceeds 120 chars; wrap this single bullet into
multiple lines (keeping the leading "- **PR follow-up**:" marker and markdown
formatting) so each line is under 120 characters—break after a natural clause
(e.g., after "honor `--repo`," or "and restore reachable shadowed-module
diagnostics") to preserve readability and lint compliance.
In `@src/specfact_cli/registry/module_availability.py`:
- Around line 14-18: The file imports private helpers (_check_core_compatibility
and _validate_module_dependencies) from module_packages which creates tight
coupling; to address this, extract these helpers into a new shared module (e.g.,
module_checks.py) with public function names, move the implementations from
module_packages to that module, update references in this file to import the
public functions from module_checks, and keep merge_module_state import as-is if
it remains module_packages-specific; ensure all call sites use the new public
symbols and run tests to confirm no behavior change.
- Around line 22-30: ModuleAvailabilityStatus currently defines AMBIGUOUS but
classify_module_availability never returns it; either remove AMBIGUOUS from the
enum or explicitly document its intended future use. Update the enum declaration
(ModuleAvailabilityStatus) to drop the AMBIGUOUS member if it’s unused, or add a
brief doc comment above AMBIGUOUS explaining the reserved scenario (e.g.,
"reserved for future multi-provider/multi-match resolution when multiple modules
claim the same command") and update any tests/docs to reflect the change; ensure
classify_module_availability and any callers remain consistent with the revised
enum.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 357ebeb0-cba7-4784-86de-0bc8cd0154f7
📒 Files selected for processing (16)
CHANGELOG.mdpyproject.tomlscripts/pre_commit_code_review.pysetup.pysrc/__init__.pysrc/specfact_cli/__init__.pysrc/specfact_cli/analyzers/code_analyzer.pysrc/specfact_cli/modules/module_registry/module-package.yamlsrc/specfact_cli/modules/module_registry/src/commands.pysrc/specfact_cli/registry/module_availability.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pytests/unit/modules/module_registry/test_commands.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/unit/specfact_cli/registry/test_module_availability.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Compatibility (Python 3.11)
- GitHub Check: Type Checking (basedpyright)
- GitHub Check: Contract-First CI
- GitHub Check: Security Audit (pip-audit)
- GitHub Check: Tests (Python 3.12)
🧰 Additional context used
📓 Path-based instructions (16)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/*.py: Maintain minimum 80% test coverage, with 100% coverage for critical paths in Python code
Use clear naming and self-documenting code, preferring clear names over comments
Ensure each function/class has a single clear purpose (Single Responsibility Principle)
Extract common patterns to avoid code duplication (DRY principle)
Apply SOLID object-oriented design principles in Python code
Use type hints everywhere in Python code and enable basedpyright strict mode
Use Pydantic models for data validation and serialization in Python
Use async/await for I/O operations in Python code
Use context managers for resource management in Python
Use dataclasses for simple data containers in Python
Enforce maximum line length of 120 characters in Python code
Use 4 spaces for indentation in Python code (no tabs)
Use 2 blank lines between classes and 1 blank line between methods in Python
Organize imports in order: Standard library → Third party → Local in Python files
Use snake_case for variables and functions in Python
Use PascalCase for class names in Python
Use UPPER_SNAKE_CASE for constants in Python
Use leading underscore (_) for private methods in Python classes
Use snake_case for Python file names
Enable basedpyright strict mode with strict type checking configuration in Python
Use Google-style docstrings for functions and classes in Python
Include comprehensive exception handling with specific exception types in Python code
Use logging with structured context (extra parameters) instead of print statements
Use retry logic with tenacity decorators (@retry) for operations that might fail
Use Pydantic BaseSettings for environment-based configuration in Python
Validate user input using Pydantic validators in Python models
Use@lru_cacheand Redis-based caching for expensive calculations in Python
Run code formatting with Black (120 character line length) and isort in Python
Run type checking with basedpyright on all Python files
Run linting with ruff and pylint on all Pyth...
Files:
src/specfact_cli/__init__.pysrc/__init__.pysetup.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pyscripts/pre_commit_code_review.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/analyzers/code_analyzer.pytests/unit/modules/module_registry/test_commands.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/modules/module_registry/src/commands.py
src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
src/**/*.py: All code changes must be followed by running the full test suite using the smart test system.
All Python files in src/ and tools/ directories must have corresponding test files in tests/ directory. If you modify src/common/logger_setup.py, you MUST have tests/unit/common/test_logger_setup.py. NO EXCEPTIONS - even small changes require tests.
All new Python runtime code files must have corresponding test files created BEFORE committing the code. NO EXCEPTIONS - no code without tests.
Test Coverage Validation: Run hatch run smart-test-unit for modified files, hatch run smart-test-folder for modified directories, and hatch run smart-test-full before committing. ALL TESTS MUST PASS.
All components must support TEST_MODE=true environment variable with test-specific behavior defined as: if os.environ.get('TEST_MODE') == 'true': # test-specific behavior
Use src/common/logger_setup.py for all logging via: from common.logger_setup import get_logger; logger = get_logger(name)
Use src/common/redis_client.py with fallback for Redis operations via: from common.redis_client import get_redis_client; redis_client = get_redis_client()
Type checking must pass with no errors using: mypy .
Test coverage must meet or exceed 80% total coverage. New code must have corresponding tests. Modified code must maintain or improve coverage. Critical paths must have 100% coverage.
Use Pydantic v2 validation for all context and data schemas.Add/update contracts on new or modified public APIs, stateful classes and adapters using
icontractdecorators andbeartyperuntime type checks
src/**/*.py: Meaningful Naming — identifiers reveal intent; avoid abbreviations. Identifiers insrc/must usesnake_case(modules/functions),PascalCase(classes),UPPER_SNAKE_CASE(constants). Avoid single-letter names outside short loop variables.
KISS — keep functions and modules small and single-purpose. Maximum function length: 120 lines (Phase A error threshold). Maximum cyclomati...
Files:
src/specfact_cli/__init__.pysrc/__init__.pysrc/specfact_cli/analyzers/code_analyzer.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/modules/module_registry/src/commands.py
@(src|tests)/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Linting must pass with no errors using: pylint src tests
Files:
src/specfact_cli/__init__.pysrc/__init__.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/analyzers/code_analyzer.pytests/unit/modules/module_registry/test_commands.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/modules/module_registry/src/commands.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{py,pyi}: After any code changes, follow these steps in order: (1) Apply linting and formatting to ensure code quality:hatch run format, (2) Type checking:hatch run type-check(basedpyright), (3) Contract-first approach: Runhatch run contract-testfor contract validation, (4) Run full test suite:hatch test --cover -v, (5) Verify all tests pass and contracts are satisfied, (6) Fix any issues and repeat steps until all tests pass
All public APIs must have@icontractdecorators and@beartypetype checking
Use Pydantic models for all data structures with data validation
Only write high-value comments if at all. Avoid talking to the user through comments
Files:
src/specfact_cli/__init__.pysrc/__init__.pysetup.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pyscripts/pre_commit_code_review.pytests/unit/specfact_cli/registry/test_module_availability.pysrc/specfact_cli/analyzers/code_analyzer.pytests/unit/modules/module_registry/test_commands.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/modules/module_registry/src/commands.py
src/specfact_cli/**/*.py
⚙️ CodeRabbit configuration file
src/specfact_cli/**/*.py: Focus on modular CLI architecture: lazy module loading, registry/bootstrap patterns, and
dependency direction. Flag breaking changes to public APIs, Pydantic models, and resource
bundling. Verify@icontract+@beartypeon public surfaces; prefer centralized logging
(get_bridge_logger) over print().
Files:
src/specfact_cli/__init__.pysrc/specfact_cli/analyzers/code_analyzer.pysrc/specfact_cli/registry/module_availability.pysrc/specfact_cli/modules/module_registry/src/commands.py
{src/__init__.py,pyproject.toml,setup.py}
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
{src/__init__.py,pyproject.toml,setup.py}: Update src/init.py first as primary source of truth for package version, then pyproject.toml and setup.py
Maintain version synchronization across src/init.py, pyproject.toml, and setup.py
Files:
src/__init__.pypyproject.tomlsetup.py
{pyproject.toml,setup.py,src/__init__.py}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Manually update version numbers in pyproject.toml, setup.py, and src/init.py when making a formal version change
Files:
src/__init__.pypyproject.tomlsetup.py
pyproject.toml
📄 CodeRabbit inference engine (.cursorrules)
When updating the version in
pyproject.toml, ensure it's newer than the latest PyPI version. The CI/CD pipeline will automatically publish to PyPI only if the new version is greater than the published version
Files:
pyproject.toml
**/test_*.py
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
**/test_*.py: Write tests first in test-driven development (TDD) using the Red-Green-Refactor cycle
Ensure each test is independent and repeatable with no shared state between tests
Organize Python imports in tests using unittest.mock for Mock and patch
Use setup_method() for test initialization and Arrange-Act-Assert pattern in test files
Use@pytest.mark.asynciodecorator for async test functions in Python
Organize test files in structure: tests/unit/, tests/integration/, tests/e2e/ by module
Files:
tests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/modules/module_registry/test_commands.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
Tests must be meaningful and test actual functionality, cover both success and failure cases, be independent and repeatable, and have clear, descriptive names. NO EXCEPTIONS - no placeholder or empty tests.
tests/**/*.py: Trim low-value unit tests when a contract covers the same assertion (type/shape/raises on negative checks)
Delete tests that only assert input validation, datatype/shape enforcement, or raises on negative conditions now guarded by contracts and runtime typing
Convert repeated edge-case permutations into one Hypothesis property with contracts acting as oraclesSecret redaction via
LoggerSetup.redact_secretsmust be covered by unit tests
Files:
tests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/modules/module_registry/test_commands.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first testing: meaningful scenarios, not redundant assertions already covered by
contracts. Flag flakiness, environment coupling, and missing coverage for changed behavior.
Files:
tests/unit/scripts/test_pre_commit_code_review.pytests/unit/specfact_cli/modules/test_multi_module_install_uninstall.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pytests/unit/analyzers/test_code_analyzer.pytests/unit/specfact_cli/registry/test_module_availability.pytests/unit/modules/module_registry/test_commands.py
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: subprocess safety, Hatch integration, and parity with documented
quality gates (format, type-check, module signing).
Files:
scripts/pre_commit_code_review.py
**/*.yaml
📄 CodeRabbit inference engine (.cursor/rules/spec-fact-cli-rules.mdc)
YAML files must pass linting using: hatch run yaml-lint with relaxed policy.
Files:
src/specfact_cli/modules/module_registry/module-package.yaml
**/*.{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Validate YAML configuration files locally using
hatch run yaml-lintbefore committing
**/*.{yml,yaml}: Format all YAML and workflow files usinghatch run yaml-fix-allbefore committing
Use Prettier to fix whitespace, indentation, and final newline across YAML files
Use yamllint with the repo .yamllint configuration (line-length 140, trailing spaces and final newline enforced) to lint non-workflow YAML files
Files:
src/specfact_cli/modules/module_registry/module-package.yaml
**/*.{md,mdc}
📄 CodeRabbit inference engine (.cursor/rules/markdown-rules.mdc)
**/*.{md,mdc}: Do not use more than one consecutive blank line anywhere in the document (MD012: No Multiple Consecutive Blank Lines)
Fenced code blocks should be surrounded by blank lines (MD031: Fenced Code Blocks)
Lists should be surrounded by blank lines (MD032: Lists)
Files must end with a single empty line (MD047: Files Must End With Single Newline)
Lines should not have trailing spaces (MD009: No Trailing Spaces)
Use asterisks (**) for strong emphasis, not underscores (__) (MD050: Strong Style)
Fenced code blocks must have a language specified (MD040: Fenced Code Language)
Headers should increment by one level at a time (MD001: Header Increment)
Headers should be surrounded by blank lines (MD022: Headers Should Be Surrounded By Blank Lines)
Only one top-level header (H1) is allowed per document (MD025: Single H1 Header)
Use consistent list markers, preferring dashes (-) for unordered lists (MD004: List Style)
Nested unordered list items should be indented consistently, typically by 2 spaces (MD007: Unordered List Indentation)
Use exactly one space after the list marker (e.g., -, *, +, 1.) (MD030: Spaces After List Markers)
Use incrementing numbers for ordered lists (MD029: Ordered List Item Prefix)
Enclose bare URLs in angle brackets or format them as links (MD034: Bare URLs)
Don't use spaces immediately inside code spans (MD038: Spaces Inside Code Spans)
Use consistent indentation (usually 2 or 4 spaces) throughout markdown files
Keep line length under 120 characters in markdown files
Use reference-style links for better readability in markdown files
Use a trailing slash for directory paths in markdown files
Ensure proper escaping of special characters in markdown files
Files:
CHANGELOG.md
CHANGELOG.md
📄 CodeRabbit inference engine (.cursor/rules/python-github-rules.mdc)
Include new version entries at the top of CHANGELOG.md when updating versions
Update CHANGELOG.md with all code changes as part of version control requirements.
Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change
Files:
CHANGELOG.md
**/*.md
📄 CodeRabbit inference engine (.cursorrules)
Avoid markdown linting errors (refer to markdown-rules)
Files:
CHANGELOG.md
🧠 Learnings (16)
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to @(pyproject.toml|setup.py|src/__init__.py|src/specfact_cli/__init__.py) : Maintain synchronized versions across pyproject.toml, setup.py, src/__init__.py, and src/specfact_cli/__init__.py.
Applied to files:
src/specfact_cli/__init__.pysrc/__init__.pypyproject.tomlsetup.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.pysrc/specfact_cli/modules/module_registry/src/commands.py
📚 Learning: 2026-03-25T21:33:15.296Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/testing-and-build-guide.mdc:0-0
Timestamp: 2026-03-25T21:33:15.296Z
Learning: Applies to {pyproject.toml,setup.py,src/__init__.py} : Manually update version numbers in pyproject.toml, setup.py, and src/__init__.py when making a formal version change
Applied to files:
src/specfact_cli/__init__.pysrc/__init__.pypyproject.tomlsetup.pytests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.py
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to {src/__init__.py,pyproject.toml,setup.py} : Update src/__init__.py first as primary source of truth for package version, then pyproject.toml and setup.py
Applied to files:
src/specfact_cli/__init__.pysrc/__init__.pypyproject.tomltests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.py
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to {src/__init__.py,pyproject.toml,setup.py} : Maintain version synchronization across src/__init__.py, pyproject.toml, and setup.py
Applied to files:
src/specfact_cli/__init__.pysrc/__init__.pypyproject.tomltests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.py
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to pyproject.toml : When updating the version in `pyproject.toml`, ensure it's newer than the latest PyPI version. The CI/CD pipeline will automatically publish to PyPI only if the new version is greater than the published version
Applied to files:
pyproject.tomltests/integration/scripts/test_check_local_version_ahead_of_pypi_integration.py
📚 Learning: 2026-03-25T21:32:29.182Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/python-github-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:29.182Z
Learning: Applies to **/test_*.py : Organize Python imports in tests using unittest.mock for Mock and patch
Applied to files:
tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py
📚 Learning: 2026-03-31T22:38:25.299Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/clean-code-principles.mdc:0-0
Timestamp: 2026-03-31T22:38:25.299Z
Learning: Applies to tests/**/*.py : Secret redaction via `LoggerSetup.redact_secrets` must be covered by unit tests
Applied to files:
tests/unit/analyzers/test_code_analyzer.py
📚 Learning: 2026-04-10T22:41:26.528Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-10T22:41:26.528Z
Learning: Enforce the clean-code review gate through `hatch run specfact code review run --json --out .specfact/code-review.json`
Applied to files:
scripts/pre_commit_code_review.py
📚 Learning: 2026-04-10T22:42:04.372Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/session_startup_instructions.mdc:0-0
Timestamp: 2026-04-10T22:42:04.372Z
Learning: After merges shipping OpenSpec- or GitHub-related work with a sibling `specfact-cli-internal` checkout present, run wiki scripts from that sibling repo's working directory (e.g., `cd ../specfact-cli-internal && python3 scripts/wiki_openspec_gh_status.py`), not from `specfact-cli` or other directories
Applied to files:
scripts/pre_commit_code_review.pysrc/specfact_cli/modules/module_registry/src/commands.py
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: Applies to openspec/changes/**/*.md : For `/opsx:archive` (Archive change): Include module signing and cleanup in final tasks. Agents MUST run `openspec archive <change-id>` from repo root (no manual `mv` under `openspec/changes/archive/`)
Applied to files:
scripts/pre_commit_code_review.pyCHANGELOG.md
📚 Learning: 2026-04-10T22:42:21.860Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-10T22:42:21.860Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
src/specfact_cli/modules/module_registry/module-package.yaml
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to CHANGELOG.md : Update CHANGELOG.md with all code changes as part of version control requirements.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-25T21:33:15.296Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/testing-and-build-guide.mdc:0-0
Timestamp: 2026-03-25T21:33:15.296Z
Learning: Applies to CHANGELOG.md : Update CHANGELOG.md to document all significant changes under Added, Fixed, Changed, or Removed sections when making a version change
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-25T21:32:57.944Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/spec-fact-cli-rules.mdc:0-0
Timestamp: 2026-03-25T21:32:57.944Z
Learning: Applies to docs/**/*.md : Update architecture documentation in docs/ for architecture changes, state machine documentation for FSM modifications, interface documentation for API changes, and configuration guides for configuration changes. DO NOT create internal docs in specfact-cli repo folder that should not be visible to end users; use the respective internal repository instead.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-04-10T22:41:19.090Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-10T22:41:19.090Z
Learning: All development work MUST use git worktrees per AGENTS.md Git Worktree Policy. Never create branches with `git checkout -b` in the primary checkout. Create worktree from origin/dev: `git worktree add ../specfact-cli-worktrees/<type>/<slug> -b <branch-name> origin/dev` where allowed types are: `feature/`, `bugfix/`, `hotfix/`, `chore/`. Forbidden in worktrees: `dev`, `main`. After creating worktree: `cd ../specfact-cli-worktrees/<type>/<slug>`. Bootstrap Hatch in worktree: `hatch env create`. Run pre-flight checks: `hatch run smart-test-status` and `hatch run contract-test-status`. Do all implementation work from the worktree, never from primary checkout. After PR merge: cleanup with `git worktree remove`, `git branch -d`, `git worktree prune`
Applied to files:
src/specfact_cli/modules/module_registry/src/commands.py
📚 Learning: 2026-04-10T22:42:04.372Z
Learnt from: CR
Repo: nold-ai/specfact-cli PR: 0
File: .cursor/rules/session_startup_instructions.mdc:0-0
Timestamp: 2026-04-10T22:42:04.372Z
Learning: Detect repository root, branch, and worktree state before implementation
Applied to files:
src/specfact_cli/modules/module_registry/src/commands.py
🔀 Multi-repo context nold-ai/specfact-cli-modules
Findings (repo: nold-ai/specfact-cli-modules)
-
docs/guides/installing-modules.md contains examples and recovery guidance (e.g., "specfact module install ...", "specfact module enable backlog", "--scope project --repo") that must be reviewed to ensure wording and exact enable/repair command strings match the PR’s new canonical id / enable/repair behavior. [::nold-ai/specfact-cli-modules::]
-
docs/getting-started/installation.md includes many "specfact init --profile ..." and "specfact init --repo ..." examples and notes about interactive vs repo-scoped init; verify these examples and the wording about auto-enable vs preserve-disabled behavior align with the PR changes. [::nold-ai/specfact-cli-modules::]
-
docs/guides/marketplace.md and docs/getting-started/choose-your-modules.md have profile/install examples (e.g., "specfact init --profile solo-developer", "specfact init --install backlog,codebase") that should be checked for any instructions that assume prior install/no-op semantics. [::nold-ai/specfact-cli-modules::]
-
docs/team-and-enterprise/multi-repo.md and related team docs explicitly recommend using --repo for per-repo init and module actions; these should be validated against the PR’s repo-root normalization behavior to ensure guidance is accurate. [::nold-ai/specfact-cli-modules::]
-
tests/unit/docs/test_docs_review.py references CLI examples (e.g., expects "specfact init ide"); if documentation example wording or exact recovery/enable command strings change, this test will likely need updating. [::nold-ai/specfact-cli-modules::]
Overall recommendation: review and update documentation examples and the docs-review test to reflect the new availability messages, canonical module id usage, and init/install repo-scoped behavior introduced by the PR. [::nold-ai/specfact-cli-modules::]
🔇 Additional comments (29)
src/specfact_cli/modules/module_registry/module-package.yaml (1)
2-2: LGTM — Community-tier manifest correctly drops optional signature.The
integrity.signatureremoval is valid becausetier: communitydoes not require signatures percrypto_validator.py(official-tier manifests would fail validation without one). The version bump to0.1.23and updated checksum align with the behavioral changes in this PR.Also applies to: 11-11, 19-20
src/specfact_cli/analyzers/code_analyzer.py (3)
35-50: Solid architectural choice for deterministic Semgrep isolation.The repo-local runtime directories under
.specfact/ensure CLI behavior remains consistent across environments and CI runs. Thesetdefaultpattern correctly preserves any explicit overrides from the calling environment.One operational consideration: verify
.specfact/cache/and.specfact/logs/are in.gitignoreto avoid committing transient Semgrep artifacts.
478-479: LGTM — Version check now uses isolated environment.Passing the repo-local env to the
--versioncheck ensures even the availability probe doesn't pollute or depend on global Semgrep state.
583-589: LGTM — Pattern detection subprocess inherits isolated environment.Consistent with the version check, the actual Semgrep execution now uses deterministic paths. This prevents cross-contamination between repositories analyzed in the same session.
src/__init__.py (1)
6-6: Version bump to 0.46.7 — primary source of truth updated.Per coding guidelines,
src/__init__.pyis the authoritative version source. Verified this aligns with the PR's behavioral fixes for module install state consistency.pyproject.toml (1)
7-7: Version synchronized with primary source.The
0.46.7version matchessrc/__init__.pyand satisfies the CI/CD requirement for PyPI publication (newer than previous).src/specfact_cli/__init__.py (1)
48-48: Runtime package version synchronized.All four version locations now report
0.46.7:
src/__init__.py✓pyproject.toml✓src/specfact_cli/__init__.py✓setup.py(verified next)setup.py (1)
10-10: Version synchronization complete.All four canonical version sources now declare
0.46.7, satisfying the cross-file version consistency requirement. Based on learnings: "Maintain synchronized versions across pyproject.toml, setup.py, src/init.py, and src/specfact_cli/init.py."tests/unit/analyzers/test_code_analyzer.py (2)
48-59: Solid unit test for Semgrep environment builder.This test validates both the environment variable configuration and the side effect of directory creation. Good use of
tmp_pathfor test isolation.
61-79: Good integration test for env propagation to subprocess.The mock validates that the constructed environment flows through to the Semgrep subprocess. Minor observation: the assertion only checks
XDG_CONFIG_HOMEandXDG_CACHE_HOME, notSEMGREP_VERSION_CACHE_PATHorSEMGREP_LOG_FILE. Consider adding those for completeness, though the companion unit test already covers them.tests/unit/specfact_cli/modules/test_multi_module_install_uninstall.py (2)
24-25: Fixture rename removes unnecessary underscore prefix.The
autouse=Truemakes this fixture apply automatically to all tests in the module, so the leading underscore convention for "private" wasn't adding value. Good cleanup.
184-185: Mock signature updated to match refactored function.The
*argsapproach correctly extractsrequested_namefromargs[1], matching the actual_install_skip_if_already_satisfiedsignature documented in the context snippet. Thestr()wrapper handles any edge cases where the argument might not be a plain string.tests/unit/scripts/test_pre_commit_code_review.py (1)
323-329: LGTM! Solid coverage for the new XDG/Semgrep runtime directory behavior.The assertions verify all four environment variables and confirm that the created directories actually exist. This aligns well with the
_build_semgrep_envreference implementation insrc/specfact_cli/analyzers/code_analyzer.py:35-50.One minor gap: consider adding an assertion that
Path(env["SEMGREP_LOG_FILE"]).parent.is_dir()to confirm the logs directory was created (the log file itself won't exist yet, but its parent should).tests/unit/modules/module_registry/test_commands.py (4)
97-146: Well-structured test for the disabled-module re-enable flow.This test validates a critical PR objective: ensuring that
installon an already-installed-but-disabled module correctly enables it without dropping unrelated state. The captured state assertions at lines 139-144 verify the preserve-merge semantics.The
orexpression in the mock lambda (lines 119-125) is syntactically valid though unconventional; a named helper would be more explicit but this is acceptable for test mocks.
148-195: Good coverage for project-scoped re-enable with explicit--repo.This test validates that the
--scope project --repoflow correctly passes the repo path to discovery and persists the enabled state. Thebase_pathscapture at line 171 is a clean way to verify the discovery call received the expected argument.
224-253: Critical test for repo-path normalization behavior.This validates an important invariant: when
--repopoints to a nested subdirectory, the implementation should normalize to the actual repository root (detected via.git). The assertions at lines 251-252 confirm both discovery and install paths are correctly normalized.This has cross-repo impact per the linked repository findings—documentation in
specfact-cli-modulesreferences this normalization behavior, so this test serves as a regression guard.
464-472: Good isolation improvement for the uninstall guidance test.Adding
tmp_pathandmonkeypatch.chdir(tmp_path)isolates this test from the actual working directory, preventing potential flakiness or cross-test interference. This aligns with the coding guideline that tests should be independent and repeatable.scripts/pre_commit_code_review.py (2)
114-118: Consistent with the reference implementation incode_analyzer.py.This helper mirrors
_ensure_semgrep_runtime_dirfromsrc/specfact_cli/analyzers/code_analyzer.py:35-38. The duplication is acceptable here sincescripts/is meant to be standalone and shouldn't import fromsrc/specfact_cli.Consider adding a brief comment noting the intentional parallel with
code_analyzer.pyfor future maintainers.
148-176: Contract coverage is appropriate for this public API.
build_review_subprocess_env()correctly uses@beartypeand@ensureto validate the return type. Since the function has no caller-provided arguments requiring validation, the absence of@requireis acceptable. The postcondition at line 149-152 ensures all returned keys and values are strings.src/specfact_cli/modules/module_registry/src/commands.py (6)
185-193: Workspace root normalization looks correct; edge case note.The
.gittraversal correctly resolves subdirectory invocations to the workspace root. One architectural consideration: if a user runs--repo /some/pathwhere no.gitexists anywhere in the ancestry, the function falls back to the original resolved path (line 193). This is reasonable—monorepo subdirs and detached worktrees are handled—but consider logging a debug-level notice when fallback occurs for troubleshooting.
196-206: Defensive manifest read with graceful fallback—good resilience.The error handling for
OSErrorandyaml.YAMLErrorensures corrupt or missing manifests don't crash the install repair path. The fallback tofallback_namemaintains forward progress.
209-221: Enable-if-disabled repair now correctly threadsbase_path—past review addressed.The
get_discovered_modules_for_state(..., base_path=base_path, preserve_existing=True)call ensures repo-scoped modules are discovered before state is written. The verification at line 221 confirms the enable actually persisted before returningTrue.One architectural note: the helper assumes
module_idis the canonical manifest name (e.g.,nold-ai/specfact-codebase). Callers at lines 234-235 and 251-253 correctly resolve this from manifest or metadata before calling—good contract adherence.
224-261: Install-skip logic now performs lifecycle-aware enable repair—architecturally sound.The two code paths (already-installed at target root vs. discovered elsewhere) both:
- Resolve the canonical
module_idfrom manifest/metadata- Thread
base_pathappropriately (repofor project scope,Nonefor user)- Report whether state was enabled in the user message
This ensures
specfact module install backlog --scope project --repo /other/reporepairs disabled state for that specific repo's modules rather than the default discovery context.
455-472: Single-point repo normalization before discovery and install—past review fully addressed.The flow now:
- Normalizes
repoto workspace root once (line 455)- Builds
target_rootfrom normalized path (line 456)- Runs discovery against normalized path (lines 457-459)
- Threads
normalized_repothrough_InstallOneParams(line 465)This eliminates the previous inconsistency where discovery could resolve to repo-root while install wrote to a subdirectory. Clean architectural fix.
457-460: The key mismatch concern is unfounded; keying and lookups are consistent.
discovered_by_nameis keyed byentry.metadata.name, which contains only the short module name (e.g.,"specfact-codebase"), not the full canonical ID. The lookup at line 249 usesrequested_name, which is also the short name extracted from the user input via_normalize_install_module_id. Both dictionary keys and lookups reference the same short-name format; no mismatch exists. The same pattern is used consistently in the uninstall path (line 591).> Likely an incorrect or invalid review comment.src/specfact_cli/registry/module_availability.py (4)
51-58: Namespace-qualified ID matching now requires exact equality—past review addressed.The fix at lines 56-57 ensures
"owner-a/review"no longer incorrectly matches"owner-b/review". Short names like"review"still benefit from tail matching, which is the intended UX.
70-81: Match prioritization logic is architecturally correct.The three-tier fallback:
- Exact/tail
module_idmatches → return immediately- Qualified
module_idwith no matches → return empty (no command fallback)- Unqualified name → try
command_namematchingThis ensures
classify_module_availability(module_id="nold-ai/specfact-codebase")never accidentally picks upother-org/specfact-codebasevia command matching.
164-191: Contract-first public API with proper@beartypeand@icontractdecorators.The classifier follows the coding guidelines for public APIs:
@beartypefor runtime type checking@requireprecondition ensuring at least one lookup key@ensurepostcondition validating return typeThe discovery-match-classify flow is clean and testable. The
enabled_mapmerge at line 183 correctly integrates manifest data with persistedmodules.jsonstate.One suggestion: consider adding a docstring with example usage since this is a public API consumed by
cli.pyfor diagnostics.
111-117: Shadowing detection asymmetry is intentional—only project→user/marketplace is marked SHADOWED.The function correctly implements the documented module precedence hierarchy: built-in > marketplace > user scope, with project scope overriding user/marketplace during discovery. Project-scoped modules shadowing user or marketplace modules are flagged as
SHADOWEDbecause this is the case where a user's global install becomes invisible within a repository context—an actionable distinction that diagnostics should surface.Other shadow directions (e.g., user shadowing marketplace) don't require explicit detection because lower-priority sources are naturally occluded by the precedence order. This design is confirmed in OpenSpec (
user-module-root/spec.md,marketplace-07-module-install-state-consistency/design.md) and aligns with the module registry's documented precedence. The implementation matches the specification intent.
1 participant
What changed
This PR fixes the inconsistent module state behavior around install, upgrade, uninstall, and
init --profile.Root cause
Module availability checks were mixing discovery results with state writes, and the install repair path could overwrite unrelated
modules.jsonrows. That made repeated runs across different repos or environments behave unreliably.Impact
specfact module installnow repairs disabled installed modules without dropping unrelated state.specfact init --repo ... --profile/--install ...refreshes module state using repo-aware discovery while preserving unrelated rows.CHANGELOG.md.Validation
hatch run linthatch run pyteston the touched-scope suitehatch run specfact code review run --json --out .specfact/code-review.changed.json --scope changedhatch run specfact code review run --bug-hunt --json --out .specfact/code-review.bug-hunt.json --scope changedopenspec validate marketplace-07-module-install-state-consistency --stricthatch run specfact