fix: keep core bundled registry URLs scoped#580
Conversation
|
Strix is installed on this repository, but we could not run this PR security review because this workspace does not have an active plan. If you'd like to continue receiving code reviews, you can add a payment method or manage billing here. |
📝 WalkthroughThis PR fixes bundled registry publishing so SpecFact CLI core bundled modules (init, upgrade, module-registry, bundle-mapper) are recorded and published as artifacts belonging to the core repository (nold-ai/specfact-cli) rather than inheriting the marketplace repository (nold-ai/specfact-cli-modules). User-visible behavior and CLI surface
Root cause
Changes
Contract / API impact
Testing and quality gates
Docs / changelog / signing / versioning
Summary impact
WalkthroughAdds validation to reject core bundled module entries that reference the marketplace repository, passes an explicit BUNDLED_REGISTRY_DOWNLOAD_BASE_URL through publish jobs and publish-module.py invocations, updates bundled-registry entries to point to the core releases repo, and adds unit tests for both validation and workflow configuration. ChangesBundled Module Repository Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/update-registry-index.py`:
- Around line 67-71: The current check only rejects download URLs containing
MODULES_REPOSITORY_URL_MARKER; tighten it to an explicit allowlist for core
modules by verifying that for any module_id in CORE_BUNDLED_MODULE_IDS the
download_url positively matches the expected repository (e.g., contains
"nold-ai/specfact-cli" or the exact canonical repo URL pattern you use) and
raise ValueError otherwise; update the guard around module_id and download_url
to test for the allowed repo string/pattern (instead of just excluding
MODULES_REPOSITORY_URL_MARKER) so no other hosts/paths can be persisted for
CORE_BUNDLED_MODULE_IDS.
In `@tests/unit/workflows/test_trustworthy_green_checks.py`:
- Around line 236-249: The test only checks that the bundled download base
appears somewhere in the output (expected_base in raw) which can pass if only
one publish job defines it; update the test to verify each publish job block
sets the env var and/or the download flag. Iterate over publish_blocks (the list
built from raw.split("\n\n") filtering on "scripts/publish-module.py") and for
every block assert expected_base in block (or assert "--download-base-url" and
"${BUNDLED_REGISTRY_DOWNLOAD_BASE_URL}" appear in each block); also keep the
existing special-case checks for blocks with "--index-fragment" to ensure they
include "--download-base-url" and the variable.
🪄 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: CHILL
Plan: Pro
Run ID: a9236a1d-d328-4d33-a248-102d3b84e372
📒 Files selected for processing (5)
.github/workflows/publish-modules.ymlresources/bundled-module-registry/index.jsonscripts/update-registry-index.pytests/unit/scripts/test_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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 (9)
**/*.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:
scripts/update-registry-index.pytests/unit/workflows/test_trustworthy_green_checks.pytests/unit/scripts/test_update_registry_index.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:
scripts/update-registry-index.pytests/unit/workflows/test_trustworthy_green_checks.pytests/unit/scripts/test_update_registry_index.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/update-registry-index.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/workflows/test_trustworthy_green_checks.pytests/unit/scripts/test_update_registry_index.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/workflows/test_trustworthy_green_checks.pytests/unit/scripts/test_update_registry_index.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/workflows/test_trustworthy_green_checks.pytests/unit/scripts/test_update_registry_index.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/workflows/test_trustworthy_green_checks.pytests/unit/scripts/test_update_registry_index.py
.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Validate GitHub workflow files using
hatch run lint-workflowsbefore committing
.github/workflows/*.{yml,yaml}: Use actionlint for semantic validation of GitHub Actions workflows
Format GitHub Actions workflows usinghatch run workflows-fmtand lint them withhatch run workflows-lintafter editing
Files:
.github/workflows/publish-modules.yml
.github/workflows/!(tests).{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Do not re-run the full test suite in other CI workflows; tests are enforced only in the dedicated Tests workflow (.github/workflows/tests.yml)
Files:
.github/workflows/publish-modules.yml
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI safety: secrets usage, workflow dependencies, alignment with hatch test / contract-test
gates, and action versions.
Files:
.github/workflows/publish-modules.yml
🔀 Multi-repo context nold-ai/specfact-cli-modules
nold-ai/specfact-cli-modules
- .github/workflows/publish-modules.yml — workflow exists that could publish module artifacts and influence download base URLs used for bundled snapshots. [::nold-ai/specfact-cli-modules::.github/workflows/publish-modules.yml]
- scripts/publish_module.py — module-publish helper present in this repo (validate/publish inputs). Changes in specfact-cli that alter publish flags/--download-base-url may need to be aligned with this script or vice-versa. [::nold-ai/specfact-cli-modules::scripts/publish_module.py]
- tests asserting repo URLs / docs/site expectations:
- tests/unit/test_modules_docs_site_contract.py — contains assertions referencing both nold-ai.github.io/specfact-cli-modules and nold-ai.github.io/specfact-cli (indicates tests care about which repo is referenced in docs/registry outputs). [::nold-ai/specfact-cli-modules::tests/unit/test_modules_docs_site_contract.py]
- multiple unit tests reference "nold-ai/specfact-cli" or "nold-ai/specfact-cli-modules" (search hits across tests), showing consumers/tests expect correct repo scoping for core vs modules artifacts. [::nold-ai/specfact-cli-modules::tests/]
Findings summary
- This repository contains workflows and publish helpers that are relevant to the PR's change (download-base-url and bundled registry publishing). Tests here already distinguish between specfact-cli and specfact-cli-modules URIs, so mismatches in download_url scope (core vs modules repository) could cause failing assertions here. [::nold-ai/specfact-cli-modules::]
🔇 Additional comments (4)
scripts/update-registry-index.py (1)
20-21: LGTM!Also applies to: 58-66
.github/workflows/publish-modules.yml (1)
44-45: LGTM!Also applies to: 118-120, 207-208, 275-277
resources/bundled-module-registry/index.json (1)
5-5: LGTM!Also applies to: 11-11, 17-17
tests/unit/scripts/test_update_registry_index.py (1)
8-8: LGTM!Also applies to: 11-19, 27-31, 35-38, 46-58, 89-95, 106-124
djm81
force-pushed
the
bugfix/core-registry-download-url-scope
branch
from
19bb6ae to
d54bfa4
Compare
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 (2)
tests/unit/scripts/test_update_registry_index.py (1)
108-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a positive-path assertion for core URLs.
Lines 108-134 only validate rejection cases. A regression that rejects valid core URLs (
specfact-cli/releases/download) would still pass and can break bundled core publishing.Proposed test addition
+def test_main_accepts_core_module_entry_from_core_repository(tmp_path: Path) -> None: + """Core bundled module entries should be accepted when scoped to core releases.""" + module = _load_script_module() + index_path = tmp_path / "index.json" + entry_path = tmp_path / "entry.yaml" + index_path.write_text(json.dumps({"schema_version": "1.0.0", "modules": []}), encoding="utf-8") + _write_entry_fragment( + entry_path, + "upgrade", + "0.1.20", + "https://github.com/nold-ai/specfact-cli/releases/download/upgrade-0.1.20.tar.gz", + "abc", + ) + + exit_code = module.main(["--index-path", str(index_path), "--entry-fragment", str(entry_path)]) + + assert exit_code == 0 + payload = json.loads(index_path.read_text(encoding="utf-8")) + assert payload["modules"][0]["id"] == "upgrade"As per coding guidelines, "Tests must be meaningful and test actual functionality, cover both success and failure cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/scripts/test_update_registry_index.py` around lines 108 - 134, The test only covers rejection of non-core download URLs; add a positive-path check that a valid core repository URL is accepted by module.main so regressions that erroneously reject core uploads are caught. Create a new test (or extend test_main_rejects_core_module_entry_from_unexpected_repository) that uses _write_entry_fragment to write an entry with a canonical core URL (e.g., matching specfact-cli/releases/download), call module.main(["--index-path", str(index_path), "--entry-fragment", str(entry_path)]), and assert exit_code == 0 and that index_path JSON now contains the new module entry (inspect the modules list) to verify successful acceptance.resources/bundled-module-registry/index.json (1)
22-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winbundle-mapper entry lacks download_url per OpenSpec contract.
The
bundle-mapperentry in the registry index is intentionally unpublished (nodownload_urlorchecksum_sha256). However, the OpenSpec module-publishing spec explicitly states that all registry entries MUST includeid,latest_version,download_url, andchecksum_sha256. This entry currently violates that contract.The current validation in
_validate_entry_scope()will not catch this because validation only applies to NEW entry fragments processed byupdate-registry-index.py, not to existing entries already in the index. Whenbundle-mapperis eventually published, the generated fragment will be validated correctly (requiring the URL to reference the core repository).To align with OpenSpec: either (1) only add
bundle-mapperto the index once it's published with a download URL, or (2) update the OpenSpec contract and validation logic to explicitly allow unpublished core module placeholders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/bundled-module-registry/index.json` around lines 22 - 24, The registry index contains a "bundle-mapper" entry missing required fields (download_url and checksum_sha256) and thus violates the OpenSpec; fix by either removing the "bundle-mapper" object from the index until a published artifact with a valid download_url and checksum_sha256 exists, or update the validation logic in update-registry-index.py (specifically _validate_entry_scope()) and the OpenSpec contract to explicitly allow unpublished core placeholders by adding an explicit flag (e.g., "unpublished": true) and ensuring _validate_entry_scope() accepts that flag as exempt from requiring download_url/checksum_sha256; target the "bundle-mapper" JSON object and the _validate_entry_scope() function when making changes.
🧹 Nitpick comments (1)
scripts/update-registry-index.py (1)
62-71: 💤 Low valueConsider defense-in-depth validation as suggested in past review.
The current positive-allowlist check (ensuring the URL starts with the correct prefix) addresses the core issue. However, the past review suggested also adding an explicit negative check to reject URLs containing
MODULES_REPOSITORY_URL_MARKERanywhere in the path—this would catch edge cases where a URL might start correctly but still reference the modules repo downstream (e.g., via redirect or complex path).The current implementation is functionally correct for the immediate fix, but the additional check would provide defense-in-depth at minimal cost.
🛡️ Optional hardening per past review suggestion
def _validate_entry_scope(entry: dict[str, Any]) -> None: """Reject core bundled module entries that are not scoped to the core repository.""" module_id = str(entry["id"]) download_url = str(entry["download_url"]) if module_id in CORE_BUNDLED_MODULE_IDS and not download_url.startswith(CORE_REPOSITORY_DOWNLOAD_URL_PREFIX): raise ValueError( f"Core bundled module entry {module_id!r} must reference nold-ai/specfact-cli, got {download_url!r}" ) + if module_id in CORE_BUNDLED_MODULE_IDS and "specfact-cli-modules" in download_url: + raise ValueError( + f"Core bundled module entry {module_id!r} must not reference specfact-cli-modules, got {download_url!r}" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/update-registry-index.py` around lines 62 - 71, In _validate_entry_scope, add a defense-in-depth negative check: after computing download_url (and alongside the existing startswith check using CORE_REPOSITORY_DOWNLOAD_URL_PREFIX and membership check against CORE_BUNDLED_MODULE_IDS), also reject any entry where MODULES_REPOSITORY_URL_MARKER appears anywhere in download_url by raising the same ValueError; update the conditional logic so core-bundled modules must both start with CORE_REPOSITORY_DOWNLOAD_URL_PREFIX and must not contain MODULES_REPOSITORY_URL_MARKER.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@resources/bundled-module-registry/index.json`:
- Around line 22-24: The registry index contains a "bundle-mapper" entry missing
required fields (download_url and checksum_sha256) and thus violates the
OpenSpec; fix by either removing the "bundle-mapper" object from the index until
a published artifact with a valid download_url and checksum_sha256 exists, or
update the validation logic in update-registry-index.py (specifically
_validate_entry_scope()) and the OpenSpec contract to explicitly allow
unpublished core placeholders by adding an explicit flag (e.g., "unpublished":
true) and ensuring _validate_entry_scope() accepts that flag as exempt from
requiring download_url/checksum_sha256; target the "bundle-mapper" JSON object
and the _validate_entry_scope() function when making changes.
In `@tests/unit/scripts/test_update_registry_index.py`:
- Around line 108-134: The test only covers rejection of non-core download URLs;
add a positive-path check that a valid core repository URL is accepted by
module.main so regressions that erroneously reject core uploads are caught.
Create a new test (or extend
test_main_rejects_core_module_entry_from_unexpected_repository) that uses
_write_entry_fragment to write an entry with a canonical core URL (e.g.,
matching specfact-cli/releases/download), call module.main(["--index-path",
str(index_path), "--entry-fragment", str(entry_path)]), and assert exit_code ==
0 and that index_path JSON now contains the new module entry (inspect the
modules list) to verify successful acceptance.
---
Nitpick comments:
In `@scripts/update-registry-index.py`:
- Around line 62-71: In _validate_entry_scope, add a defense-in-depth negative
check: after computing download_url (and alongside the existing startswith check
using CORE_REPOSITORY_DOWNLOAD_URL_PREFIX and membership check against
CORE_BUNDLED_MODULE_IDS), also reject any entry where
MODULES_REPOSITORY_URL_MARKER appears anywhere in download_url by raising the
same ValueError; update the conditional logic so core-bundled modules must both
start with CORE_REPOSITORY_DOWNLOAD_URL_PREFIX and must not contain
MODULES_REPOSITORY_URL_MARKER.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dca96fad-e135-479d-b51d-bfd338b07a07
📒 Files selected for processing (5)
.github/workflows/publish-modules.ymlresources/bundled-module-registry/index.jsonscripts/update-registry-index.pytests/unit/scripts/test_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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: Type Checking (basedpyright)
- GitHub Check: Linting (ruff, pylint, safe-write guard)
- GitHub Check: Contract-First CI
- GitHub Check: Tests (Python 3.12)
- GitHub Check: Compatibility (Python 3.11)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.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:
scripts/update-registry-index.pytests/unit/scripts/test_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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:
scripts/update-registry-index.pytests/unit/scripts/test_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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/update-registry-index.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/scripts/test_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.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/scripts/test_update_registry_index.pytests/unit/workflows/test_trustworthy_green_checks.py
.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Validate GitHub workflow files using
hatch run lint-workflowsbefore committing
.github/workflows/*.{yml,yaml}: Use actionlint for semantic validation of GitHub Actions workflows
Format GitHub Actions workflows usinghatch run workflows-fmtand lint them withhatch run workflows-lintafter editing
Files:
.github/workflows/publish-modules.yml
.github/workflows/!(tests).{yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/testing-and-build-guide.mdc)
Do not re-run the full test suite in other CI workflows; tests are enforced only in the dedicated Tests workflow (.github/workflows/tests.yml)
Files:
.github/workflows/publish-modules.yml
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI safety: secrets usage, workflow dependencies, alignment with hatch test / contract-test
gates, and action versions.
Files:
.github/workflows/publish-modules.yml
🔀 Multi-repo context nold-ai/specfact-cli-modules
Findings
nold-ai/specfact-cli-modules
-
Workflow that publishes modules and builds bundled registry snapshots: .github/workflows/publish-modules.yml — contains the publish job and the step that runs scripts/publish-module.py to generate registry artifacts. [::nold-ai/specfact-cli-modules::.github/workflows/publish-modules.yml]
-
Compatibility wrapper for the publish helper: scripts/publish-module.py — imports and delegates to scripts.publish_module (note wrapper name differs from internal script). [::nold-ai/specfact-cli-modules::scripts/publish-module.py]
-
Publish helper implementation: scripts/publish_module.py — performs manifest vs registry version checks and is the canonical publish validation used by the workflow. (Head of file present in repo.) [::nold-ai/specfact-cli-modules::scripts/publish_module.py]
-
Tests that assert which repository/hosts are expected:
- tests/unit/test_modules_docs_site_contract.py — contains expectations about docs/hosts and asserts absence/presence of nold-ai.github.io/specfact-cli-modules vs nold-ai.github.io/specfact-cli. Changes to download_url scoping could affect these assertions. [::nold-ai/specfact-cli-modules::tests/unit/test_modules_docs_site_contract.py]
- Multiple unit tests reference "nold-ai/specfact-cli" and "nold-ai/specfact-cli-modules" (e.g., tests under tests/unit/specfact_backlog/ and tests/unit/scripts/) — indicates consumers and tests in this repo are sensitive to repo scoping and URL strings. [::nold-ai/specfact-cli-modules::tests/]
-
Registry and publishing-related codepaths:
- registry/index.json and registry/modules are referenced and manipulated by the workflow and scripts (publish step reads registry/index.json in workflow). [::nold-ai/specfact-cli-modules::.github/workflows/publish-modules.yml]
- docs and openspec mention publish-modules workflow as canonical for updating registry; changing workflow flags (--download-base-url) or adding validation in other repo (specfact-cli) is relevant. [::nold-ai/specfact-cli-modules::docs, ::openspec]
Conclusion: specfact-cli-modules contains the workflow and publish helpers that produce bundled registry entries and tests that assert correct repo/host scoping. The PR changes in specfact-cli (explicit download base, guard rejecting core modules from unexpected repo) are directly relevant — mismatched download_url bases or altered flags will affect artifacts produced by this repo's publish workflow and may impact the tests noted above.
🔇 Additional comments (7)
tests/unit/scripts/test_update_registry_index.py (1)
8-106: LGTM!tests/unit/workflows/test_trustworthy_green_checks.py (1)
233-251: LGTM!scripts/update-registry-index.py (2)
20-21: LGTM!
58-58: LGTM!.github/workflows/publish-modules.yml (2)
44-44: LGTM!Also applies to: 207-207
118-120: LGTM!Also applies to: 275-277
resources/bundled-module-registry/index.json (1)
5-5: 🏗️ Heavy liftNo actionable changes required. The URL format in the bundled registry is correct by design and matches the schema enforced by
scripts/update-registry-index.pyandscripts/publish-module.py.The bundled registry file (
resources/bundled-module-registry/index.json) serves as metadata for the publishing workflow and cross-repo module dependencies, not as live download sources. Thedownload_urlentries follow the intentional pattern{base_url}/{module-id}-{version}.tar.gzgenerated bypublish-module.py(line:f"{download_base_url.rstrip('/')}/{tarball_name}"). The version identifier is embedded in the tarball name itself (e.g.,init-0.1.33.tar.gz), eliminating the need for an intermediate TAG directory. The repository-scoping guard inscripts/update-registry-index.pyalready validates that core modules referencehttps://github.com/nold-ai/specfact-cli/releases/download/as intended.
1 participant
Summary
Fixes the bundled registry publishing path so core modules from
specfact-clido not get snapshot entries pointing atnold-ai/specfact-cli-modules.Root Cause
publish-modules.ymlgenerated bundled registry fragments throughscripts/publish-module.pywithout overriding that script's marketplace-oriented default--download-base-url, so core module snapshots inherited the modules-repo URL base.Changes
https://github.com/nold-ai/specfact-cli/releases/download.scripts/update-registry-index.pythat rejects known core bundled module IDs when their generateddownload_urlpoints atnold-ai/specfact-cli-modules.init,upgrade, andmodule-registry.Validation
hatch run pytest tests/unit/scripts/test_update_registry_index.py tests/unit/workflows/test_trustworthy_green_checks.py -qhatch run formathatch run linthatch run workflows-linthatch run yaml-linthatch run type-check(exit 0; existing warning set only)hatch run check-version-sourcesgit diff --check