Conversation
…ommands feat: add requirements runtime module
chore(registry): publish changed modules
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (14)
📝 WalkthroughWalkthroughAdds a new ChangesRequirements bundle implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
djm81
added
documentation
![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: e87e2d3386
ℹ️ 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: 4
🧹 Nitpick comments (2)
packages/specfact-requirements/src/specfact_requirements/requirements/runtime.py (1)
21-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded profile allowlist risks drifting from core.
KNOWN_REQUIREMENT_CONTEXT_PROFILESduplicates the canonical profile set that presumably lives inspecfact_cli.requirements.context. Since this module intentionally avoids eager-importing that core module (for CI-safe command discovery), any new/renamed profile added to core later will silently be rejected here until this list is manually updated.As per path instructions, "Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior," and as per coding guidelines, "Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)" — this duplication is a dry-category risk worth tracking (e.g., a contract test asserting parity with core's profile enum).
🤖 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 `@packages/specfact-requirements/src/specfact_requirements/requirements/runtime.py` around lines 21 - 32, The hardcoded KNOWN_REQUIREMENT_CONTEXT_PROFILES allowlist in runtime.py can drift from the canonical profile registry in specfact_cli.requirements.context. Update the validation approach so runtime.py does not maintain a separate profile list; instead, add a parity/contract check against the core profile source (or a single shared constant) while preserving the lazy-import behavior. Use KNOWN_REQUIREMENT_CONTEXT_PROFILES and the requirement context loading path as the key symbols to refactor and keep future profile additions/renames automatically aligned.Sources: Coding guidelines, Path instructions
tests/unit/specfact_requirements/test_requirements_runtime.py (1)
1-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the "paired core module unavailable" path.
Good coverage of the happy path, but there's no test that
import_requirements_file_to_bundle/validate_requirements_bundle/etc. raiseRequirementsCoreUnavailableErrorcleanly whenspecfact_cli.requirements.contextorspecfact_cli.models.requirementscan't be imported (the scenario the lazy-load fix targets).Want me to draft a test that monkeypatches
_load_requirements_module(or blocks the import like the integration test does) to assert the error surfaces correctly?
As per path instructions, "Ensure changes to adapters or bridges have targeted coverage."🤖 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/specfact_requirements/test_requirements_runtime.py` around lines 1 - 90, Add a targeted test for the lazy-load failure path in the requirements runtime helpers: cover `import_requirements_file_to_bundle`, `validate_requirements_bundle`, and any shared loader path so they raise `RequirementsCoreUnavailableError` when `specfact_cli.requirements.context` or `specfact_cli.models.requirements` cannot be imported. Use the existing `test_requirements_runtime.py` setup and either monkeypatch `_load_requirements_module` or block the import in the same way as the integration test, then assert the error surfaces cleanly from the public functions.Source: Path instructions
🤖 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 `@openspec/changes/requirements-02-module-commands/CHANGE_VALIDATION.md`:
- Line 59: The validation artifact in CHANGE_VALIDATION.md contains a
machine-specific absolute public key path, which should be removed from the
checked-in evidence. Update the command under the validation report to use a
portable placeholder or repo-relative reference instead of the /Users/dom/...
path, keeping the verify-modules-signature command readable and reproducible.
Use the existing command entry and the referenced module-signing-public.pem path
as the target spot to redact.
In
`@openspec/changes/requirements-02-module-commands/specs/backlog-adapter/spec.md`:
- Around line 4-20: Update the scenario title in backlog-adapter spec so it
matches the import/normalization flow described in the body; the current title
for the acceptance-criteria scenario still uses “extraction,” which conflicts
with the requirements import workflow. Adjust the wording in the scenario header
within the backlog-adapter spec to align with the rest of the document and keep
the scenario text about adapter source fields, normalization, and item identity
unchanged.
In `@openspec/changes/requirements-02-module-commands/TDD_EVIDENCE.md`:
- Line 34: Redact the machine-specific absolute key path in the TDD evidence
entry so it no longer exposes a local /Users/... location and can be replayed
elsewhere. Update the logged `hatch run verify-modules-signature` command in
`TDD_EVIDENCE.md` to use a portable placeholder or relative path for the
`--public-key-file` argument, matching the same redaction approach used
elsewhere in the evidence file.
In
`@packages/specfact-requirements/src/specfact_requirements/requirements/runtime.py`:
- Around line 86-90: The sidecar write in _write_requirements_sidecar is
non-atomic, unlike save_project_bundle, so update it to use the same atomic
write pattern when persisting requirements.inputs.yaml. Locate the helper by its
name and replace the direct write_text call with an atomic temp-file-and-rename
flow (or reuse the existing bundle atomic writer path in save_project_bundle),
so a crash during write cannot leave a partially written sidecar.
---
Nitpick comments:
In
`@packages/specfact-requirements/src/specfact_requirements/requirements/runtime.py`:
- Around line 21-32: The hardcoded KNOWN_REQUIREMENT_CONTEXT_PROFILES allowlist
in runtime.py can drift from the canonical profile registry in
specfact_cli.requirements.context. Update the validation approach so runtime.py
does not maintain a separate profile list; instead, add a parity/contract check
against the core profile source (or a single shared constant) while preserving
the lazy-import behavior. Use KNOWN_REQUIREMENT_CONTEXT_PROFILES and the
requirement context loading path as the key symbols to refactor and keep future
profile additions/renames automatically aligned.
In `@tests/unit/specfact_requirements/test_requirements_runtime.py`:
- Around line 1-90: Add a targeted test for the lazy-load failure path in the
requirements runtime helpers: cover `import_requirements_file_to_bundle`,
`validate_requirements_bundle`, and any shared loader path so they raise
`RequirementsCoreUnavailableError` when `specfact_cli.requirements.context` or
`specfact_cli.models.requirements` cannot be imported. Use the existing
`test_requirements_runtime.py` setup and either monkeypatch
`_load_requirements_module` or block the import in the same way as the
integration test, then assert the error surfaces cleanly from the public
functions.
🪄 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: 8f9926ff-7831-488b-9934-625436f3c7e4
⛔ Files ignored due to path filters (1)
registry/modules/specfact-requirements-0.1.1.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (33)
docs/_data/nav.ymldocs/bundles/requirements/overview.mddocs/index.mddocs/reference/commands.generated.jsondocs/reference/commands.generated.mdllms.txtopenspec/changes/requirements-02-module-commands/CHANGE_VALIDATION.mdopenspec/changes/requirements-02-module-commands/TDD_EVIDENCE.mdopenspec/changes/requirements-02-module-commands/design.mdopenspec/changes/requirements-02-module-commands/proposal.mdopenspec/changes/requirements-02-module-commands/specs/backlog-adapter/spec.mdopenspec/changes/requirements-02-module-commands/specs/module-io-contract/spec.mdopenspec/changes/requirements-02-module-commands/specs/requirements-module/spec.mdopenspec/changes/requirements-02-module-commands/tasks.mdpackages/specfact-requirements/module-package.yamlpackages/specfact-requirements/src/specfact_requirements/__init__.pypackages/specfact-requirements/src/specfact_requirements/requirements/__init__.pypackages/specfact-requirements/src/specfact_requirements/requirements/app.pypackages/specfact-requirements/src/specfact_requirements/requirements/commands.pypackages/specfact-requirements/src/specfact_requirements/requirements/runtime.pypyproject.tomlregistry/index.jsonregistry/modules/specfact-requirements-0.1.1.tar.gz.sha256registry/signatures/specfact-requirements-0.1.1.tar.sigscripts/check-bundle-imports.pyscripts/check-command-contract.pyscripts/check-docs-commands.pyscripts/check-prompt-commands.pyscripts/generate-command-overview.pytests/conftest.pytests/integration/specfact_requirements/test_command_apps.pytests/unit/docs/test_bundle_overview_cli_examples.pytests/unit/specfact_requirements/test_requirements_runtime.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
nold-ai/specfact-cli(manual) → reviewed against branchdevinstead of the default branch
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
⚠️ CI failures not shown inline (3)
GitHub Actions: Module Signature Hardening / 2_Verify Module Signatures.txt: Promote dev to main: requirements runtime module
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then�[0m
�[36;1m echo "::error::Missing SPECFACT_MODULE_PRIVATE_SIGN_KEY. Configure the secret so same-repo PRs can auto-sign module manifests."�[0m
GitHub Actions: Module Signature Hardening / Verify Module Signatures: Promote dev to main: requirements runtime module
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mBASE_REF=""�[0m
�[36;1mif [ "pull_request" = "pull_request" ]; then�[0m
�[36;1m BASE_REF="origin/main"�[0m
�[36;1melif [ "pull_request" = "workflow_dispatch" ]; then�[0m
�[36;1m BASE_REF="origin/"�[0m
�[36;1mfi�[0m
�[36;1mif [ -z "$BASE_REF" ]; then�[0m
�[36;1m echo "::error::Missing comparison base for module verification."�[0m
GitHub Actions: Module Signature Hardening / Verify Module Signatures: Promote dev to main: requirements runtime module
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then�[0m
�[36;1m echo "::error::Missing SPECFACT_MODULE_PRIVATE_SIGN_KEY. Configure the secret so same-repo PRs can auto-sign module manifests."�[0m
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
packages/specfact-requirements/src/specfact_requirements/__init__.pypackages/specfact-requirements/src/specfact_requirements/requirements/__init__.pyscripts/check-prompt-commands.pyscripts/check-docs-commands.pytests/conftest.pypackages/specfact-requirements/src/specfact_requirements/requirements/app.pytests/unit/specfact_requirements/test_requirements_runtime.pytests/integration/specfact_requirements/test_command_apps.pytests/unit/docs/test_bundle_overview_cli_examples.pypackages/specfact-requirements/src/specfact_requirements/requirements/commands.pyscripts/generate-command-overview.pyscripts/check-bundle-imports.pypackages/specfact-requirements/src/specfact_requirements/requirements/runtime.pyscripts/check-command-contract.py
packages/**/src/**/*.py
⚙️ CodeRabbit configuration file
packages/**/src/**/*.py: Focus on adapter and bridge patterns: imports from specfact_cli (models, runtime, validators),
Typer/Rich command surfaces, and clear boundaries so core upgrades do not silently break bundles.
Flag breaking assumptions about registry loading, lazy imports, and environment/mode behavior.
Files:
packages/specfact-requirements/src/specfact_requirements/__init__.pypackages/specfact-requirements/src/specfact_requirements/requirements/__init__.pypackages/specfact-requirements/src/specfact_requirements/requirements/app.pypackages/specfact-requirements/src/specfact_requirements/requirements/commands.pypackages/specfact-requirements/src/specfact_requirements/requirements/runtime.py
openspec/**/*.md
⚙️ CodeRabbit configuration file
openspec/**/*.md: Specification truth: proposal/tasks/spec deltas vs. bundle behavior, CHANGE_ORDER, and
drift vs. shipped modules or docs.
Files:
openspec/changes/requirements-02-module-commands/TDD_EVIDENCE.mdopenspec/changes/requirements-02-module-commands/specs/requirements-module/spec.mdopenspec/changes/requirements-02-module-commands/specs/module-io-contract/spec.mdopenspec/changes/requirements-02-module-commands/specs/backlog-adapter/spec.mdopenspec/changes/requirements-02-module-commands/design.mdopenspec/changes/requirements-02-module-commands/proposal.mdopenspec/changes/requirements-02-module-commands/CHANGE_VALIDATION.mdopenspec/changes/requirements-02-module-commands/tasks.md
registry/**
⚙️ CodeRabbit configuration file
registry/**: Registry and index consistency: bundle listings, version pins, and compatibility with
published module artifacts.
Files:
registry/modules/specfact-requirements-0.1.1.tar.gz.sha256registry/signatures/specfact-requirements-0.1.1.tar.sigregistry/index.json
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/check-prompt-commands.pyscripts/check-docs-commands.pyscripts/generate-command-overview.pyscripts/check-bundle-imports.pyscripts/check-command-contract.py
packages/**/module-package.yaml
⚙️ CodeRabbit configuration file
packages/**/module-package.yaml: Validate metadata: name, version, commands, dependencies, and parity with packaged src.
Call out semver and signing implications when manifests or payloads change.
Files:
packages/specfact-requirements/module-package.yaml
docs/**/*.md
⚙️ CodeRabbit configuration file
docs/**/*.md: User-facing and cross-site accuracy: Jekyll front matter, links per documentation-url-contract,
CLI examples matching bundled commands.
Files:
docs/reference/commands.generated.mddocs/bundles/requirements/overview.mddocs/index.md
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/conftest.pytests/unit/specfact_requirements/test_requirements_runtime.pytests/integration/specfact_requirements/test_command_apps.pytests/unit/docs/test_bundle_overview_cli_examples.py
🧠 Learnings (1)
📚 Learning: 2026-04-02T21:49:07.435Z
Learnt from: djm81
Repo: nold-ai/specfact-cli-modules PR: 136
File: registry/modules/specfact-spec-0.40.17.tar.gz.sha256:1-1
Timestamp: 2026-04-02T21:49:07.435Z
Learning: In nold-ai/specfact-cli-modules, module tarball signature files under registry/signatures/*.tar.sig are produced by the publish-modules GitHub Actions runner during the publish workflow (not committed to the branch). During PR pre-merge review, do not flag missing *.tar.sig files as blockers; treat signatures as publish-time artifacts.
Applied to files:
registry/signatures/specfact-requirements-0.1.1.tar.sig
🪛 ast-grep (0.44.1)
tests/unit/specfact_requirements/test_requirements_runtime.py
[info] 45-52: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"requirements": [
_requirement_record(),
{"requirement_id": "REQ-BROKEN", "schema_version": "1", "title": "Missing source refs"},
]
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 67-67: use jsonify instead of json.dumps for JSON output
Context: json.dumps([_requirement_record()])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 79-79: use jsonify instead of json.dumps for JSON output
Context: json.dumps([_requirement_record("REQ-COVERED", with_evidence=True)])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
tests/integration/specfact_requirements/test_command_apps.py
[info] 31-45: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
[
{
"schema_version": "1",
"requirement_id": "REQ-CLI",
"title": "CLI imports requirement context",
"sources": [
{
"source_type": "issue",
"locator": "https://github.com//issues/165",
}
],
}
]
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 78-78: Command coming from incoming request
Context: subprocess.run([sys.executable, "-c", code], capture_output=True, check=False, text=True, env=env)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
packages/specfact-requirements/src/specfact_requirements/requirements/commands.py
[info] 49-49: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, indent=2, sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
docs/bundles/requirements/overview.md
[style] ~90-~90: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...alidation findings and coverage gaps. - It does not expose authoring templates. - ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~91-~91: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... does not expose authoring templates. - It does not perform bidirectional backlog ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
openspec/changes/requirements-02-module-commands/proposal.md
[style] ~32-~32: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nifest and command overview wiring. - NEW: Command runtime preserves bounded co...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli (inspected at refs/heads/dev)
-
docs/architecture/implementation-status.md:26anddocs/architecture/overview.md:19say older flat command families likespecfact requirements ...are not canonical delivery targets. That conflicts with the new bundle docs/CLI surface being added in the PR and is worth checking for doc-model alignment. [::nold-ai/specfact-cli::] -
openspec/parking-lot/ai-integration-04-intent-skills/design.md:18,32,49andopenspec/parking-lot/ai-integration-04-intent-skills/specs/agent-skill-intent-workflow/spec.md:17,25,30still referencespecfact requirements capture,specfact requirements validate, andspecfact requirements trace, plus a prerequisite check forspecfact requirements --help. The PR’s new command set isimport/validate/list/coverage, so these references look stale or mismatched. [::nold-ai/specfact-cli::] -
openspec/changes/requirements-02-module-commands/specs/requirements-module/spec.md:9,16,24also still describes the olderextract/author/validate/listshape forspecfact requirements ..., which does not match the PR’s new runtime command names. [::nold-ai/specfact-cli::]
🔇 Additional comments (35)
openspec/changes/requirements-02-module-commands/proposal.md (1)
1-78: LGTM!Also applies to: 87-89
openspec/changes/requirements-02-module-commands/design.md (1)
3-22: LGTM!Also applies to: 32-66
openspec/changes/requirements-02-module-commands/specs/requirements-module/spec.md (1)
3-35: LGTM!openspec/changes/requirements-02-module-commands/specs/module-io-contract/spec.md (1)
4-20: LGTM!openspec/changes/requirements-02-module-commands/tasks.md (1)
5-36: LGTM!docs/_data/nav.yml (1)
58-62: LGTM!docs/bundles/requirements/overview.md (1)
26-36: 🗄️ Data Integrity & IntegrationVerify command-surface parity with specfact-cli architecture docs and OpenSpec specs.
This page documents
specfact requirements import|validate|list|coverageas the bundle's canonical command surface. Cross-repo findings shownold-ai/specfact-cli'sdocs/architecture/implementation-status.md/overview.mdstate that flat command families likespecfact requirements ...are not canonical delivery targets, and OpenSpec parking-lot design/spec docs plusopenspec/changes/requirements-02-module-commands/specs/requirements-module/spec.mdstill describe a different shape (capture/trace/extract/authorinstead ofimport/coverage). Confirm this page's command names/table are the source of truth going forward and that the stale cross-repo references will be updated to avoid conflicting guidance for users and agents.Also applies to: 68-75
docs/index.md (1)
72-72: LGTM!registry/signatures/specfact-requirements-0.1.1.tar.sig (1)
1-1: LGTM!Based on learnings, module tarball signature files under
registry/signatures/*.tar.sigare produced by the publish-modules CI workflow at publish time, so this artifact's presence and content are expected and not something to hand-verify in review.Source: Learnings
tests/unit/docs/test_bundle_overview_cli_examples.py (1)
58-77: LGTM!Also applies to: 132-132
docs/reference/commands.generated.json (1)
1698-1790: LGTM!docs/reference/commands.generated.md (1)
96-100: LGTM!llms.txt (1)
100-104: LGTM!registry/index.json (1)
94-108: 🗄️ Data Integrity & IntegrationVerify
core_compatibilitywas intentionally omitted.Every sibling registry entry publishes a core version range, but this new module record does not. If downstream consumers use that field for compatibility filtering, the bundle will be treated inconsistently; the current validator also won’t catch the omission.
registry/modules/specfact-requirements-0.1.1.tar.gz.sha256 (1)
1-1: LGTM!packages/specfact-requirements/src/specfact_requirements/__init__.py (1)
1-6: LGTM!packages/specfact-requirements/src/specfact_requirements/requirements/__init__.py (1)
1-7: LGTM!packages/specfact-requirements/src/specfact_requirements/requirements/app.py (1)
1-7: LGTM!packages/specfact-requirements/src/specfact_requirements/requirements/runtime.py (1)
1-20: LGTM!Also applies to: 39-65, 92-131, 146-188
packages/specfact-requirements/src/specfact_requirements/requirements/commands.py (3)
1-55: LGTM!Also applies to: 86-122
6-6: 🎯 Functional CorrectnessNo action needed for
enum.StrEnum. Python >=3.11 is declared inpyproject.toml, so this import is supported here.
56-139: 🩺 Stability & AvailabilityNo separate catch needed here — the root CLI already wraps mounted Typer apps in a global
except Exceptionhandler and prints a clean error with a nonzero exit code, soRequirementsCoreUnavailableErrordoes not reach users as a raw traceback when invoked throughspecfact_cli.cli.> Likely an incorrect or invalid review comment.tests/integration/specfact_requirements/test_command_apps.py (2)
113-121: 🎯 Functional CorrectnessThis locked-in command surface conflicts with linked OpenSpec spec deltas.
This test correctly asserts the shipped surface is
import/validate/list/coveragewith noauthorcommand. However, per linked-repository findings,openspec/changes/requirements-02-module-commands/specs/requirements-module/spec.mdstill describes anextract/author/validate/listshape, and other OpenSpec parking-lot docs still referencespecfact requirements capture/trace. That's a real docs/spec-vs-implementation drift for this module's public contract.Please confirm the OpenSpec spec deltas for this change (layer 1 in the stack) have been updated to match the shipped
import/validate/list/coveragesurface before this promotes tomain, since stale specs can mislead downstream consumers/agents relying onopenspec/changes/requirements-02-module-commands/specs/requirements-module/spec.md.Source: Linked repositories
1-112: LGTM!packages/specfact-requirements/module-package.yaml (1)
9-10: 🗄️ Data Integrity & Integration
nold-ai/specfact-projectis already listed inregistry/index.json.> Likely an incorrect or invalid review comment.scripts/check-command-contract.py (2)
214-216: 🎯 Functional CorrectnessSame decorator-order note as
scripts/check-bundle-imports.py.Same
@beartypeabove@ensureordering here; see the note onscripts/check-bundle-imports.py(lines 174-176) for the recommended-order concern from beartype's docs.
16-17: LGTM!Also applies to: 23-31, 94-101, 135-163, 214-215, 234-238
scripts/generate-command-overview.py (2)
259-261: 🎯 Functional CorrectnessSame decorator-order note as
scripts/check-bundle-imports.py.Same
@beartype-outermost/@ensure-innermost stacking as flagged onscripts/check-bundle-imports.py(lines 174-176); this is the third recurrence of the pattern in this PR.
17-18: LGTM!Also applies to: 27-40, 163-164, 208-220, 233-256
pyproject.toml (1)
92-101: LGTM!Also applies to: 130-139, 281-289
tests/conftest.py (1)
24-31: LGTM!scripts/check-bundle-imports.py (2)
18-25: LGTM!Also applies to: 62-69, 120-192
174-176: 🎯 Functional CorrectnessVerify
@beartype/@ensuredecorator order onmain().
@beartypeis applied outermost here (and incheck-command-contract.py'smain/generate-command-overview.py'sbuild_records/_check/main), wrapping theicontract.ensure-decorated function rather than the raw function. Beartype's own docs state for callable mode "@beartypeshould usually be listed last" (i.e., closest to the function). icontract'sensuredoes callfunctools.update_wrapper, so signature/annotations are likely preserved and this may work fine in practice — but it's worth confirming this order doesn't cause beartype to type-check a synthesized wrapper signature instead of the intended one, given the pattern repeats across multiple new decorators in this PR.🐻 Suggested order per beartype's own guidance
-@beartype -@ensure(lambda result: result in {0, 1}) +@ensure(lambda result: result in {0, 1}) +@beartype def main() -> int:scripts/check-docs-commands.py (1)
76-89: LGTM!scripts/check-prompt-commands.py (1)
35-48: LGTM!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/specfact_requirements/test_requirements_runtime.py (1)
66-88: 📐 Maintainability & Code Quality | 🔵 TrivialConsider parametrizing the duplicate profile-alias test.
Lines 78-88 duplicate the body of the test at lines 66-76 (
test_validate_requirements_bundle_uses_profile_aware_core_validation) with only theprofilestring differing. A@pytest.mark.parametrizeover("enterprise", "enterprise-full-stack")would consolidate this without losing coverage.
[optional_and_nitpick]♻️ Proposed consolidation
-def test_validate_requirements_bundle_uses_profile_aware_core_validation(tmp_path: Path) -> None: - source = tmp_path / "requirements.json" - source.write_text(json.dumps([_requirement_record()]), encoding="utf-8") - bundle_dir = _bundle_dir(tmp_path) - import_requirements_file_to_bundle(source, bundle_dir) - - report = validate_requirements_bundle(bundle_dir, profile="enterprise") - - assert report.status == "failed" - assert report.violations[0]["location"] == "requirements.inputs[REQ-165].evidence_links" - - -def test_validate_requirements_bundle_accepts_hyphenated_profile_alias(tmp_path: Path) -> None: - source = tmp_path / "requirements.json" - source.write_text(json.dumps([_requirement_record()]), encoding="utf-8") - bundle_dir = _bundle_dir(tmp_path) - import_requirements_file_to_bundle(source, bundle_dir) - - report = validate_requirements_bundle(bundle_dir, profile="enterprise-full-stack") - - assert report.status == "failed" - assert report.violations[0]["location"] == "requirements.inputs[REQ-165].evidence_links" +@pytest.mark.parametrize("profile", ["enterprise", "enterprise-full-stack"]) +def test_validate_requirements_bundle_uses_profile_aware_core_validation(tmp_path: Path, profile: str) -> None: + source = tmp_path / "requirements.json" + source.write_text(json.dumps([_requirement_record()]), encoding="utf-8") + bundle_dir = _bundle_dir(tmp_path) + import_requirements_file_to_bundle(source, bundle_dir) + + report = validate_requirements_bundle(bundle_dir, profile=profile) + + assert report.status == "failed" + assert report.violations[0]["location"] == "requirements.inputs[REQ-165].evidence_links"🤖 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/specfact_requirements/test_requirements_runtime.py` around lines 66 - 88, Consolidate the two near-identical bundle validation tests into a single parametrized pytest case by applying `@pytest.mark.parametrize` over the profile argument with both "enterprise" and "enterprise-full-stack". Keep the shared setup and assertions in the existing test_validate_requirements_bundle_uses_profile_aware_core_validation path so coverage stays the same while removing the duplicate test body.
🤖 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.
Nitpick comments:
In `@tests/unit/specfact_requirements/test_requirements_runtime.py`:
- Around line 66-88: Consolidate the two near-identical bundle validation tests
into a single parametrized pytest case by applying `@pytest.mark.parametrize` over
the profile argument with both "enterprise" and "enterprise-full-stack". Keep
the shared setup and assertions in the existing
test_validate_requirements_bundle_uses_profile_aware_core_validation path so
coverage stays the same while removing the duplicate test body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26807f86-b85e-4f7f-964e-0b50bebc5d14
📒 Files selected for processing (5)
packages/specfact-requirements/module-package.yamlpackages/specfact-requirements/src/specfact_requirements/requirements/commands.pypackages/specfact-requirements/src/specfact_requirements/requirements/runtime.pytests/integration/specfact_requirements/test_command_apps.pytests/unit/specfact_requirements/test_requirements_runtime.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
nold-ai/specfact-cli(manual) → reviewed against open PR#634devinstead of the default branch
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/specfact-requirements/module-package.yaml
- packages/specfact-requirements/src/specfact_requirements/requirements/commands.py
- tests/integration/specfact_requirements/test_command_apps.py
- packages/specfact-requirements/src/specfact_requirements/requirements/runtime.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
⚠️ CI failures not shown inline (3)
GitHub Actions: Module Signature Hardening / Verify Module Signatures: Promote dev to main: requirements runtime module
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then�[0m
�[36;1m echo "::error::Missing SPECFACT_MODULE_PRIVATE_SIGN_KEY. Configure the secret so same-repo PRs can auto-sign module manifests."�[0m
GitHub Actions: Module Signature Hardening / 2_Verify Module Signatures.txt: Promote dev to main: requirements runtime module
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY}" ]; then�[0m
�[36;1m echo "::error::Missing SPECFACT_MODULE_PRIVATE_SIGN_KEY. Configure the secret so same-repo PRs can auto-sign module manifests."�[0m
GitHub Actions: pr-orchestrator / 1_verify-module-signatures.txt: Promote dev to main: requirements runtime module
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mif [ -z "${SPECFACT_MODULE_PUBLIC_SIGN_KEY:-}" ] && [ -z "${SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM:-}" ]; then�[0m
�[36;1m echo "warning: no public signing key secret set; verifier must resolve key from repo/default path"�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1mVERIFY_CMD=(python scripts/verify-modules-signature.py --payload-from-filesystem --enforce-version-bump)�[0m
�[36;1m�[0m
�[36;1mif [ "pull_request" = "pull_request" ]; then�[0m
�[36;1m BASE_REF="origin/main"�[0m
�[36;1m TARGET_BRANCH="main"�[0m
�[36;1m VERIFY_CMD+=(--version-check-base "$BASE_REF")�[0m
�[36;1m if [ "$TARGET_BRANCH" = "main" ]; then�[0m
�[36;1m VERIFY_CMD+=(--require-signature)�[0m
�[36;1m fi�[0m
�[36;1melse�[0m
�[36;1m # Push/workflow_dispatch: compare against the ref tip before this push (not HEAD~1).�[0m
�[36;1m # Merge commits and signing-only updates keep the same semver; HEAD~1 is the wrong�[0m
�[36;1m # parent for three-way merges and falsely fails --enforce-version-bump.�[0m
�[36;1m BEFORE="2859604993274fc01e5280c108fcac6cfce5e115"�[0m
�[36;1m if [ -z "$BEFORE" ] || [ "$BEFORE" = "0000000000000000000000000000000000000000" ]; then�[0m
�[36;1m BEFORE="HEAD~1"�[0m
�[36;1m fi�[0m
�[36;1m VERIFY_CMD+=(--version-check-base "$BEFORE")�[0m
�[36;1m if [ "328/merge" = "main" ]; then�[0m
�[36;1m VERIFY_CMD+=(--require-signature)�[0m
�[36;1m fi�[0m
�[36;1mfi�[0m
�[36;1m�[0m
�[36;1m"${VERIFY_CMD[@]}"�[0m
shell: /usr/bin/bash -e {0}
env:
SPECFACT_MODULE_PUBLIC_SIGN_KEY: ***
SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM:
pythonLocation: /opt/hostedtoolcache/Python/3.12.13/x64
PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/3.12.13/x64/lib/pkgconfig
Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.13/x64
LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12...
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Files:
tests/unit/specfact_requirements/test_requirements_runtime.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: Contract-first and integration tests: migration suites, bundle validation, and flakiness.
Ensure changes to adapters or bridges have targeted coverage.
Files:
tests/unit/specfact_requirements/test_requirements_runtime.py
🪛 ast-grep (0.44.1)
tests/unit/specfact_requirements/test_requirements_runtime.py
[info] 79-79: use jsonify instead of json.dumps for JSON output
Context: json.dumps([_requirement_record()])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 91-91: use jsonify instead of json.dumps for JSON output
Context: json.dumps([_requirement_record("REQ-PRESERVE", with_evidence=True)])
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli (inspected at refs/heads/dev)
-
docs/architecture/implementation-status.md:26anddocs/architecture/overview.md:19sayspecfact requirements ...is not part of the canonical current CLI surface, which conflicts with this PR adding a newspecfact requirementsruntime bundle/command group. [::nold-ai/specfact-cli::] -
openspec/parking-lot/ai-integration-04-intent-skills/design.md:18,32,49,.../spec.md:17,25,30, and.../tasks.md:49-50,54still referencespecfact requirements capture,specfact requirements validate,specfact requirements trace, andspecfact requirements --help. Those references are stale relative to the newimport / validate / list / coveragesurface. [::nold-ai/specfact-cli::] -
openspec/changes/requirements-02-module-commands/specs/requirements-module/spec.md:1-40andopenspec/changes/archive/2026-04-08-cross-repo-issue-realignment/analysis.md:26,72still describe the older flatspecfact requirements ...family and/or older command names. These docs will need alignment with the new grouped runtime commands. [::nold-ai/specfact-cli::]
🔇 Additional comments (3)
tests/unit/specfact_requirements/test_requirements_runtime.py (3)
79-79: Static analysis hint is a false positive.The
use-jsonifyhints on lines 79 and 91 don't apply —json.dumpshere writes fixture JSON to a local test file, not an HTTP response body, so there's no output-encoding risk.Also applies to: 91-91
90-106: LGTM! Good targeted coverage for the atomic-save persistence contract on the requirements sidecar, per path instructions for bridge/adapter changes.
43-65: LGTM!Also applies to: 107-118
chore(modules): auto-sign module manifests
chore(registry): publish changed modules
chore(modules): auto-sign module manifests
chore(modules): auto-sign module manifests
chore(registry): publish changed modules
chore(modules): auto-sign module manifests
chore(registry): publish changed modules
1 participant
Summary
Promotes the current
devbranch tomainafter merging the requirements runtime module work.Included changes:
Highlights:
nold-ai/specfact-requirementsmodule package.requirementsruntime commands for import, validate, list, and coverage.Validation
From #326 before merge:
openspec validate requirements-02-module-commands --stricthatch run check-command-overviewhatch run check-command-contracthatch run validate-prompt-commandshatch run python scripts/check-docs-commands.py7 passedhatch run type-checkhatch run lintVerified 7 module manifest(s).PASS, score120, no findingsNotes
This is a branch promotion PR only; no local files were changed while creating it.