Conversation
Paired change with specfact-cli#500. Moves module signing from local interactive requirement to CI step triggered by PR approval. Adds sign-modules-on-approval.yml (packages/ manifest discovery) and relaxes verify-module-signatures to checksum-only for dev-targeting PRs. GitHub: specfact-cli-modules#185 Parent Feature: #163 (Validation And Governance Runtime Follow-Ups) Paired core: specfact-cli#500 under specfact-cli#353 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Split pr-orchestrator verify-module-signatures: require signatures only for main-targeting PRs/pushes; dev uses checksum + version enforcement. - Add sign-modules-on-approval workflow on approved reviews for PRs to dev/main: sign changed packages via sign-modules.py --changed-only, commit and push with [skip ci]. - Add workflow structure tests and TDD evidence for marketplace-06. - Exclude openspec/changes Markdown from the pre-commit review gate so task lists are not parsed as Python; update filter test. Made-with: Cursor
Made-with: Cursor
…tures) - Add scripts/pre-commit-verify-modules-signature.sh: --require-signature on main / GITHUB_REF_NAME=main only; always --payload-from-filesystem --enforce-version-bump. - Point .pre-commit-config.yaml verify hook at the wrapper; update modules-pre-commit-quality-parity spec, ci-integration delta, openspec config context, and agent quality-gates doc. - Tests for wrapper content and hook entry. Block 2 skipped locally: specfact code review requires installed bundles (specfact-codebase); verify via hatch run lint + targeted pytest. Made-with: Cursor
- Export manifests_count from discover step; show in job summary. - Checkout PR head.ref; fetch base and use merge-base for --changed-only. - Restrict job to same-repo PRs (fork signing not supported with GITHUB_TOKEN). - Tighten workflow tests and tasks/spec text; Codex P1 items resolved. Made-with: Cursor
- Run only for same-repo PRs so push targets the PR branch - Checkout head.ref for branch tip; use merge-base for --changed-only - Strengthen workflow tests; align OpenSpec tasks and TDD evidence Co-authored-by: Dom <djm81@users.noreply.github.com>
…t GITHUB_BASE_REF - Extend test_sign_modules_on_approval_trigger_and_job_filter for concurrency group + cancel-in-progress and contents: write. - Use GITHUB_BASE_REF (PR target) in pre-commit-verify-modules-signature.sh; update specs, TDD evidence, and script tests accordingly. Made-with: Cursor
…-signing feat(ci): marketplace-06 module signing on approval + branch-aware verify
📝 WalkthroughMarketplace-06 CI Module Signing: Dev→Main PromotionSummaryPromotes dev → main after merge of PR Bundle and Module Surface
Manifest and Integrity (module-package.yaml)
Cross-Repo: specfact-cli alignment and dev-deps
Docs and Documentation-URL Contract
OpenSpec Change ID & Scenario Coverage
Tests and QA
Pre-Merge Requirements & Notes
WalkthroughAdds CI-driven module signing on PR approval, makes verify-module-signatures branch-aware (require signatures only for events targeting main), introduces a branch-aware pre-commit wrapper, updates pre-commit config, tests, docs, and OpenSpec artifacts for the new signing flow. Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant SignWF as sign-modules-on-approval (Workflow)
participant SignScript as scripts/sign-modules.py
participant Orchestrator as pr-orchestrator verify step
participant Repo as PR Branch
Dev->>GH: Submit PR (and Approve)
GH->>SignWF: Trigger on pull_request_review submitted (approved)
SignWF->>Repo: Checkout PR head (same-repo only)
SignWF->>SignScript: Run sign-modules.py --changed-only --base-ref MERGE_BASE --bump-version patch --payload-from-filesystem
SignScript-->>SignWF: Produce signed manifests
SignWF->>Repo: Commit & push signed manifests (if changes)
GH->>Orchestrator: Run verify-module-signatures job
Orchestrator->>Orchestrator: Determine TARGET_BRANCH (PR base or push ref)
alt TARGET_BRANCH = main
Orchestrator->>Orchestrator: VERIFY_CMD += --require-signature
else
Orchestrator->>Orchestrator: VERIFY_CMD without --require-signature
end
Orchestrator->>SignScript: Exec verify command (payload+version checks, conditional signature)
SignScript-->>Orchestrator: Exit pass/fail
Orchestrator-->>GH: Gate result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
… wrapper - README, CI/CD guide, command chains: default verify without --require-signature; note main-equivalent checks and sign-modules-on-approval. - Module signing / security / publishing: pr-orchestrator policy, merge-base signing job, fork limitation, pre-commit-verify script. - Agent rules 20-repository-context: align essential commands with gate docs. - PR and issue templates: checklist and examples match dev vs main policy. Made-with: Cursor
![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: f9d13c6122
ℹ️ 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/marketplace-06-ci-module-signing/design.md`:
- Around line 51-54: The fenced code block containing the mapfile/MANIFESTS
snippet and python call in design.md violates MD031 (no blank lines around
fenced code); fix by adding a blank line immediately before the opening ```bash
and a blank line immediately after the closing ``` so the block is separated
from surrounding text (locate the snippet that references mapfile -t MANIFESTS
and python scripts/sign-modules.py to find the block).
- Around line 44-56: The current design mistakenly bypasses --changed-only by
passing explicit manifest paths to scripts/sign-modules.py; instead compute the
merge base (assign to MERGE_BASE using git merge-base between HEAD and
origin/${PR_BASE_REF}) and invoke scripts/sign-modules.py in changed-only mode
with --base-ref set to that MERGE_BASE and include --bump-version patch and
--payload-from-filesystem so sign-modules.py determines changed manifests itself
(update the workflow step that currently builds MANIFESTS and calls
scripts/sign-modules.py to use MERGE_BASE and the --changed-only invocation).
In
`@openspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.md`:
- Around line 36-41: The "Missing signing secret" scenario only checks
SPECFACT_MODULE_PRIVATE_SIGN_KEY; update the spec and any related validation to
also require SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE and to fail with a
clear error naming which secret is missing. Specifically, in the "Missing
signing secret" scenario and in the signing workflow validation logic (the code
path that inspects SPECFACT_MODULE_PRIVATE_SIGN_KEY), add an equivalent check
for SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE so the workflow returns a
descriptive failure message listing the missing secret(s) and ensures no partial
commits occur when either secret is unset or empty.
In `@openspec/changes/marketplace-06-ci-module-signing/tasks.md`:
- Line 1: Add a level-1 heading above the current content to satisfy
markdownlint: insert "# Tasks: marketplace-06-ci-module-signing" as the first
line (followed by a blank line) above the existing "## 1. Branch, coordination,
and issue sync" heading so the document has a proper top-level heading.
In `@scripts/pre_commit_code_review.py`:
- Around line 56-58: The current check using
normalized.startswith("openspec/changes/") and
normalized.lower().endswith(".md") unconditionally excludes all Markdown under
openspec/changes/; change it so only TDD_EVIDENCE.md remains excluded while
other files under openspec/changes/<change-id>/ should NOT be bypassed.
Concretely, update the conditional around
normalized.startswith("openspec/changes/") and
normalized.lower().endswith(".md") to inspect the basename (e.g.,
os.path.basename(normalized)) and return False only when the basename
case-insensitively equals "TDD_EVIDENCE.md"; for any other .md under
openspec/changes/ do not return False so they continue to trigger the
review-freshness logic.
In `@tests/unit/workflows/test_sign_modules_on_approval.py`:
- Around line 49-66: The test test_sign_modules_on_approval_sign_and_secrets
currently asserts the presence of the private key emptiness guard but misses the
equivalent check for the passphrase; add an assertion that the workflow contains
the emptiness test for SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE (e.g., '[ -z
"${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]') so the required-secrets
failure contract is covered alongside the existing private key assertion in that
test.
🪄 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: 26398d36-eb6c-46d4-b426-9015b0264faa
📒 Files selected for processing (21)
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.yml.pre-commit-config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/CHANGE_ORDER.mdopenspec/changes/marketplace-06-ci-module-signing/.openspec.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/proposal.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.mdopenspec/config.yamlopenspec/specs/modules-pre-commit-quality-parity/spec.mdscripts/pre-commit-verify-modules-signature.shscripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/test_pre_commit_quality_parity.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_on_approval.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). (3)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.11)
🧰 Additional context used
📓 Path-based instructions (7)
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/CHANGE_ORDER.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/proposal.md
**/*.{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:
scripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/test_pre_commit_quality_parity.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_on_approval.py
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.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/scripts/test_pre_commit_code_review.pytests/unit/test_pre_commit_quality_parity.pytests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_on_approval.py
docs/agent-rules/**/*.md
📄 CodeRabbit inference engine (.cursorrules)
Load
docs/agent-rules/INDEX.mdand canonical rule files selected by its applicability matrix
Files:
docs/agent-rules/50-quality-gates-and-review.md
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/agent-rules/50-quality-gates-and-review.md
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/sign-modules-on-approval.yml.github/workflows/pr-orchestrator.yml
🧠 Learnings (15)
📓 Common learnings
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:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
📚 Learning: 2026-04-02T21:49:11.371Z
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:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/.openspec.yamlopenspec/CHANGE_ORDER.md.pre-commit-config.yamlopenspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdtests/unit/test_pre_commit_verify_modules_signature_script.pyopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.md.github/workflows/sign-modules-on-approval.ymlscripts/pre-commit-verify-modules-signature.shtests/unit/workflows/test_pr_orchestrator_signing.pyopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.md.github/workflows/pr-orchestrator.ymltests/unit/workflows/test_sign_modules_on_approval.pyopenspec/changes/marketplace-06-ci-module-signing/proposal.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
openspec/CHANGE_ORDER.md.pre-commit-config.yamlopenspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdtests/unit/test_pre_commit_verify_modules_signature_script.pyopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.md.github/workflows/sign-modules-on-approval.ymlscripts/pre-commit-verify-modules-signature.shopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.md.github/workflows/pr-orchestrator.ymltests/unit/workflows/test_sign_modules_on_approval.pyopenspec/changes/marketplace-06-ci-module-signing/proposal.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Treat canonical rule docs (docs/agent-rules/INDEX.md) as the source of truth for worktree policy, OpenSpec gating, GitHub completeness checks, TDD order, quality gates, versioning, and documentation rules
Applied to files:
openspec/CHANGE_ORDER.mddocs/agent-rules/50-quality-gates-and-review.md
📚 Learning: 2026-04-13T10:38:15.842Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-13T10:38:15.842Z
Learning: Adhere to worktree policy, OpenSpec gating, GitHub hierarchy-cache refresh, TDD order, quality gates, versioning, and documentation rules as defined in `docs/agent-rules/`
Applied to files:
openspec/CHANGE_ORDER.mdopenspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Work belongs on feature/*, bugfix/*, hotfix/*, or chore/* branches, normally in a worktree rooted under ../specfact-cli-modules-worktrees/
Applied to files:
openspec/CHANGE_ORDER.mdopenspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/tasks.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces 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.pyopenspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Treat the clean-code compliance gate as mandatory: the review surface enforces `naming`, `kiss`, `yagni`, `dry`, and `solid` categories and blocks regressions
Applied to files:
scripts/pre_commit_code_review.pydocs/agent-rules/50-quality-gates-and-review.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
scripts/pre_commit_code_review.py
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Applied to files:
scripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pyopenspec/config.yamlopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/proposal.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h} : Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Applied to files:
scripts/pre_commit_code_review.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
.pre-commit-config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.md.github/workflows/sign-modules-on-approval.ymlscripts/pre-commit-verify-modules-signature.shopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.md.github/workflows/pr-orchestrator.ymlopenspec/changes/marketplace-06-ci-module-signing/proposal.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Run the required verification and quality gates for the touched scope before finalization
Applied to files:
openspec/config.yamldocs/agent-rules/50-quality-gates-and-review.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Perform `spec -> tests -> failing evidence -> code -> passing evidence` in that order for behavior changes
Applied to files:
openspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Full governance rules live in docs/agent-rules/; do not treat this file (GitHub Copilot Instructions) as a complete standalone handbook
Applied to files:
docs/agent-rules/50-quality-gates-and-review.md
🪛 LanguageTool
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md
[uncategorized] ~4-~4: The official name of this software platform is spelled with a capital “H”.
Context: ...ow tests → Red evidence → implement .github/workflows/sign-modules-on-approval.yml...
(GITHUB)
[uncategorized] ~8-~8: The official name of this software platform is spelled with a capital “H”.
Context: ...t_sign_modules_on_approval.py(expects.github/workflows/sign-modules-on-approval.yml`...
(GITHUB)
[uncategorized] ~62-~62: The official name of this software platform is spelled with a capital “H”.
Context: ...l) - Same-repo PRs only: job if adds `github.event.pull_request.head.repo.full_name ...
(GITHUB)
openspec/changes/marketplace-06-ci-module-signing/tasks.md
[uncategorized] ~21-~21: The official name of this software platform is spelled with a capital “H”.
Context: ...t verify by target branch - [x] 3.1 In .github/workflows/pr-orchestrator.yml, in the ...
(GITHUB)
[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”.
Context: ...job, extract the target branch fromgithub.event.pull_request.base.ref` (PR events...
(GITHUB)
[uncategorized] ~22-~22: The official name of this software platform is spelled with a capital “H”.
Context: ....pull_request.base.ref(PR events) andgithub.ref` (push events). - [x] 3.2 For eve...
(GITHUB)
[uncategorized] ~36-~36: The official name of this software platform is spelled with a capital “H”.
Context: ...dules-on-approval.yml - [x] 4.1 Create .github/workflows/sign-modules-on-approval.yml...
(GITHUB)
[uncategorized] ~38-~38: The official name of this software platform is spelled with a capital “H”.
Context: ...roved review, base dev or main, and `github.event.pull_request.head.repo.full_name ...
(GITHUB)
openspec/changes/marketplace-06-ci-module-signing/proposal.md
[uncategorized] ~17-~17: The official name of this software platform is spelled with a capital “H”.
Context: ...ts back to the PR branch. - MODIFY: .github/workflows/pr-orchestrator.yml `verify-...
(GITHUB)
🪛 markdownlint-cli2 (0.22.0)
openspec/changes/marketplace-06-ci-module-signing/tasks.md
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
openspec/changes/marketplace-06-ci-module-signing/design.md
[warning] 51-51: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 54-54: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
Repository: nold-ai/specfact-cli [::nold-ai/specfact-cli::]
Findings relevant to this PR (branch-aware verify/sign workflows + pre-commit wrapper):
-
CI workflow consumers / references
- .github/workflows/pr-orchestrator.yml — invokes verifier and sign scripts:
- python scripts/verify-modules-signature.py --require-signature ... (lines near 120 and 122) [::nold-ai/specfact-cli::]
- python scripts/sign-modules.py --changed-only --base-ref "$BASE_REF" ... (line near 736) [::nold-ai/specfact-cli::]
- .github/workflows/sign-modules.yml — contains verifier invocation variants and calls scripts/sign-modules.py (multiple places) [::nold-ai/specfact-cli::]
- .github/workflows/publish-modules.yml — calls python scripts/sign-modules.py --payload-from-filesystem "$MANIFEST" [::nold-ai/specfact-cli::]
- .github/workflows/pr-orchestrator.yml — invokes verifier and sign scripts:
-
Local tooling / scripts that call or must remain compatible
- scripts/sign-modules.py — signing implementation and used by CI and tooling; referenced by many workflows and docs (scripts/sign-module.sh also shells out to this) [::nold-ai/specfact-cli::]
- scripts/verify-modules-signature.py — verifier expected by workflows and docs (numerous references across docs and tests). Changes to how workflows build/append flags affect how this script is invoked (flags: --require-signature, --enforce-version-bump, --payload-from-filesystem, --version-check-base) [::nold-ai/specfact-cli::]
- scripts/sign-module.sh and scripts/publish-module.py — call sign-modules.py and rely on its CLI flags/behavior [::nold-ai/specfact-cli::]
- scripts/pre-commit-smart-checks.sh & scripts/setup-git-hooks.sh — previously reference hatch run ./scripts/verify-modules-signature.py --require-signature (pre-commit flow) and will be affected by pre-commit wrapper replacement [::nold-ai/specfact-cli::]
-
Tests that expect specific CLI invocations or workflow shapes
- tests/unit/specfact_cli/registry/test_signing_artifacts.py — asserts presence of scripts/verify-modules-signature.py and checks for "verify-modules-signature.py --require-signature" in content (will need alignment if invocation differs) [::nold-ai/specfact-cli::]
- tests/unit/workflows/test_trustworthy_green_checks.py — inspects sign/verify workflows (sign-modules workflow) [::nold-ai/specfact-cli::]
- New/updated unit tests in this PR (described in PR) target:
- .github/workflows/pr-orchestrator.yml contents (branch-aware VERIFY_CMD array and conditional --require-signature)
- .github/workflows/sign-modules-on-approval.yml workflow behavior and steps
- scripts/pre-commit-verify-modules-signature.sh contents and pre-commit entry
Ensure these tests align with existing tests that expect verifier invocations (e.g., test_signing_artifacts.py) [::nold-ai/specfact-cli::]
-
Code that constructs signing/verification payloads
- src/specfact_cli/registry/module_installer.py — contains logic "Build payload identical to scripts/sign-modules.py so verification matches after signing" (implies coupling to sign/verify payload shape and flags) — changes in how/when flags like --payload-from-filesystem or --version-check-base are supplied could affect parity expectations here [::nold-ai/specfact-cli::]
-
Documentation and OpenSpec references
- Many docs reference running verify-modules-signature.py with --require-signature and related flags (docs/reference/module-security.md, docs/guides/module-signing-and-key-rotation.md, openspec/*). The PR already updates docs to reflect branch-aware behavior; ensure doc changes remain consistent with workflow changes [::nold-ai/specfact-cli::]
Relevant impact summary
- The repository contains many direct consumers of scripts/sign-modules.py and scripts/verify-modules-signature.py (workflows, scripts, docs, and tests). The PR’s change to how workflows build the VERIFY_CMD array and conditional append of --require-signature must keep the verifier script’s CLI surface unchanged (flags present/absent as before) — callers are numerous and tests assert specific flag presence.
- src/specfact_cli/registry/module_installer.py and tests/test_signing_artifacts.py are tightly coupled to signing/verification behavior; verify that unit tests expecting a fixed invocation string are updated to accept the new array-based invocation or preserved behavior.
🔇 Additional comments (25)
tests/unit/scripts/test_pre_commit_code_review.py (1)
47-51: Downstream test update acknowledgedThis assertion is consistent with the new filter behavior; no separate issue beyond the root-cause concern already raised in
scripts/pre_commit_code_review.py.openspec/specs/modules-pre-commit-quality-parity/spec.md (1)
8-16: Spec-to-implementation parity looks correctThe branch-aware
--require-signaturerule and always-on--payload-from-filesystem --enforce-version-bumpwording aligns with the wrapper and CI policy split.openspec/changes/marketplace-06-ci-module-signing/.openspec.yaml (1)
1-2: Metadata addition is clean
schemaandcreatedfields are consistent with a new OpenSpec change artifact.openspec/CHANGE_ORDER.md (1)
82-87: CHANGE_ORDER entry is well-formedThe new marketplace order row captures scope and blocker linkage cleanly.
.pre-commit-config.yaml (1)
9-9: Wrapper hook wiring is correctSwitching the entrypoint to the branch-aware wrapper cleanly mirrors CI behavior.
openspec/config.yaml (1)
32-35: Config context update matches runtime behaviorThe documented branch-aware pre-commit signature policy aligns with the implemented wrapper and hook chain.
tests/unit/test_pre_commit_quality_parity.py (1)
88-103: Targeted guardrail test is solidThis adds precise regression coverage for the pre-commit adapter boundary (
verify-module-signatures→ branch-aware wrapper).docs/agent-rules/50-quality-gates-and-review.md (2)
46-46: LGTM — branch-aware signature gate documentation aligns with CI and pre-commit wrapper.The updated command accurately documents the split policy:
--payload-from-filesystem --enforce-version-bumpalways, with--require-signatureconditional onmaintarget. This matches the implementation inpr-orchestrator.yml(Lines 99-102) andscripts/pre-commit-verify-modules-signature.sh(Lines 19-23).
54-54: Pre-commit ordering and wrapper reference is correct.The documentation correctly identifies that the wrapper script handles the conditional
--require-signaturebased on branch context (mainlocally orGITHUB_BASE_REF=mainin Actions), maintaining parity between local and CI verification behavior.scripts/pre-commit-verify-modules-signature.sh (2)
1-24: LGTM — branch-aware wrapper correctly implements CI parity.The script properly:
- Prioritizes
GITHUB_BASE_REFfor Actions PR contexts before falling back to local branch detection- Only requires signatures when targeting
main- Uses
execso the wrapper's exit code reflects the verifier's result- Always includes
--payload-from-filesystem --enforce-version-bumpper the specThis aligns with the design constraint in
openspec/changes/marketplace-06-ci-module-signing/design.mdrequiring signature enforcement split by target branch.
9-9: Minor: Detached HEAD returns empty_branch, which safely falls through to no signature requirement.When in detached HEAD state (common during CI rebases or checkouts by SHA),
git rev-parse --abbrev-ref HEADreturnsHEAD(not empty), so the|| trueguard handles the error case while detached HEAD produces a non-matching branch name. This is acceptable behavior sinceGITHUB_BASE_REFshould be set in CI contexts where this matters.tests/unit/test_pre_commit_verify_modules_signature_script.py (1)
9-27: LGTM — test validates the branch-aware policy contract.The test effectively verifies:
- Branch detection mechanisms (
git rev-parse,GITHUB_BASE_REF)- Required flags are always present (
--payload-from-filesystem,--enforce-version-bump)--require-signatureonly appears in the_require_signature == truebranchThe string-splitting approach (Lines 22-27) correctly validates that
--require-signaturedoesn't leak into the else branch, ensuring dev-targeting PRs won't fail on missing signatures..github/workflows/pr-orchestrator.yml (2)
82-106: LGTM — branch-aware signature verification aligns with spec requirements.The implementation correctly:
- Derives
TARGET_BRANCHfromgithub.event.pull_request.base.reffor PRs (Line 85) or stripsrefs/heads/for push events (Line 87)- Builds
VERIFY_CMDarray with base flags (Line 99)- Conditionally appends
--require-signatureonly whenTARGET_BRANCH = "main"(Lines 100-102)- Uses proper array expansion
"${VERIFY_CMD[@]}"to avoid word-splitting issues (Line 106)This satisfies the CI requirement from
openspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.md: dev PRs with unsigned integrity blocks pass; main-targeting PRs enforce signatures.
86-88: Clarify: Push events tomainwill enforce--require-signature.For direct pushes to
main,TARGET_BRANCHresolves tomainand signatures are required. For pushes todev, signatures are not required. This is the expected behavior per the spec ("pushes tomainmust fail if any manifest lacks a valid signature").openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md (1)
1-87: LGTM — TDD evidence follows the requiredspec → tests → failing → code → passingorder.The document properly captures:
- Red phase:
FileNotFoundErrorwhensign-modules-on-approval.ymlwas missing- Green phase: Comprehensive verification including workflow tests, contract-test (555 passed), lint, yaml-lint, and actionlint
- PR review notes: Security hardening details (same-repo gating, merge-base scoping)
The static analysis hints about lowercase "github" in YAML paths (e.g.,
.github/workflows/...) are false positives — these are filesystem paths, not references to the GitHub platform name.tests/unit/workflows/test_pr_orchestrator_signing.py (1)
9-26: LGTM — test validates the branch-aware verification contract.The test comprehensively asserts:
- Required components exist (
verify-module-signatures, script path, flags)TARGET_BRANCHderivation fromgithub.event.pull_request.base.refandGITHUB_REFexpansion- Branch guard
if [ "$TARGET_BRANCH" = "main" ]; thenprecedesVERIFY_CMD+=(--require-signature)- Proper ordering via index comparison (Line 23)
This contract-based approach aligns with the linked repository context showing existing tests in
specfact-clithat assert specific flag presence.openspec/changes/marketplace-06-ci-module-signing/tasks.md (1)
1-75: LGTM — task checklist aligns with implementation and TDD evidence.The checklist accurately tracks the implementation:
- ✅ Branch-aware
pr-orchestrator.yml(Tasks 3.1-3.5)- ✅
sign-modules-on-approval.ymlworkflow (Tasks 4.1-4.8)- ✅ Pre-commit wrapper integration (Task 3.5)
- ✅ Quality gates and evidence capture (Tasks 5.1-5.5)
Pending items 6.3-6.4 (worktree/branch cleanup) are appropriately deferred to post-merge human action.
.github/workflows/sign-modules-on-approval.yml (5)
17-20: LGTM — job condition properly gates execution.The condition correctly requires:
review.state == 'approved'— only runs on approvalbase.ref == 'dev' || base.ref == 'main'— only for protected branch targetshead.repo.full_name == github.repository— critical same-repo check preventing fork PRs from pushing (fork PRs would fail the push anyway due to token scope, but this avoids wasted CI time)
28-34: LGTM — early secrets guard prevents partial execution.Failing immediately if
SPECFACT_MODULE_PRIVATE_SIGN_KEYorSPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASEis empty ensures the workflow doesn't proceed to checkout, install dependencies, or discover manifests before realizing it can't sign. This aligns with the spec requirement to "fail clearly and avoid partial commits."
59-69: LGTM — merge-base scoping correctly targets PR-specific changes.The pattern:
git fetch origin "${PR_BASE_REF}" --no-tags MERGE_BASE="$(git merge-base HEAD "origin/${PR_BASE_REF}")"...followed by
--changed-only --base-ref "$MERGE_BASE"ensures signing is scoped to the PR delta (commits since the merge-base), not the moving base-branch tip. This prevents signing unrelated modules when the base branch has advanced.This matches the contract defined in
scripts/sign-modules.py(Lines 346-427 in context snippet) where--changed-onlywith--base-reffilters manifests by git changes.
71-85: LGTM — commit/push step handles edge cases correctly.Key behaviors:
- No-op when clean:
git status --porcelain -- packages/check (Line 77) prevents empty commits- Scoped staging:
git add -u -- packages/(Line 82) only stages modified tracked files underpackages/- CI skip:
[skip ci]in commit message (Line 83) prevents re-triggering workflows- Correct push target:
HEAD:${PR_HEAD_REF}(Line 85) pushes to the PR source branch
8-10: Concurrency scoped to PR number prevents parallel signing races.The
group: sign-modules-on-approval-${{ github.event.pull_request.number }}withcancel-in-progress: trueensures that if multiple approvals happen rapidly (e.g., re-request then approve), only the latest run proceeds. This avoids potential git push conflicts.openspec/changes/marketplace-06-ci-module-signing/proposal.md (1)
14-60: Scope and constraints are well-aligned with the CI-signing change.The proposal cleanly captures branch-aware verification, approval-triggered signing, and explicit non-goals (no publish/install-time behavior change).
openspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.md (1)
5-53: CI/pre-commit policy split is specified clearly and matches implementation intent.The dev-vs-main enforcement scenarios are concrete and testable, including post-signing unblocking and
mainpush enforcement.tests/unit/workflows/test_sign_modules_on_approval.py (1)
14-47: Good workflow contract coverage for trigger, gating, and write-back behavior.These tests provide strong guardrails around the approval trigger, same-repo restriction, and summary/commit path.
Also applies to: 68-76
openspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.md
Show resolved
Hide resolved
djm81
added
enhancement
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/authoring/module-signing.md`:
- Around line 92-95: The example block in docs/authoring/module-signing.md is
inconsistent: the text says dev-targeting CI omits --require-signature but the
shown command for hatch run python scripts/verify-modules-signature.py includes
--require-signature; update the example to match the statement by either
removing --require-signature from the example command (showing the dev-targeting
variant) or change the preceding sentence to say this is the strict/main check
command and keep the example as-is; reference the flags --require-signature,
--payload-from-filesystem, --enforce-version-bump and the script
verify-modules-signature.py when making the change so the CLI sample and
explanatory text are consistent.
In `@docs/authoring/publishing-modules.md`:
- Line 85: Update the guidance to reinstate version-bump enforcement by adding
the --enforce-version-bump flag back to the verify command and clarify that only
the signature strictness differs between main and dev: instruct users to run
scripts/verify-modules-signature.py --require-signature --enforce-version-bump
--payload-from-filesystem on main, and on dev allow checksum-only acceptance of
manifests but still require --enforce-version-bump when signed module assets or
manifests are affected; reference the script name
(scripts/verify-modules-signature.py) and the flags (--require-signature,
--enforce-version-bump, --payload-from-filesystem) so editors can locate and
update the exact line.
In `@README.md`:
- Line 53: Update the README.md link target for "Module signing" to a repo-safe
path instead of the site-root absolute "/authoring/module-signing/": replace the
link target in the line with **CI signing:** (the `[Module
signing](/authoring/module-signing/)` reference) with a relative repo path such
as `docs/authoring/module-signing.md` or a full modules site URL so the link
works correctly in GitHub README rendering.
🪄 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: 3faf21ba-ccf1-4189-a44a-84e3ebe88cbc
📒 Files selected for processing (11)
.github/ISSUE_TEMPLATE/augmentation_metadata.md.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/change_proposal.md.github/pull_request_template.mdREADME.mddocs/agent-rules/20-repository-context.mddocs/authoring/module-signing.mddocs/authoring/publishing-modules.mddocs/guides/ci-cd-pipeline.mddocs/guides/command-chains.mddocs/reference/module-security.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
docs/agent-rules/**/*.md
📄 CodeRabbit inference engine (.cursorrules)
Load
docs/agent-rules/INDEX.mdand canonical rule files selected by its applicability matrix
Files:
docs/agent-rules/20-repository-context.md
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/agent-rules/20-repository-context.mddocs/authoring/publishing-modules.mddocs/guides/command-chains.mddocs/reference/module-security.mddocs/guides/ci-cd-pipeline.mddocs/authoring/module-signing.md
🧠 Learnings (11)
📓 Common learnings
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:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/change_proposal.mddocs/agent-rules/20-repository-context.mddocs/authoring/publishing-modules.md.github/ISSUE_TEMPLATE/augmentation_metadata.mddocs/guides/command-chains.mddocs/reference/module-security.md.github/pull_request_template.mddocs/guides/ci-cd-pipeline.mdREADME.mddocs/authoring/module-signing.md
📚 Learning: 2026-04-02T21:49:11.371Z
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:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Applied to files:
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/change_proposal.mddocs/agent-rules/20-repository-context.mddocs/authoring/publishing-modules.md.github/ISSUE_TEMPLATE/augmentation_metadata.mddocs/guides/command-chains.mddocs/reference/module-security.md.github/pull_request_template.mddocs/guides/ci-cd-pipeline.mdREADME.mddocs/authoring/module-signing.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/change_proposal.mddocs/agent-rules/20-repository-context.mddocs/authoring/publishing-modules.md.github/ISSUE_TEMPLATE/augmentation_metadata.mddocs/guides/command-chains.mddocs/reference/module-security.md.github/pull_request_template.mddocs/guides/ci-cd-pipeline.mddocs/authoring/module-signing.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
.github/ISSUE_TEMPLATE/bug_report.mddocs/agent-rules/20-repository-context.mddocs/guides/command-chains.mddocs/guides/ci-cd-pipeline.mdREADME.md
📚 Learning: 2026-04-13T10:38:15.842Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-13T10:38:15.842Z
Learning: Adhere to worktree policy, OpenSpec gating, GitHub hierarchy-cache refresh, TDD order, quality gates, versioning, and documentation rules as defined in `docs/agent-rules/`
Applied to files:
.github/ISSUE_TEMPLATE/change_proposal.mddocs/agent-rules/20-repository-context.mddocs/guides/command-chains.mddocs/guides/ci-cd-pipeline.mdREADME.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Run the required verification and quality gates for the touched scope before finalization
Applied to files:
.github/ISSUE_TEMPLATE/change_proposal.mddocs/guides/command-chains.mddocs/guides/ci-cd-pipeline.mdREADME.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Treat canonical rule docs (docs/agent-rules/INDEX.md) as the source of truth for worktree policy, OpenSpec gating, GitHub completeness checks, TDD order, quality gates, versioning, and documentation rules
Applied to files:
docs/agent-rules/20-repository-context.mddocs/guides/ci-cd-pipeline.mdREADME.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Full governance rules live in docs/agent-rules/; do not treat this file (GitHub Copilot Instructions) as a complete standalone handbook
Applied to files:
docs/agent-rules/20-repository-context.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Applied to files:
README.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Treat the clean-code compliance gate as mandatory: the review surface enforces `naming`, `kiss`, `yagni`, `dry`, and `solid` categories and blocks regressions
Applied to files:
README.md
🪛 LanguageTool
README.md
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...rom this repository (not forks) run .github/workflows/sign-modules-on-approval.yml...
(GITHUB)
🔀 Multi-repo context nold-ai/specfact-cli
nold-ai/specfact-cli
Findings:
- .github/workflows/pr-orchestrator.yml contains VERIFY_CMD array construction and conditional appending of --require-signature; workflow invokes scripts/verify-modules-signature.py with flags including --payload-from-filesystem and --enforce-version-bump [::nold-ai/specfact-cli::]
- .github/workflows/sign-modules-on-approval.yml (new) invokes scripts/sign-modules.py with --changed-only --base-ref "$BASE_REF" --payload-from-filesystem, computes MERGE_BASE via git merge-base, checks signing secrets, discovers packages/*/module-package.yaml, and commits/pushes signed manifests back to PR head for same-repo PRs [::nold-ai/specfact-cli::]
- .pre-commit-config.yaml hook for verify-module-signatures now uses ./scripts/pre-commit-verify-modules-signature.sh (test added to assert this) [::nold-ai/specfact-cli::]
- scripts/pre-commit-verify-modules-signature.sh (new) is branch-aware: always uses --payload-from-filesystem and --enforce-version-bump and conditionally appends --require-signature when target is main; it execs scripts/verify-modules-signature.py [::nold-ai/specfact-cli::]
- scripts/verify-modules-signature.py and scripts/sign-modules.py remain the CLI entrypoints; many docs, scripts, and tests expect verifier invocations containing --require-signature (e.g., tests/unit/specfact_cli/registry/test_signing_artifacts.py asserts presence of "verify-modules-signature.py --require-signature") [::nold-ai/specfact-cli::]
- scripts/sign-modules.py is invoked elsewhere (publish workflows, scripts/publish-module.py); new workflow uses --base-ref/--changed-only — verify these CLI flags/options are supported by the script (script files present) [::nold-ai/specfact-cli::]
- src/specfact_cli/registry/module_installer.py builds payloads intended to be identical to scripts/sign-modules.py; changes to when/which flags are supplied (version-check-base, payload-from-filesystem) could affect parity expectations [::nold-ai/specfact-cli::]
- Multiple docs and OpenSpec files across the repo still contain examples referencing --require-signature; the PR updates many of these, but numerous occurrences remain (see openspec/* and docs/* references) — reviewers should confirm documentation consistency with workflow behavior [::nold-ai/specfact-cli::]
Conclusion:
- Changes are repository-local but affect many callers/tests/docs that expect fixed verifier invocation including --require-signature. Key risks: tests asserting literal "--require-signature" may need updates; ensure sign-modules.py supports --base-ref/--changed-only; confirm docs and other scripts (setup-git-hooks.sh, pre-commit-smart-checks.sh) updated where they still call verifier with unconditional --require-signature [::nold-ai/specfact-cli::]
🔇 Additional comments (11)
.github/ISSUE_TEMPLATE/augmentation_metadata.md (1)
35-36: Branch-aware verification guidance is clear and correct.Line 35 and Line 36 align with the new dev-vs-main policy without over-constraining feature/dev workflows.
.github/ISSUE_TEMPLATE/change_proposal.md (1)
23-23: Acceptance criterion now correctly models target-branch policy.Line 23 is a good update: dev-targeting verification stays strict on payload/version checks while main policy explicitly adds signature requirement.
.github/ISSUE_TEMPLATE/bug_report.md (1)
25-27: Repro instructions now cover both CI policy modes.Lines 25-27 and Line 60 make bug triage much more precise by separating default verify behavior from main-equivalent strict checks.
Also applies to: 60-60
docs/agent-rules/20-repository-context.md (1)
48-49: Repository-context command block is aligned with current gate behavior.Line 48 and Line 49 correctly present the default verify invocation and the main-only signature escalation.
docs/guides/command-chains.md (1)
81-81: CI-ready command chain update is consistent and actionable.Line 81 plus Line 87 preserve the gate order while documenting when strict signature enforcement is required.
Also applies to: 87-87
docs/guides/ci-cd-pipeline.md (1)
38-38: CI pipeline guide now accurately reflects the module-integrity boundary.These updates correctly describe when
--require-signatureis applied and howsign-modules-on-approvaloperates for same-repo PRs.Also applies to: 44-44, 52-52, 54-54, 57-60
docs/reference/module-security.md (1)
49-52: Security reference update is aligned with verifier/runtime contract.Line 49 to Line 52 cleanly capture the core adapter boundary: checksum+version enforcement always, signature requirement conditional by target policy, and CI signing automation scope.
README.md (2)
44-44: Branch-aware signing docs align with implementation.The README updates are consistent with
scripts/pre-commit-verify-modules-signature.shand the approval-based signing workflow semantics.Also applies to: 51-53, 62-62
51-51:⚠️ Potential issue | 🟡 MinorFix duplicated word in policy text.
There is a typo: “CI checks checksums…”. Please remove one “checks” for clarity.
⛔ Skipped due to learnings
Learnt from: CR Repo: nold-ai/specfact-cli-modules PR: 0 File: .cursorrules:0-0 Timestamp: 2026-04-13T10:38:15.842Z Learning: Adhere to worktree policy, OpenSpec gating, GitHub hierarchy-cache refresh, TDD order, quality gates, versioning, and documentation rules as defined in `docs/agent-rules/`Learnt from: CR Repo: nold-ai/specfact-cli-modules PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-04-13T10:38:29.379Z Learning: Treat canonical rule docs (docs/agent-rules/INDEX.md) as the source of truth for worktree policy, OpenSpec gating, GitHub completeness checks, TDD order, quality gates, versioning, and documentation rulesLearnt 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:11.371Z Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.Learnt from: CR Repo: nold-ai/specfact-cli-modules PR: 0 File: AGENTS.md:0-0 Timestamp: 2026-04-13T10:38:43.524Z Learning: Enforce module signatures and version bumps when signed module assets or manifests are affectedLearnt from: CR Repo: nold-ai/specfact-cli-modules PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2026-04-13T10:38:22.837Z Learning: Signed module or manifest changes require version-bump review and verify-modules-signature.github/pull_request_template.md (1)
43-46: Template now correctly reflects branch-aware signature policy.The checklist wording is accurate for
devvsmain, and the approval-triggered same-repo signing expectation is correctly captured.Also applies to: 52-52
docs/authoring/module-signing.md (1)
114-124: CI/pre-commit branch-aware signing behavior is documented clearly.These sections accurately reflect conditional
--require-signatureenforcement and same-repo approval signing flow.As per coding guidelines, “User-facing and cross-site accuracy … CLI examples matching bundled commands.”
Also applies to: 134-143
- Pin signing workflow checkout to PR head SHA; merge-base changed-only signing; require both signing secrets with named errors; handle push rejection. - Gate keeps OpenSpec Markdown for freshness; SpecFact run skips non-Python openspec paths; restore icontract on build_review_command only. - Update marketplace-06 OpenSpec artifacts and docs; extend unit tests. - Split workflow signing tests to satisfy cyclomatic-complexity review gate. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
docs/authoring/module-signing.md (1)
92-95: 🧹 Nitpick | 🔵 TrivialClarify the "verify after signing" example intent.
The comment at lines 92-93 says dev-targeting CI omits
--require-signature, yet the command at line 94 includes it. The text explains this is for "checking as for main," but the juxtaposition can confuse readers. Consider rewording to make it explicit this is the strict/main-equivalent verification step:📝 Suggested clarification
-# Verify after signing (must match sign payload mode). For a dev-targeting branch, CI omits -# --require-signature; add it when checking as for main: +# Verify after signing (must match sign payload mode). This command matches the strict +# main-equivalent check; omit --require-signature for dev-parity: hatch run python scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump --version-check-base origin/dev🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/authoring/module-signing.md` around lines 92 - 95, The "Verify after signing" example is confusing because the prose says dev-targeting CI omits --require-signature but the example command includes it; update the text around the hatch run python scripts/verify-modules-signature.py command to explicitly state this command is the strict/main-equivalent verification (i.e., show how to run the verifier with --require-signature, --payload-from-filesystem, --enforce-version-bump and --version-check-base origin/dev when you want to perform the same checks as main), so readers understand the example demonstrates the stricter check rather than the normal dev-targeting CI invocation.README.md (1)
53-53:⚠️ Potential issue | 🟡 MinorUse a repo-relative link for the module signing reference.
The link
[Module signing](/authoring/module-signing/)is site-root absolute and will break in GitHub README rendering. Use a relative repo path instead.📝 Suggested fix
-**CI signing:** Approved PRs to `dev` or `main` from **this repository** (not forks) run `.github/workflows/sign-modules-on-approval.yml`, which can commit signed manifests using repository secrets. See [Module signing](/authoring/module-signing/). +**CI signing:** Approved PRs to `dev` or `main` from **this repository** (not forks) run `.github/workflows/sign-modules-on-approval.yml`, which can commit signed manifests using repository secrets. See [Module signing](docs/authoring/module-signing.md).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 53, Update the repository-relative link used in the "**CI signing:**" line so the "Module signing" link is not site-root absolute; replace the existing [Module signing](/authoring/module-signing/) target with a repo-relative path such as [Module signing](./authoring/module-signing/) (or authoring/module-signing/) so the link works when the README is rendered on GitHub.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/workflows/test_sign_modules_on_approval.py`:
- Around line 14-85: The tests use loose substring checks that can give false
positives; update the three test functions
(test_sign_modules_on_approval_trigger_and_job_filter,
test_sign_modules_on_approval_secrets_guard,
test_sign_modules_on_approval_commit_push_and_summary) to parse the generated
workflow YAML (use yaml.safe_load on _workflow_text()) and then scope assertions
to the specific job/step nodes instead of global substrings: locate the job by
its key (e.g. the sign job), find steps by unique identifiers like "id:
discover", step name "Guard signing secrets", and step that sets "MERGE_BASE" or
outputs "steps.discover.outputs.manifests_count", and assert the expected
keys/values inside those nodes (e.g. triggers/types/permissions under the job,
the guard step shell and exit checks, and the commit/push step's commit message,
push args and summary outputs). Use structured YAML assertions so the checks
fail if content moves to the wrong job/step.
---
Duplicate comments:
In `@docs/authoring/module-signing.md`:
- Around line 92-95: The "Verify after signing" example is confusing because the
prose says dev-targeting CI omits --require-signature but the example command
includes it; update the text around the hatch run python
scripts/verify-modules-signature.py command to explicitly state this command is
the strict/main-equivalent verification (i.e., show how to run the verifier with
--require-signature, --payload-from-filesystem, --enforce-version-bump and
--version-check-base origin/dev when you want to perform the same checks as
main), so readers understand the example demonstrates the stricter check rather
than the normal dev-targeting CI invocation.
In `@README.md`:
- Line 53: Update the repository-relative link used in the "**CI signing:**"
line so the "Module signing" link is not site-root absolute; replace the
existing [Module signing](/authoring/module-signing/) target with a
repo-relative path such as [Module signing](./authoring/module-signing/) (or
authoring/module-signing/) so the link works when the README is rendered on
GitHub.
🪄 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: 3653af99-0923-46ab-890d-f2c33555ba76
📒 Files selected for processing (9)
.github/workflows/sign-modules-on-approval.ymlREADME.mddocs/authoring/module-signing.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.mdscripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/workflows/test_sign_modules_on_approval.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: quality (3.12)
- GitHub Check: quality (3.11)
- GitHub Check: quality (3.13)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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:
scripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pytests/unit/workflows/test_sign_modules_on_approval.py
scripts/**/*.py
⚙️ CodeRabbit configuration file
scripts/**/*.py: Deterministic tooling: signing, publishing, docs generation; subprocess and path safety.
Files:
scripts/pre_commit_code_review.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/scripts/test_pre_commit_code_review.pytests/unit/workflows/test_sign_modules_on_approval.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/marketplace-06-ci-module-signing/tasks.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.md
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/authoring/module-signing.md
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/sign-modules-on-approval.yml
🧠 Learnings (13)
📓 Common learnings
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:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: This repository enforces 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.pytests/unit/scripts/test_pre_commit_code_review.pyREADME.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Fix SpecFact code review findings, including warnings, unless a rare explicit exception is documented
Applied to files:
scripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.py
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Treat the clean-code compliance gate as mandatory: the review surface enforces `naming`, `kiss`, `yagni`, `dry`, and `solid` categories and blocks regressions
Applied to files:
scripts/pre_commit_code_review.pyREADME.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: When a change is paired with work in specfact-cli, review the paired public change artifacts there before widening scope or redefining shared workflow semantics
Applied to files:
scripts/pre_commit_code_review.pytests/unit/scripts/test_pre_commit_code_review.pyopenspec/changes/marketplace-06-ci-module-signing/tasks.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdREADME.mdopenspec/changes/marketplace-06-ci-module-signing/design.md
📚 Learning: 2026-04-13T10:38:15.842Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .cursorrules:0-0
Timestamp: 2026-04-13T10:38:15.842Z
Learning: Adhere to worktree policy, OpenSpec gating, GitHub hierarchy-cache refresh, TDD order, quality gates, versioning, and documentation rules as defined in `docs/agent-rules/`
Applied to files:
scripts/pre_commit_code_review.pyopenspec/changes/marketplace-06-ci-module-signing/tasks.mdREADME.md
📚 Learning: 2026-04-13T10:38:29.379Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-13T10:38:29.379Z
Learning: Applies to **/*.{js,ts,tsx,jsx,py,java,cs,go,rb,php,cpp,c,h} : Preserve the clean-code compliance gate and its category references (naming, kiss, yagni, dry, and solid)
Applied to files:
scripts/pre_commit_code_review.py
📚 Learning: 2026-04-02T21:49:11.371Z
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:11.371Z
Learning: In nold-ai/specfact-cli-modules, module tarball signatures (registry/signatures/*.tar.sig) are generated by the `publish-modules` GitHub Actions runner during the publish workflow, not committed locally to the branch. Missing signature files should NOT be flagged as a pre-merge blocker in PRs.
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.mddocs/authoring/module-signing.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdREADME.mdopenspec/changes/marketplace-06-ci-module-signing/design.md.github/workflows/sign-modules-on-approval.ymltests/unit/workflows/test_sign_modules_on_approval.py
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.mddocs/authoring/module-signing.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdREADME.mdopenspec/changes/marketplace-06-ci-module-signing/design.md.github/workflows/sign-modules-on-approval.yml
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.mddocs/authoring/module-signing.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.md
📚 Learning: 2026-04-13T10:38:22.837Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-04-13T10:38:22.837Z
Learning: Work belongs on feature/*, bugfix/*, hotfix/*, or chore/* branches, normally in a worktree rooted under ../specfact-cli-modules-worktrees/
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Perform `spec -> tests -> failing evidence -> code -> passing evidence` in that order for behavior changes
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.md
📚 Learning: 2026-04-13T10:38:43.524Z
Learnt from: CR
Repo: nold-ai/specfact-cli-modules PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-13T10:38:43.524Z
Learning: Run the required verification and quality gates for the touched scope before finalization
Applied to files:
README.md
🪛 LanguageTool
openspec/changes/marketplace-06-ci-module-signing/tasks.md
[uncategorized] ~23-~23: The official name of this software platform is spelled with a capital “H”.
Context: ...t verify by target branch - [x] 3.1 In .github/workflows/pr-orchestrator.yml, in the ...
(GITHUB)
[uncategorized] ~24-~24: The official name of this software platform is spelled with a capital “H”.
Context: ...job, extract the target branch fromgithub.event.pull_request.base.ref` (PR events...
(GITHUB)
[uncategorized] ~24-~24: The official name of this software platform is spelled with a capital “H”.
Context: ....pull_request.base.ref(PR events) andgithub.ref` (push events). - [x] 3.2 For eve...
(GITHUB)
[uncategorized] ~38-~38: The official name of this software platform is spelled with a capital “H”.
Context: ...dules-on-approval.yml - [x] 4.1 Create .github/workflows/sign-modules-on-approval.yml...
(GITHUB)
[uncategorized] ~40-~40: The official name of this software platform is spelled with a capital “H”.
Context: ...roved review, base dev or main, and `github.event.pull_request.head.repo.full_name ...
(GITHUB)
docs/authoring/module-signing.md
[uncategorized] ~138-~138: The official name of this software platform is spelled with a capital “H”.
Context: ...o equals the base repo). It checks out **github.event.pull_request.head.sha`** (the com...
(GITHUB)
README.md
[uncategorized] ~53-~53: The official name of this software platform is spelled with a capital “H”.
Context: ...rom this repository (not forks) run .github/workflows/sign-modules-on-approval.yml...
(GITHUB)
🪛 markdownlint-cli2 (0.22.0)
openspec/changes/marketplace-06-ci-module-signing/design.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
[warning] 31-31: Ordered list item prefix
Expected: 1; Actual: 3; Style: 1/1/1
(MD029, ol-prefix)
🔀 Multi-repo context nold-ai/specfact-cli
Linked repositories findings
nold-ai/specfact-cli
-
verify CLI scripts exist:
- scripts/verify-modules-signature.py present. [::nold-ai/specfact-cli::scripts/verify-modules-signature.py]
- scripts/sign-modules.py present. [::nold-ai/specfact-cli::scripts/sign-modules.py]
-
Workflows referencing the verifier/signing:
-
.github/workflows/pr-orchestrator.yml contains invocations that include verify/sign commands and branch-aware sign call: e.g. lines invoking:
- python scripts/verify-modules-signature.py --require-signature --enforce-version-bump --payload-from-filesystem --version-check-base "$BASE_REF"
and - python scripts/verify-modules-signature.py --require-signature --enforce-version-bump
- python scripts/sign-modules.py --changed-only --base-ref "$BASE_REF" --bump-version patch --payload-from-filesystem
[::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
- python scripts/verify-modules-signature.py --require-signature --enforce-version-bump --payload-from-filesystem --version-check-base "$BASE_REF"
-
.github/workflows/sign-modules.yml references verify and sign scripts and includes invocations of verify-modules-signature.py with --require-signature (multiple places). [::nold-ai/specfact-cli::.github/workflows/sign-modules.yml]
-
-
Tests and configuration that assert or expect --require-signature:
- tests/unit/specfact_cli/registry/test_signing_artifacts.py asserts presence of "verify-modules-signature.py --require-signature" and checks script existence. [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py]
- pyproject.toml contains an entry with "python scripts/verify-modules-signature.py --require-signature". [::nold-ai/specfact-cli::pyproject.toml]
- scripts/pre-commit-smart-checks.sh contains an unconditional hatch run ./scripts/verify-modules-signature.py --require-signature invocation. [::nold-ai/specfact-cli::scripts/pre-commit-smart-checks.sh]
-
Documentation and OpenSpec still include many literal references to --require-signature:
- Multiple docs and openspec files reference running verify-modules-signature.py with --require-signature (examples include docs/guides/, openspec/, docs/reference/module-security.md, etc.). Representative occurrences shown in search output. [::nold-ai/specfact-cli::docs/, ::nold-ai/specfact-cli::openspec/]
-
Potential friction points (observed in repo):
- The repository contains many literal usages/tests/docs that assert or call verify-modules-signature.py with unconditional --require-signature, while the PR's changes make signature requirement conditional (branch-aware). These mismatches likely require test updates and docs/config alignment. Evidence: tests and scripts above reference the flag literally. [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py][::nold-ai/specfact-cli::scripts/pre-commit-smart-checks.sh][::nold-ai/specfact-cli::pyproject.toml]
-
Other callers of sign-modules.py:
- scripts/publish-module.py and scripts/sign-module.sh call sign-modules.py; publish workflows also call sign-modules.py. Ensure the new flags (--changed-only, --base-ref) are supported by scripts/sign-modules.py (script is present; callers exist). [::nold-ai/specfact-cli::scripts/publish-module.py][::nold-ai/specfact-cli::.github/workflows/publish-modules.yml][::nold-ai/specfact-cli::scripts/sign-modules.py]
Summary assessment:
- Relevant cross-repo/context artifacts are all within this repo. Key findings: verifier and signer scripts exist; workflows and tests/docs still contain literal --require-signature usages that could conflict with the new branch-aware behavior introduced by the PR. Tests and helper scripts (pre-commit-smart-checks.sh, pyproject hooks, and various OpenSpec/docs) will need review/updates to align with conditional --require-signature behavior.
🔇 Additional comments (23)
scripts/pre_commit_code_review.py (3)
54-57: LGTM! TDD_EVIDENCE.md exclusion correctly scoped.The case-insensitive basename check (
Path(normalized).name.casefold() == "tdd_evidence.md") ensures only TDD evidence files are excluded from the gate, while other OpenSpec Markdown underopenspec/changes/remains eligible. This addresses the prior review gap around blanket exclusion.
85-93: Clean separation between gate eligibility and SpecFact input filtering.
_specfact_review_paths()correctly filters OpenSpec Markdown after gate-eligibility decisions, ensuring the SpecFact CLI (which expects Python) doesn't receive Markdown paths. The two-stage filtering maintains proper separation of concerns.
287-293: LGTM! Graceful skip when only OpenSpec Markdown is staged.The early exit with code 0 and clear messaging prevents false failures when commits contain only OpenSpec Markdown (which is gate-eligible but not SpecFact-reviewable).
tests/unit/scripts/test_pre_commit_code_review.py (3)
37-41: Good coverage for the SpecFact path filtering.The test validates that
_specfact_review_pathscorrectly filters out OpenSpec Markdown while preserving Python files. This mirrors the production behavior where SpecFact CLI should not receive Markdown inputs.
44-62: Updated test correctly reflects gate eligibility split.The expected output
["tests/test_app.py", "openspec/changes/foo/tasks.md", "openspec/changes/foo/proposal.md"]confirms that OpenSpec Markdown (exceptTDD_EVIDENCE.md) remains gate-eligible while the exclusion applies only to the evidence file.
89-97: LGTM! Validates the OpenSpec-only skip path.This test ensures that when only OpenSpec Markdown is staged,
main()exits successfully with the expected skip message rather than failing or invoking SpecFact unnecessarily.docs/authoring/module-signing.md (2)
114-124: LGTM! Branch-aware verification documented correctly.The two verification modes are clearly distinguished: dev/feature (lines 117-118) omits
--require-signaturewhile main (lines 122-124) includes it. This matches the CI policy inpr-orchestrator.ymland the pre-commit wrapper behavior.
134-143: LGTM! CI signing documentation comprehensive.The section accurately documents:
- Branch-aware
--require-signatureenforcement inpr-orchestrator.yml- Same-repo approval-triggered signing via
sign-modules-on-approval.yml- Pre-commit wrapper mirroring CI policy via
GITHUB_BASE_REFThe static analysis hint about "GitHub" capitalization is a false positive—
github.event...references are YAML/Actions expressions, not prose.openspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.md (3)
36-41: Past concern addressed: Both signing secrets now specified.Lines 39-40 correctly require failure when either
SPECFACT_MODULE_PRIVATE_SIGN_KEYorSPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASEis missing, with clear error naming which secret(s) are absent. This matches the guard step implementation insign-modules-on-approval.ymllines 28-38.
11-29: Merge-base scoping correctly specified.The scenarios properly distinguish dev (merge-base with
origin/dev) from main (merge-base withorigin/main) for determining which manifests have changed. This ensures signing is scoped to actual PR changes rather than accumulated drift from the moving base-branch tip—critical for registry manifest integrity.
62-66: LGTM! Idempotency requirement well-specified.The spec mandates byte-for-byte identical
integrity:blocks on re-runs, ensuring CI doesn't create spurious commits. This is essential for registry manifest stability and aligns with the deterministic signing expectations inscripts/sign-modules.py..github/workflows/sign-modules-on-approval.yml (4)
17-20: LGTM! Job condition correctly scopes to same-repo approved PRs.The condition gates on:
approvedreview state- Base branch
devormain- Head repo matches base repo (excludes forks)
This prevents unauthorized signing attempts and aligns with the OpenSpec requirement that fork PRs are out of scope.
28-38: LGTM! Secret guard with named errors.Both
SPECFACT_MODULE_PRIVATE_SIGN_KEYandSPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASEare validated with specific::error::annotations before any checkout or signing occurs. This satisfies the spec requirement for clear failure naming the missing secret.
63-73: LGTM! Merge-base-scoped signing implementation.The signing step correctly:
- Fetches the PR base ref (not HEAD tip)
- Computes
MERGE_BASEfor PR-local change detection- Invokes
sign-modules.pywith--changed-only --base-ref "$MERGE_BASE"This ensures signing is scoped to actual PR changes, not accumulated drift from the moving base branch—critical for the
packages/*/module-package.yamlregistry contract.
75-92: Robust commit/push with push-rejection guidance.The step handles the edge case where the PR branch advances after approval (lines 89-92), providing actionable guidance to update and re-approve. The
[skip ci]commit message prevents workflow recursion, andgit add -u -- packages/correctly scopes to manifest changes only.README.md (2)
44-44: LGTM! Quality gate command matches dev/feature CI.The
verify-modules-signaturecommand correctly omits--require-signature, aligning with the branch-aware policy where dev/feature branches verify checksums and version bumps without requiring cryptographic signatures.
62-62: LGTM! Code review gate documentation accurate.The description correctly reflects the two-stage filtering:
TDD_EVIDENCE.mdexcluded from gate eligibility, and remaining OpenSpec Markdown excluded from SpecFact input (since the review CLI treats paths as Python).openspec/changes/marketplace-06-ci-module-signing/design.md (2)
42-59: Past concern addressed: Decision 1 now consistent.The design correctly documents computing
MERGE_BASEviagit merge-baseand invokingsign-modules.pywith--changed-only --base-ref "$MERGE_BASE". This matches the actual workflow implementation and ensures the script determines changed modules itself rather than receiving explicit paths.
80-89: LGTM! Risks documented with practical mitigations.The version-bump race condition risk (lines 84-88) acknowledges the edge case where simultaneous PR approvals could produce the same patch version, with a reasonable mitigation noting idempotent signing and single-maintainer cadence. The mutex-locking future option is appropriately deferred.
openspec/changes/marketplace-06-ci-module-signing/tasks.md (3)
1-2: Past concern addressed: Top-level heading added.The file now starts with
# Tasks: marketplace-06-ci-module-signingfollowed by a blank line, satisfying the MD041 (first-line-h1) lint rule.
48-53: Task 4.5 accurately reflects the merge-base approach.The task correctly specifies computing
MERGE_BASEbefore--changed-only, ensuring PR-scoped deltas rather than base-branch tip comparisons. The secret failure requirement aligns with the workflow guard step implementation.
70-77: LGTM! PR linkage and cleanup tasks documented.The cross-repo coordination is well-documented with links to both the modules issue (
#185) and the paired specfact-cli PR. Post-merge cleanup tasks (worktree removal, branch deletion) are appropriately marked pending.tests/unit/workflows/test_sign_modules_on_approval.py (1)
49-55: Good coverage improvement on required signing secrets.Including both private-key and passphrase emptiness guards closes an important failure-path contract gap for approval-triggered signing.
2 participants
Follow-up after merging specfact-cli-modules#188 into
dev.This PR promotes the current
devline tomain, including:verify-module-signaturesinpr-orchestrator.ymlsign-modules-on-approval.yml(same-repo PRs; merge-base scoped signing)pre-commit-verify-modules-signature.sh,GITHUB_BASE_REFin Actions)marketplace-06-ci-module-signingPlease confirm CI is green on this PR before merge.
Made with Cursor