feat(ci): marketplace-06 module signing on approval + branch-aware verify#188
feat(ci): marketplace-06 module signing on approval + branch-aware verify#188
Conversation
- 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
📝 WalkthroughOverviewImplements marketplace-06 CI changes: adds a CI signing workflow that signs changed module manifests on approved PR reviews targeting dev or main, and makes module-signature verification branch-aware so cryptographic signatures are required only at the main boundary while dev/feature branches enforce checksum + version-bump checks. Bundle & Module Surface
Manifest & Integrity
Cross-repo / Contract Alignment
Tests
Documentation & OpenSpec
Registry / Deployment Impact
OpenSpec Change ID & Scenario Coverage
Related Issues & Notes
WalkthroughAdds an approval-triggered CI signing workflow, makes module-signature enforcement branch-aware (require signatures only for main) across CI and pre-commit, introduces a branch-aware pre-commit wrapper script, excludes OpenSpec markdown from the specfact review gate, and adds/updates unit tests and OpenSpec docs/TDD evidence. Changes
Sequence Diagram(s)sequenceDiagram
actor Reviewer
participant GitHub as GitHub Actions
participant Checkout as Checkout
participant Runner as Runner (Actions)
participant Python as Python 3.12
participant Discover as Manifest Discovery
participant Sign as sign-modules.py
participant Git as Git Commit/Push
participant Summary as Job Summary
Reviewer->>GitHub: Submit approved review (base: dev or main)
GitHub->>GitHub: Trigger sign-modules-on-approval
GitHub->>Checkout: Checkout PR head commit
Checkout->>Runner: Start job on runner
Runner->>Python: Setup Python & deps
Python->>Discover: Find `packages/**/module-package.yaml`
Discover->>Sign: Invoke `scripts/sign-modules.py --changed-only --bump-version patch --payload-from-filesystem --base-ref "$MERGE_BASE"`
Sign->>Sign: Sign/bump manifests if applicable
alt Changes detected under packages/
Sign->>Git: Commit changes & push to `HEAD:${PR_HEAD_REF}`
Git->>Summary: Write `COMMIT_CHANGED: true` and `MANIFESTS_COUNT:`
else No changes
Sign->>Summary: Write `COMMIT_CHANGED: false` and `MANIFESTS_COUNT:`
end
Summary->>GitHub: Update job summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
djm81
added
enhancement
![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: 81116d2b3e
ℹ️ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/sign-modules-on-approval.yml:
- Around line 50-55: The "Discover module manifests" step currently uses mapfile
to populate the MANIFESTS array but never exports its result, so either remove
this informational step if it's unnecessary noise, or modify it to export the
discovered count for downstream summary use: capture the length of MANIFESTS
(from the mapfile call) and write it to GITHUB_OUTPUT (or set-output) under a
clear key like manifests_count so later steps can consume it; reference the
MANIFESTS array, the mapfile invocation, and the step name "Discover module
manifests" when making the change.
- Around line 35-38: The workflow currently checks out the PR commit using
actions/checkout@v4 with ref: ${{ github.event.pull_request.head.sha }}, which
can diverge from the branch targeted by the subsequent push
(HEAD:${PR_HEAD_REF}); change the checkout to use the branch ref (${{
github.event.pull_request.head.ref }}) so you get the latest branch tip, or if
you prefer to keep the SHA, add a pre-push sync step that runs git fetch origin
"${PR_HEAD_REF}" and git pull --rebase origin "${PR_HEAD_REF}" (or equivalent)
before the commit/push to ensure a fast-forward; adjust the action configuration
and the step sequence that references PR_HEAD_REF and HEAD:${PR_HEAD_REF}
accordingly.
In `@openspec/changes/marketplace-06-ci-module-signing/tasks.md`:
- Around line 40-45: Task 4.5 in the marketplace-06 CI tasks incorrectly
recommends using the --allow-same-version flag while the actual workflow
sign-modules-on-approval.yml enforces version bumps; update the task text to
remove or caveat the --allow-same-version suggestion and instead state that the
workflow enforces version increments (or mention the alternative “or use
--changed-only --base-ref \"$BASE_REF\"” path), keeping the note about failing
immediately if SPECFACT_MODULE_PRIVATE_SIGN_KEY or
SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE is empty; reference the task label
“4.5” and the flag name “--allow-same-version” so the change is easy to find.
In `@tests/unit/workflows/test_pr_orchestrator_signing.py`:
- Around line 19-24: The test currently checks substrings but not that the
--require-signature append is inside the main-branch conditional; update
tests/unit/workflows/test_pr_orchestrator_signing.py to assert the full
conditional head 'if [ "$TARGET_BRANCH" = "main" ]; then' (or equivalent exact
block opener) is present, keep the existing assertion for
'VERIFY_CMD+=(--require-signature)', and add a sequence check by locating the
index of the TARGET_BRANCH check string and the index of
'VERIFY_CMD+=(--require-signature)' in the workflow string and asserting the
branch check index is before the require-signature index so the append is
guarded by the main-branch condition.
In `@tests/unit/workflows/test_sign_modules_on_approval.py`:
- Around line 41-49: Update the test_sign_modules_on_approval_sign_and_secrets
test to also assert that the guard step for secrets is present and enforces
failure: check that the workflow string (workflow) contains the guard step title
"Guard signing secrets", contains the shell check for missing
SPECFACT_MODULE_PRIVATE_SIGN_KEY (e.g. '[ -z
"${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]') and that the guard includes an "exit
1" on failure; this ensures the guard step (not just env vars) is validated by
the 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: ce850a30-c760-42c9-b026-d659aecd33c9
📒 Files selected for processing (8)
.github/workflows/pr-orchestrator.yml.github/workflows/sign-modules-on-approval.ymlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.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_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.11)
- GitHub Check: quality (3.13)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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_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/workflows/test_pr_orchestrator_signing.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/TDD_EVIDENCE.md
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: CI: secrets, hatch/verify-modules-signature gates, contract-test alignment, action versions.
Files:
.github/workflows/pr-orchestrator.yml.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: .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/
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
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.pyopenspec/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.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/changes/marketplace-06-ci-module-signing/tasks.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md.github/workflows/sign-modules-on-approval.yml
📚 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: 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-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.md.github/workflows/pr-orchestrator.ymlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdtests/unit/workflows/test_pr_orchestrator_signing.py.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.md.github/workflows/pr-orchestrator.ymlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md.github/workflows/sign-modules-on-approval.ymltests/unit/workflows/test_sign_modules_on_approval.py
📚 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/changes/marketplace-06-ci-module-signing/tasks.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.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.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: 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.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: Require public GitHub metadata completeness before implementation when linked issue workflow applies: parent, labels, project assignment, blockers, and blocked-by relationships
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: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
.github/workflows/pr-orchestrator.yml
🪛 LanguageTool
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] ~32-~32: 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] ~34-~34: The official name of this software platform is spelled with a capital “H”.
Context: ...bmitted]. - [x] 4.2 Add job condition: if: github.event.review.state == 'approved' && (gi...
(GITHUB)
[uncategorized] ~40-~40: The official name of this software platform is spelled with a capital “H”.
Context: ...). - [x] 4.5 Add signing step: compute BASE_REF="origin/${{ github.event.pull_request.base.ref }}"`; for...
(GITHUB)
🔀 Multi-repo context nold-ai/specfact-cli
nold-ai/specfact-cli
- .github/workflows/pr-orchestrator.yml — contains verify-module-signatures job referenced by tests and docs; tests assert presence and conditional VERIFY_CMD logic. [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
- .github/workflows/sign-modules.yml — existing signing workflow; new sign-on-approval workflow in PR will be analogous. [::nold-ai/specfact-cli::.github/workflows/sign-modules.yml]
- scripts/sign-modules.py — signing implementation used by CI and docs; referenced by multiple docs and scripts (publish-module.py, sign-module.sh, docs). Changes to workflow flags/args (e.g., --changed-only, --base-ref, --payload-from-filesystem, --bump-version, --require-signature) must match this script's CLI. [::nold-ai/specfact-cli::scripts/sign-modules.py]
- scripts/verify-modules-signature.py — verifier script invoked by pr-orchestrator verify step (rg matched references to verify-module-signatures job invoking this script). Ensure pr-orchestrator changes still build the invocation args compatible with this script. [::nold-ai/specfact-cli::scripts/verify-modules-signature.py]
- scripts/publish-module.py — calls scripts/sign-modules.py; CI signing/push behavior that updates manifests may interact with publishing flows. [::nold-ai/specfact-cli::scripts/publish-module.py]
- scripts/pre_commit_code_review.py — present and heavily tested; PR changes exclude openspec/changes/*.md from review gate — tests updated accordingly (tests/unit/scripts/test_pre_commit_code_review.py). Verify altered path filtering aligns with other tests/consumers. [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
- tests/unit/workflows/* — multiple tests reference verify-module-signatures and sign-modules workflows (e.g., tests/unit/workflows/test_trustworthy_green_checks.py, tests/unit/workflows/test_pr_orchestrator_signing.py, tests/unit/workflows/test_sign_modules_on_approval.py). These tests assert workflow contents and flags; they must be kept in sync with the YAML changes. [::nold-ai/specfact-cli::tests/unit/workflows/]
- docs/guides/module-signing-and-key-rotation.md and docs/module-system/module-marketplace.md — document invocation patterns for scripts/sign-modules.py and CI usage; updates to workflow arg behavior (signature requirement conditional on target branch) should be reflected in docs if necessary. [::nold-ai/specfact-cli::docs/guides/module-signing-and-key-rotation.md]
Conclusion: The PR touches workflow invocation arguments and adds a signing-on-approval workflow that will call scripts/sign-modules.py. Relevant consumers and contract points found: scripts/sign-modules.py, scripts/verify-modules-signature.py, scripts/publish-module.py, pre_commit_code_review.py, multiple workflow tests, and documentation pages. Reviewers should verify the new workflow and pr-orchestrator argument construction remain compatible with scripts/sign-modules.py and verify-modules-signature.py CLI expectations, and that unit tests/tests/workflows reflect the updated YAML.
🔇 Additional comments (7)
.github/workflows/pr-orchestrator.yml (1)
82-106: LGTM on branch-aware signature enforcement logic.The
VERIFY_CMDarray construction cleanly separates the conditional--require-signatureflag formainversusdevbranches. This aligns well with the trust model documented in the linked issue: signatures enforced at themainboundary, checksum-only integrity fordev-targeting PRs.The script invocation matches the
scripts/verify-modules-signature.pyargparse definition (context snippet 2 from relevant code snippets), where--require-signatureis astore_trueoptional flag..github/workflows/sign-modules-on-approval.yml (2)
56-66: Script invocation correctly matchessign-modules.pyCLI contract.The flags
--changed-only,--base-ref,--bump-version patch, and--payload-from-filesystemalign with the argparse definition inscripts/sign-modules.py(lines 346-395 in linked repo context). The combination triggers the intended flow: filter changed modules, auto-bump patch version if unchanged, enforce version bump, and sign with filesystem payload.
67-81: LGTM on commit-back logic.The step properly:
- Sets bot identity for attribution
- Uses
git status --porcelain -- packages/to detect actual changes- Stages only modified tracked files with
git add -u- Includes
[skip ci]to prevent infinite workflow loops- Outputs
changed=true/falsefor downstream summarytests/unit/scripts/test_pre_commit_code_review.py (1)
37-51: LGTM on test update for OpenSpec Markdown exclusion.The test correctly verifies that
openspec/changes/**/*.mdfiles (includingtasks.mdandproposal.md) are now filtered out, leaving only paths under valid review-gate prefixes liketests/. This aligns with thescripts/pre_commit_code_review.pychange that prevents false positives when the review CLI attempts Python analysis on Markdown docs.scripts/pre_commit_code_review.py (1)
56-58: LGTM on Markdown exclusion from review gate.The exclusion correctly filters
openspec/changes/**/*.mdfiles before the prefix matching at Line 65. The case-insensitive.lower().endswith(".md")handles mixed-case extensions. The comment documents the rationale: SpecFact CLI treats these as Python and emits noise.Note:
openspec/changes/remains in theprefixestuple (Line 65), which still allows non-Markdown files under that path to participate in the gate if needed.openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md (1)
1-48: LGTM on TDD evidence documentation.The document follows the expected
spec -> tests -> failing -> code -> passingorder. The Green verification section properly recordspytest,hatch run lint,yaml-lint, andactionlintresults. The pre-commit note explains the Markdown exclusion rationale clearly.tests/unit/workflows/test_sign_modules_on_approval.py (1)
52-58: LGTM on commit/push/summary assertions.The test properly validates:
- Bot identity (
github-actions[bot])- Commit message format with
[skip ci]- Push target pattern (
HEAD:${PR_HEAD_REF})- Summary output markers
|
@cursor review this PR |
|
Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings. |
…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
|
@cursor review this PR, I enabled it. |
Bugbot-style review (what I verified and changed)I treated the PR thread as a review checklist and validated it against
Verification: Git: Rebased on the latest remote branch, pushed Note: Fork PRs no longer run this job; maintainers still need another path (e.g. manual sign or   |
- 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>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/TDD_EVIDENCE.md`:
- Around line 5-7: Update TDD_EVIDENCE.md to include the actual Red-state
failing output for the missing workflow: run the failing test command (e.g.,
pytest tests/unit/workflows/test_sign_modules_on_approval.py or the CI job that
referenced sign-modules-on-approval.yml), capture the console output or a
concise excerpt with a timestamp, and insert that block immediately before the
Green-run evidence; reference the specific artifacts in the narrative
(sign-modules-on-approval.yml,
tests/unit/workflows/test_sign_modules_on_approval.py, and pr-orchestrator.yml)
so the TDD chain shows spec -> tests -> failing evidence -> code -> passing
evidence in order.
In `@openspec/specs/modules-pre-commit-quality-parity/spec.md`:
- Around line 10-13: Add a blank line after the heading "#### Scenario:
Signature verification hook is configured" so the subsequent list starts on a
separate paragraph, fixing markdownlint MD022; edit the spec.md content around
the "Scenario: Signature verification hook is configured" heading to insert one
empty line before the first list item ("- **WHEN** a developer installs...") so
the heading and list are correctly separated.
In `@scripts/pre-commit-verify-modules-signature.sh`:
- Around line 9-13: The branch-detection fallback uses GITHUB_REF_NAME (the PR
head) which is incorrect for PR base/target checks; update the logic that sets
_require_signature (currently comparing "$_branch" and "${GITHUB_REF_NAME:-}")
to instead check "${GITHUB_BASE_REF:-}" so that in Actions PR events the base
branch (target) is used; ensure the variables referenced remain _branch and
_require_signature and only replace the fallback env var name from
GITHUB_REF_NAME to GITHUB_BASE_REF.
In `@tests/unit/test_pre_commit_verify_modules_signature_script.py`:
- Around line 11-16: The test currently only checks for substrings; strengthen
it to verify the conditional behavior by asserting that "--require-signature" is
present only inside the main-branch conditional and not in the default path:
update the test that sets or inspects the rendered script/text (variable text)
to use a regex or substring checks to confirm that the sequence '== "main"' (or
the surrounding if/branch header) is followed within that branch body by
"--require-signature", and also add an assertion that the default/else branch
(or the script outside that if) does NOT contain "--require-signature";
reference the existing assertions for "GITHUB_REF_NAME" and '== "main"' and the
"--require-signature" token in your new checks to locate where to add the
stronger assertions.
🪄 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: 57b355f3-28fd-408a-98eb-a34eee722a72
📒 Files selected for processing (12)
.pre-commit-config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/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/tasks.mdopenspec/config.yamlopenspec/specs/modules-pre-commit-quality-parity/spec.mdscripts/pre-commit-verify-modules-signature.shtests/unit/test_pre_commit_quality_parity.pytests/unit/test_pre_commit_verify_modules_signature_script.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.11)
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.12)
🧰 Additional context used
📓 Path-based instructions (5)
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
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/design.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/proposal.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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:
tests/unit/test_pre_commit_verify_modules_signature_script.pytests/unit/test_pre_commit_quality_parity.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/test_pre_commit_verify_modules_signature_script.pytests/unit/test_pre_commit_quality_parity.py
🧠 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
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
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/
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
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: 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
📚 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:
.pre-commit-config.yamlopenspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdscripts/pre-commit-verify-modules-signature.shopenspec/changes/marketplace-06-ci-module-signing/proposal.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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:
.pre-commit-config.yamlopenspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdscripts/pre-commit-verify-modules-signature.shopenspec/changes/marketplace-06-ci-module-signing/proposal.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/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: 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/changes/marketplace-06-ci-module-signing/design.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdscripts/pre-commit-verify-modules-signature.shopenspec/changes/marketplace-06-ci-module-signing/proposal.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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:
openspec/config.yamlopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/proposal.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/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: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
openspec/config.yamldocs/agent-rules/50-quality-gates-and-review.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.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/config.yamlopenspec/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:
openspec/config.yaml
📚 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: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/50-quality-gates-and-review.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
📚 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:
docs/agent-rules/50-quality-gates-and-review.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:
docs/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: Do not implement from the `dev` or `main` checkout by default
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/design.md
🪛 LanguageTool
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] ~35-~35: 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] ~37-~37: The official name of this software platform is spelled with a capital “H”.
Context: ...bmitted]. - [x] 4.2 Add job condition: if: github.event.review.state == 'approved' && (gi...
(GITHUB)
[uncategorized] ~43-~43: The official name of this software platform is spelled with a capital “H”.
Context: ...). - [x] 4.5 Add signing step: compute BASE_REF="origin/${{ github.event.pull_request.base.ref }}"`; for...
(GITHUB)
🪛 markdownlint-cli2 (0.22.0)
openspec/specs/modules-pre-commit-quality-parity/spec.md
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔀 Multi-repo context nold-ai/specfact-cli
Findings for nold-ai/specfact-cli:
-
scripts/sign-modules.py — CLI exposes changed-only, base-ref, bump-version, payload-from-filesystem, passphrase handling; used by workflows/publish and publish-module.py. [::nold-ai/specfact-cli::scripts/sign-modules.py]
-
scripts/verify-modules-signature.py — verifier expects flags including --payload-from-filesystem, --enforce-version-bump and supports --require-signature. Workflows invoke it with those flags. [::nold-ai/specfact-cli::scripts/verify-modules-signature.py]
-
.github/workflows/pr-orchestrator.yml — contains verify-module-signatures invocation that includes --require-signature and --payload-from-filesystem in existing lines. Tests in repo assert conditional VERIFY_CMD logic and presence of VERIFY_CMD+=(--require-signature). [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
-
.github/workflows/sign-modules.yml — existing signing workflow invoking scripts/sign-modules.py and scripts/verify-modules-signature.py; contains examples of using --payload-from-filesystem and --version-check-base. [::nold-ai/specfact-cli::.github/workflows/sign-modules.yml]
-
scripts/pre_commit_code_review.py — present and heavily tested; test suite references it. PR’s change to exclude openspec/changes/*.md will affect this module and its tests. [::nold-ai/specfact-cli::scripts/pre_commit_code_review.py]
-
Tests and docs that reference these tools:
- tests/unit/specfact_cli/registry/test_signing_artifacts.py — checks presence/usage of sign/verify scripts and workflow invocations (search hits asserting verify script exists and includes --require-signature). [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py]
- docs/guides/module-signing-and-key-rotation.md and other docs reference verify/sign command forms; ensure documentation matches new branch-aware behavior. [::nold-ai/specfact-cli::docs/guides/module-signing-and-key-rotation.md]
Implication summary:
- The workflows and pre-commit wrapper introduced by the PR must remain compatible with the CLI flags already implemented by scripts/sign-modules.py and scripts/verify-modules-signature.py — those scripts do expose the flags referenced by the workflows.
- Updating pr-orchestrator to conditionally append --require-signature must align with test assertions that look for VERIFY_CMD+=(--require-signature) and with any documentation/tests that assume always-present --require-signature (several tests/docs check for it).
- Changes to scripts/pre_commit_code_review.py path filtering will impact its unit tests (tests/unit/scripts/test_pre_commit_code_review.py) and should be kept in sync.
🔇 Additional comments (6)
.pre-commit-config.yaml (1)
9-9: Good alignment with branch-aware signing policy.Switching the hook entry to the wrapper keeps local verification behavior consistent with CI boundary rules (
mainenforces signature requirement).tests/unit/test_pre_commit_quality_parity.py (1)
88-103: Nice parity guard for pre-commit wiring.This test cleanly enforces that
verify-module-signaturesresolves through the branch-aware wrapper, which helps keep local and CI policy in sync.openspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.md (1)
38-53: Spec update is consistent with the new branch-boundary trust model.The added requirement and scenarios clearly encode
mainvs non-mainverification behavior and align with the CI-signing flow described in this PR.scripts/pre-commit-verify-modules-signature.sh (1)
15-20: Good verifier invocation baseline and exit-path handling.Always including payload/version-bump checks and using
exechere is solid.openspec/specs/modules-pre-commit-quality-parity/spec.md (1)
8-16: Policy wording update is clear and operationally aligned.The requirement/scenario updates capture the intended pre-commit parity behavior well.
openspec/changes/marketplace-06-ci-module-signing/proposal.md (1)
19-21: Proposal-to-spec parity looks tight across workflow, hook, and impact sections.These updates keep the modules/core signing contract and docs surface aligned for this change set.
Also applies to: 40-42, 47-47, 53-54
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-22: The test function
test_sign_modules_on_approval_trigger_and_job_filter currently checks triggers
and filters but misses assertions for concurrency and permissions; update the
test (in test_sign_modules_on_approval_trigger_and_job_filter) to assert that
the rendered workflow string stored in workflow contains "concurrency:" and
"cancel-in-progress: true" and also contains "permissions:" and "contents:
write" so the workflow enforces a concurrency group and grants contents write
permission.
🪄 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: a43ac1dc-3978-4d80-bbc0-833ec3072224
📒 Files selected for processing (6)
.github/workflows/sign-modules-on-approval.ymlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.mdtests/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.13)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/workflows/test_pr_orchestrator_signing.pytests/unit/workflows/test_sign_modules_on_approval.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/workflows/test_pr_orchestrator_signing.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/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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 (10)
📓 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: .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/
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
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
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: .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
📚 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:
tests/unit/workflows/test_pr_orchestrator_signing.pyopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md.github/workflows/sign-modules-on-approval.ymlopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.mdtests/unit/workflows/test_sign_modules_on_approval.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: Perform `spec -> tests -> failing evidence -> code -> passing evidence` in that order for behavior changes
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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:
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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/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: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md.github/workflows/sign-modules-on-approval.ymlopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/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: This repository enforces the clean-code review gate through hatch run specfact code review run --json --out .specfact/code-review.json
Applied to files:
openspec/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: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md.github/workflows/sign-modules-on-approval.ymlopenspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/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/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: Require public GitHub metadata completeness before implementation when linked issue workflow applies: parent, labels, project assignment, blockers, and blocked-by relationships
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.md
🪛 LanguageTool
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md
[uncategorized] ~33-~33: 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] ~35-~35: 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] ~37-~37: 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)
🔀 Multi-repo context nold-ai/specfact-cli
nold-ai/specfact-cli
-
scripts/verify-modules-signature.py — verifier implementation that supports flags used by workflows (e.g. --payload-from-filesystem, --enforce-version-bump, --require-signature, --version-check-base). [::nold-ai/specfact-cli::scripts/verify-modules-signature.py]
-
scripts/sign-modules.py — signer implementation supporting --changed-only, --base-ref, --bump-version, --payload-from-filesystem and key/passphrase env vars referenced by new workflow. [::nold-ai/specfact-cli::scripts/sign-modules.py]
-
.github/workflows/pr-orchestrator.yml — existing verify step invokes verifier with --require-signature (and version-check-base when BASE_REF set). Tests and PR changes will need to align with this file’s conditional VERIFY_CMD behavior. [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml]
-
.github/workflows/sign-modules.yml — existing signing workflow invokes sign-modules and verifier; contains verifier calls that include --require-signature and optionally --version-check-base. New sign-on-approval workflow in modules repo should match expectations here (flags, fetch/merge-base semantics). [::nold-ai/specfact-cli::.github/workflows/sign-modules.yml]
-
tests/unit/specfact_cli/registry/test_signing_artifacts.py — unit tests assert presence of scripts and workflows and specifically check for "verify-modules-signature.py --require-signature" in workflow/test content; these tests will be sensitive to the PR’s change that makes --require-signature conditional. [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py]
-
tests/unit/workflows/test_trustworthy_green_checks.py — references SIGN_WORKFLOW path (.github/workflows/sign-modules.yml) and inspects jobs; related workflow changes may affect these tests. [::nold-ai/specfact-cli::tests/unit/workflows/test_trustworthy_green_checks.py]
-
docs (multiple) — docs/guides/module-signing-and-key-rotation.md, docs/reference/module-security.md, docs/module-system/module-marketplace.md, docs/guides/publishing-modules.md, docs/agent-rules/50-quality-gates-and-review.md all reference verifier/signing commands and the expectation of --require-signature; documentation should be reviewed to ensure wording matches branch-aware behavior. [::nold-ai/specfact-cli::docs/guides/module-signing-and-key-rotation.md] [::nold-ai/specfact-cli::docs/reference/module-security.md] [::nold-ai/specfact-cli::docs/module-system/module-marketplace.md] [::nold-ai/specfact-cli::docs/guides/publishing-modules.md] [::nold-ai/specfact-cli::docs/agent-rules/50-quality-gates-and-review.md]
Implication summary:
- The core CLI repo already exposes the required signer/verifier flags; workflows and docs/tests in this repo assert always-present --require-signature in places — those tests/docs will need updating or are already covered by paired PR changes to expect conditional/appended --require-signature behavior. [::nold-ai/specfact-cli::]
🔇 Additional comments (11)
openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md (2)
5-7: Red-state evidence still incomplete.The TDD evidence acknowledges the Red phase wasn't captured. While the implicit failure (missing file causing test failure) is understood, capturing actual failing output strengthens the audit trail per the TDD order guideline.
9-44: Green-state evidence is thorough.The verification commands cover workflow tests, contract tests, linting, YAML validation, actionlint, and the SpecFact code review gate. The PR review follow-up notes at Lines 31–35 helpfully document the rationale for same-repo gating and merge-base computation.
openspec/changes/marketplace-06-ci-module-signing/specs/ci-module-signing-on-approval/spec.md (1)
16-17: Spec accurately reflects merge-base and fork-exclusion implementation.The specification changes align with the workflow implementation:
- Merge-base logic at Lines 16–17 and 26–27 matches the workflow's
git merge-base HEAD "origin/${PR_BASE_REF}"computation (see.github/workflows/sign-modules-on-approval.yml:64).- Fork exclusion scenario at Lines 43–48 matches the job condition
github.event.pull_request.head.repo.full_name == github.repository.This ensures the spec serves as a reliable contract for the signing boundary between bundled modules and CI infrastructure.
Also applies to: 26-27, 43-48
tests/unit/workflows/test_pr_orchestrator_signing.py (1)
19-23: Branch-guard sequencing assertion addresses prior feedback.The sequence check at Line 23 (
workflow.index(branch_guard) < workflow.index(require_append)) ensures--require-signatureis guarded by themainbranch conditional. This directly addresses the prior review suggestion about validating the conditional block structure..github/workflows/sign-modules-on-approval.yml (3)
1-27: Workflow structure follows security and CI best practices.Key security measures are well-implemented:
- Same-repo guard (Line 20) prevents unauthorized fork pushes
- Concurrency group (Lines 8–10) prevents race conditions on same PR
permissions: contents: write(Lines 12–13) is appropriately minimal- Secrets are guarded early (Lines 28–34) with clear error messaging
This maintains the trust model where signatures are enforced at
mainwhile feature branches rely on CI signing.
65-69: Signing flags align with core repo script interface.The invocation matches the
sign-modules.pyargument parser in the linkednold-ai/specfact-clirepository:
--changed-only+--base-ref "$MERGE_BASE"for PR-scoped manifest selection--bump-version patchfor auto-incrementing unchanged versions--payload-from-filesystemfor checksum consistency with install verificationThis maintains contract alignment between the modules repo CI and the core CLI signing implementation.
82-82:git add -u -- packages/is appropriate here.The signing workflow discovers existing
module-package.yamlfiles and updates theirintegrityfield and version (if bumped). Sincesign-modules.pyonly modifies existing manifests and never creates new ones,git add -ucorrectly stages the tracked file modifications without including untracked files.openspec/changes/marketplace-06-ci-module-signing/tasks.md (2)
45-50: Task 4.5 correctly documents the signing step implementation.The task now accurately describes:
- Merge-base computation for PR-scoped deltas
--changed-only --base-ref "$MERGE_BASE" --bump-version patch --payload-from-filesystem- Explicit note that
--allow-same-versionis not usedThis addresses the prior review feedback about the discrepancy between task documentation and actual implementation.
29-31: Pre-commit wrapper task aligns with implementation.Task 3.5 correctly documents the
scripts/pre-commit-verify-modules-signature.shwrapper that conditionally applies--require-signaturebased on branch (main viagit rev-parseorGITHUB_REF_NAME). This maintains parity between local pre-commit hooks and CI verification gates.tests/unit/workflows/test_sign_modules_on_approval.py (2)
45-61: Secret guard assertions address prior review feedback.Lines 47–49 now explicitly verify:
- The guard step exists (
"Guard signing secrets")- The empty-check pattern (
'[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]')- The failure exit (
"exit 1")This ensures the guard step (not just env vars) is validated, addressing the prior suggestion.
9-11: Helper function reduces duplication across tests.The
_workflow_text()helper cleanly encapsulates the file read, andREPO_ROOTcalculation is consistent withtest_pr_orchestrator_signing.py. Good pattern for workflow contract tests.
…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
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/specs/ci-integration/spec.md`:
- Around line 38-53: This change introduces a cross-repo contract requiring the
pre-commit hook (verify-modules-signature.py) to mirror the CI job behavior
(verify-module-signatures) around the --require-signature flag and branch
detection; before merging, coordinate and wait for the paired core-repo change
nold-ai/specfact-cli#500 to land, then update this spec to reference the new
core behavior and adjust any workflow/test/docs that rely on
GITHUB_BASE_REF/main detection and adapter-boundary semantics so
verify-modules-signature.py, the CI job verify-module-signatures, and related
docs/tests remain consistent.
In `@openspec/changes/marketplace-06-ci-module-signing/tasks.md`:
- Around line 72-73: Update the checklist item "6.2 Link the PR to the GitHub
issue..." to require explicit parity validation in the paired repo
(nold-ai/specfact-cli) before marking done: require the specfact-cli PR link
plus a confirmation that its CI/workflows/tests/docs have been updated and
passed (parity validation status), and require the PR body to state the parity
check passed (e.g., "parity: confirmed") before closing the marketplace/core PR
so module signature policy and shared workflow semantics cannot drift.
In `@tests/unit/workflows/test_sign_modules_on_approval.py`:
- Around line 51-65: The test is missing an explicit assertion that the
passphrase-empty shell guard is present; add an assertion mirroring the
private-key guard such as asserting the string '[ -z
"${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]' exists in the generated
workflow (in the test_sign_modules_on_approval.py assertions alongside the
existing SPECFACT_MODULE_PRIVATE_SIGN_KEY guard and the "exit 1" check) so the
fail-fast behavior for the passphrase is validated.
- Around line 14-27: The test_sign_modules_on_approval_trigger_and_job_filter
test currently uses loose token checks; update it to assert exact trigger and
filter blocks returned by _workflow_text() to enforce the approval-only
contract: check for the full pull_request trigger block including types:
[submitted], the exact conditional "if: github.event.review.state == 'approved'
&& (github.event.pull_request.base.ref == 'dev' ||
github.event.pull_request.base.ref == 'main')" (or equivalent exact string used
in the workflow), the repo equality
"github.event.pull_request.head.repo.full_name == github.repository", and exact
concurrency/permissions lines ("concurrency:\n cancel-in-progress: true" and
"permissions:\n contents: write") instead of token-level checks; locate these
assertions in test_sign_modules_on_approval_trigger_and_job_filter and replace
the loose contains assertions with equality/substring assertions that match the
full expected snippets produced by _workflow_text().
🪄 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: 63ae8aef-c994-4acd-ba91-c05c0ac2039d
📒 Files selected for processing (10)
docs/agent-rules/50-quality-gates-and-review.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/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.shtests/unit/test_pre_commit_verify_modules_signature_script.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.13)
- GitHub Check: quality (3.12)
- GitHub Check: quality (3.11)
🧰 Additional context used
📓 Path-based instructions (5)
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/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.md
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
**/*.{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/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_sign_modules_on_approval.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/test_pre_commit_verify_modules_signature_script.pytests/unit/workflows/test_sign_modules_on_approval.py
🧠 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: .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/
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
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
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: .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
📚 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/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdscripts/pre-commit-verify-modules-signature.shdocs/agent-rules/50-quality-gates-and-review.mdopenspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdtests/unit/workflows/test_sign_modules_on_approval.pyopenspec/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: Enforce module signatures and version bumps when signed module assets or manifests are affected
Applied to files:
openspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdscripts/pre-commit-verify-modules-signature.shdocs/agent-rules/50-quality-gates-and-review.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: Signed module or manifest changes require version-bump review and verify-modules-signature
Applied to files:
openspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/design.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdscripts/pre-commit-verify-modules-signature.shdocs/agent-rules/50-quality-gates-and-review.mdopenspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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:
openspec/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mdopenspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/changes/marketplace-06-ci-module-signing/tasks.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/specs/modules-pre-commit-quality-parity/spec.mdopenspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.mddocs/agent-rules/50-quality-gates-and-review.mdopenspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.mdopenspec/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: Do not implement from the `dev` or `main` checkout by default
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/design.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: Detect repository root, active branch, and worktree state before proceeding with implementation
Applied to files:
scripts/pre-commit-verify-modules-signature.sh
📚 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/50-quality-gates-and-review.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
📚 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:
docs/agent-rules/50-quality-gates-and-review.mdopenspec/config.yamlopenspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.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/config.yamlopenspec/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:
openspec/config.yaml
📚 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: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: Require public GitHub metadata completeness before implementation when linked issue workflow applies: parent, labels, project assignment, blockers, and blocked-by relationships
Applied to files:
openspec/changes/marketplace-06-ci-module-signing/tasks.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)
🔀 Multi-repo context nold-ai/specfact-cli
Findings for nold-ai/specfact-cli
-
Workflow that currently invokes verifier with always-present --require-signature:
- .github/workflows/pr-orchestrator.yml — Verify Module Signatures step: runs python scripts/verify-modules-signature.py --require-signature ... [::nold-ai/specfact-cli::.github/workflows/pr-orchestrator.yml: Verify bundled module checksums and signatures step]
-
Separate workflow also assumes --require-signature is always used:
- .github/workflows/Module Signature Hardening.yml — Verify bundled module signatures step uses --require-signature [::nold-ai/specfact-cli::.github/workflows/Module Signature Hardening.yml: Verify bundled module signatures step]
- .github/workflows/sign-modules.yml — contains conditional invocations that include --require-signature in multiple branches (lines referencing --version-check-base and fallback) [::nold-ai/specfact-cli::.github/workflows/sign-modules.yml]
-
Tests that assert workflow/scripts include a literal "--require-signature" (will be sensitive to making the flag conditional):
- tests/unit/specfact_cli/registry/test_signing_artifacts.py — asserts presence of "verify-modules-signature.py --require-signature" in workflow content [::nold-ai/specfact-cli::tests/unit/specfact_cli/registry/test_signing_artifacts.py:~508]
- tests/unit/workflows/test_trustworthy_green_checks.py — references SIGN_MODULES = .github/workflows/sign-modules.yml and inspects sign workflow jobs (may assume current behavior) [::nold-ai/specfact-cli::tests/unit/workflows/test_trustworthy_green_checks.py:15,163]
-
Scripts and docs referencing always-required signature behavior (may need wording updates to reflect branch-aware policy):
- scripts/pre-commit-smart-checks.sh — runs hatch run ./scripts/verify-modules-signature.py --require-signature --enforce-version-bump [::nold-ai/specfact-cli::scripts/pre-commit-smart-checks.sh]
- scripts/setup-git-hooks.sh — documents hatch run ./scripts/verify-modules-signature.py --require-signature [::nold-ai/specfact-cli::scripts/setup-git-hooks.sh]
- docs and openspec entries with multiple references to
hatch run ./scripts/verify-modules-signature.py --require-signature(e.g., docs/guides/module-signing-and-key-rotation.md, openspec/specs/*) [::nold-ai/specfact-cli::docs/guides/module-signing-and-key-rotation.md] [::nold-ai/specfact-cli::openspec/**]
-
Signer/verifier implementations support the flags used by the new workflows:
- scripts/verify-modules-signature.py — supports --payload-from-filesystem, --enforce-version-bump, --require-signature, --version-check-base [::nold-ai/specfact-cli::scripts/verify-modules-signature.py]
- scripts/sign-modules.py — supports --changed-only, --base-ref, --bump-version, --payload-from-filesystem [::nold-ai/specfact-cli::scripts/sign-modules.py]
Implication: Tests and documentation in this repo expect literal --require-signature usage; making the flag conditional (as in the modules-repo PR) will require updating tests/docs/workflow assertions here (or ensuring workflows are updated in this repo to match branch-aware behavior). Also, existing workflows in this repo still invoke --require-signature unconditionally and may need alignment with the branch-aware wrapper and sign-on-approval workflow introduced in the PR under review.
🔇 Additional comments (9)
scripts/pre-commit-verify-modules-signature.sh (1)
9-23: Branch-aware signature gate wiring looks correct.The wrapper cleanly mirrors CI policy: checksum/version-bump verification always runs, and
--require-signatureis injected only when target branch resolves tomain.openspec/specs/modules-pre-commit-quality-parity/spec.md (1)
8-16: Spec update is consistent with implemented signing trust boundaries.The requirement/scenario wording correctly captures checksum+version enforcement on all branches and signature enforcement at the
mainboundary.openspec/config.yaml (1)
32-35: Quality-gate context is accurately synchronized with the new pre-commit flow.The ordering and
main-boundary signature rule are documented clearly and match the workflow intent.docs/agent-rules/50-quality-gates-and-review.md (1)
46-54: Rule documentation now matches the enforced policy.Good update: the quality gate and pre-commit sections now describe conditional signature enforcement at the
mainboundary instead of unconditional enforcement.openspec/changes/marketplace-06-ci-module-signing/design.md (1)
16-19: Design deltas are coherent with the implemented branch-aware trust model.The wrapper behavior and non-goal clarification correctly preserve dev-branch flow while keeping signature enforcement at the
mainboundary.Also applies to: 33-34
tests/unit/test_pre_commit_verify_modules_signature_script.py (1)
20-27: Nice hardening of policy-contract coverage.The test now verifies
--require-signatureis scoped to the true branch and absent from the false path, which is the critical behavior contract.openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md (1)
12-36: Red-state evidence is now concrete and auditable.Including the failing command/output excerpt before green runs makes the TDD chain verifiable for this workflow behavior change.
openspec/changes/marketplace-06-ci-module-signing/tasks.md (1)
3-71: Checklist progression matches the implemented CI/signing design.The completed items align with the branch-aware verify/signing flow and test evidence expectations.
tests/unit/workflows/test_sign_modules_on_approval.py (1)
29-47: Coverage for checkout/setup, discovery outputs, and write-back summary looks solid.These assertions capture the critical CI behavior for signing execution and reporting.
Also applies to: 68-76
| ### Requirement: pre-commit verify mirrors pr-orchestrator signature policy | ||
|
|
||
| The repository pre-commit hook that runs `verify-modules-signature.py` SHALL apply the same branch rule as the `verify-module-signatures` CI job: omit `--require-signature` unless the integration target is `main` (local branch `main`, or `GITHUB_BASE_REF=main` on pull request events in Actions). | ||
|
|
||
| #### Scenario: Commit on a feature branch without a signing key | ||
|
|
||
| - **WHEN** a developer commits on a branch other than `main` | ||
| - **AND** manifests satisfy checksum and version-bump policy but lack a valid signature | ||
| - **THEN** the pre-commit signature hook SHALL pass (no `--require-signature`) | ||
| - **AND** the developer SHALL remain unblocked until a `main`-targeting flow enforces signatures in CI | ||
|
|
||
| #### Scenario: Commit on main requires signatures | ||
|
|
||
| - **WHEN** a developer commits on branch `main` | ||
| - **AND** any `packages/*/module-package.yaml` lacks a valid signature under `--require-signature` | ||
| - **THEN** the pre-commit signature hook SHALL fail |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Coordinate merge timing with the paired core-repo policy change.
This requirement defines a cross-repo contract (“pre-commit/CI mirror policy”). Before rollout, confirm nold-ai/specfact-cli#500 lands with its related workflow/test/docs updates so adapter-boundary semantics don’t drift between modules and core.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@openspec/changes/marketplace-06-ci-module-signing/specs/ci-integration/spec.md`
around lines 38 - 53, This change introduces a cross-repo contract requiring the
pre-commit hook (verify-modules-signature.py) to mirror the CI job behavior
(verify-module-signatures) around the --require-signature flag and branch
detection; before merging, coordinate and wait for the paired core-repo change
nold-ai/specfact-cli#500 to land, then update this spec to reference the new
core behavior and adjust any workflow/test/docs that rely on
GITHUB_BASE_REF/main detection and adapter-boundary semantics so
verify-modules-signature.py, the CI job verify-module-signatures, and related
docs/tests remain consistent.
| - [x] 6.2 Link the PR to the GitHub issue created in 1.2 and to the paired specfact-cli PR. | ||
| *(Closes #185 in PR body; link specfact-cli PR manually when it exists.)* |
There was a problem hiding this comment.
Make paired-core parity an explicit done-criterion before closing.
Line 72–73 is marked done, but it still frames the paired specfact-cli link as manual/future. For a trust-model contract change, this should explicitly require parity validation status in nold-ai/specfact-cli (workflows/tests/docs), otherwise modules/core can drift on signature policy.
🛠️ Suggested task-text adjustment
-- [x] 6.2 Link the PR to the GitHub issue created in 1.2 and to the paired specfact-cli PR.
- *(Closes `#185` in PR body; link specfact-cli PR manually when it exists.)*
+- [x] 6.2 Link the PR to the GitHub issue created in 1.2 and to the paired specfact-cli PR,
+ and record parity status for branch-aware signature policy in core workflows/tests/docs.
+ *(Closes `#185` in PR body; paired core PR: `#500`.)*Based on learnings: “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.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openspec/changes/marketplace-06-ci-module-signing/tasks.md` around lines 72 -
73, Update the checklist item "6.2 Link the PR to the GitHub issue..." to
require explicit parity validation in the paired repo (nold-ai/specfact-cli)
before marking done: require the specfact-cli PR link plus a confirmation that
its CI/workflows/tests/docs have been updated and passed (parity validation
status), and require the PR body to state the parity check passed (e.g.,
"parity: confirmed") before closing the marketplace/core PR so module signature
policy and shared workflow semantics cannot drift.
| def test_sign_modules_on_approval_trigger_and_job_filter() -> None: | ||
| workflow = _workflow_text() | ||
| assert "pull_request_review:" in workflow | ||
| assert "types:" in workflow | ||
| assert "submitted" in workflow | ||
| assert "github.event.review.state == 'approved'" in workflow | ||
| assert "github.event.pull_request.base.ref == 'dev'" in workflow | ||
| assert "github.event.pull_request.base.ref == 'main'" in workflow | ||
| assert "github.event.pull_request.head.repo.full_name == github.repository" in workflow | ||
| assert "concurrency:" in workflow | ||
| assert "cancel-in-progress: true" in workflow | ||
| assert "permissions:" in workflow | ||
| assert "contents: write" in workflow | ||
|
|
There was a problem hiding this comment.
Harden trigger/filter assertions to enforce exact workflow contract.
Current checks are token-level ("types:", "submitted"), so the test can still pass if extra review types or broader conditions are introduced. Tighten this to exact contract strings for the approval gate.
🧪 Suggested assertion tightening
- assert "types:" in workflow
- assert "submitted" in workflow
+ assert "types: [submitted]" in workflow
+ assert "github.event.review.state == 'approved'" in workflow
+ assert "(github.event.pull_request.base.ref == 'dev' || github.event.pull_request.base.ref == 'main')" in workflowAs per coding guidelines: “tests/**/*.py: Contract-first and integration tests... Ensure changes to adapters or bridges have targeted coverage.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/workflows/test_sign_modules_on_approval.py` around lines 14 - 27,
The test_sign_modules_on_approval_trigger_and_job_filter test currently uses
loose token checks; update it to assert exact trigger and filter blocks returned
by _workflow_text() to enforce the approval-only contract: check for the full
pull_request trigger block including types: [submitted], the exact conditional
"if: github.event.review.state == 'approved' &&
(github.event.pull_request.base.ref == 'dev' ||
github.event.pull_request.base.ref == 'main')" (or equivalent exact string used
in the workflow), the repo equality
"github.event.pull_request.head.repo.full_name == github.repository", and exact
concurrency/permissions lines ("concurrency:\n cancel-in-progress: true" and
"permissions:\n contents: write") instead of token-level checks; locate these
assertions in test_sign_modules_on_approval_trigger_and_job_filter and replace
the loose contains assertions with equality/substring assertions that match the
full expected snippets produced by _workflow_text().
| assert "Guard signing secrets" in workflow | ||
| assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]' in workflow | ||
| assert "exit 1" in workflow | ||
| assert "MERGE_BASE=" in workflow | ||
| assert "git merge-base HEAD" in workflow | ||
| assert 'git fetch origin "${PR_BASE_REF}"' in workflow | ||
| assert "--no-tags" in workflow | ||
| assert "scripts/sign-modules.py" in workflow | ||
| assert "--changed-only" in workflow | ||
| assert "--base-ref" in workflow | ||
| assert '"$MERGE_BASE"' in workflow | ||
| assert "--bump-version patch" in workflow | ||
| assert "--payload-from-filesystem" in workflow | ||
| assert "SPECFACT_MODULE_PRIVATE_SIGN_KEY" in workflow | ||
| assert "SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE" in workflow |
There was a problem hiding this comment.
Add an explicit assertion for passphrase-empty guard behavior.
The test validates key-empty guard logic, but it does not explicitly assert the passphrase-empty shell guard expression. Presence of the variable name alone is weaker than asserting the fail-fast check.
🔐 Suggested assertion addition
assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]' in workflow
+ assert '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]' in workflow
assert "exit 1" in workflowAs per coding guidelines: “tests/**/*.py: Contract-first and integration tests... Ensure changes to adapters or bridges have targeted coverage.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/workflows/test_sign_modules_on_approval.py` around lines 51 - 65,
The test is missing an explicit assertion that the passphrase-empty shell guard
is present; add an assertion mirroring the private-key guard such as asserting
the string '[ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]' exists in
the generated workflow (in the test_sign_modules_on_approval.py assertions
alongside the existing SPECFACT_MODULE_PRIVATE_SIGN_KEY guard and the "exit 1"
check) so the fail-fast behavior for the passphrase is validated.
2 participants
Implements
marketplace-06-ci-module-signing(OpenSpec).verify-module-signaturesinpr-orchestrator.yml(no--require-signaturefor dev-targeting events).sign-modules-on-approval.ymlon approved PR reviews targetingdevormain.tests/unit/workflows/.*.mdin pre-commit review gate (avoids false positives on task lists).Closes #185
Related: nold-ai/specfact-cli#500
Made with Cursor