Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions .github/workflows/pr-orchestrator.yml
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,31 @@ jobs:
python -m pip install pyyaml cryptography cffi
- name: Verify bundled module signatures and version bumps
run: |
set -euo pipefail
TARGET_BRANCH=""
if [ "${{ github.event_name }}" = "pull_request" ]; then
TARGET_BRANCH="${{ github.event.pull_request.base.ref }}"
else
TARGET_BRANCH="${GITHUB_REF#refs/heads/}"
fi

BASE_REF=""
if [ "${{ github.event_name }}" = "pull_request" ]; then
BASE_REF="origin/${{ github.event.pull_request.base.ref }}"
fi

if [ -z "${SPECFACT_MODULE_PUBLIC_SIGN_KEY:-}" ] && [ -z "${SPECFACT_MODULE_SIGNING_PUBLIC_KEY_PEM:-}" ]; then
echo "warning: no public signing key secret set; verifier must resolve key from repo/default path"
fi

VERIFY_CMD=(python scripts/verify-modules-signature.py --payload-from-filesystem --enforce-version-bump)
if [ "$TARGET_BRANCH" = "main" ]; then
VERIFY_CMD+=(--require-signature)
fi
if [ -n "$BASE_REF" ]; then
python scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump --version-check-base "$BASE_REF"
else
python scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump
VERIFY_CMD+=(--version-check-base "$BASE_REF")
fi
"${VERIFY_CMD[@]}"

quality:
name: quality (${{ matrix.python-version }})
Expand Down
101 changes: 101 additions & 0 deletions .github/workflows/sign-modules-on-approval.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# yaml-language-server: $schema=https://json.schemastore.org/github-workflow.json
name: sign-modules-on-approval

on:
pull_request_review:
types: [submitted]

concurrency:
group: sign-modules-on-approval-${{ github.event.pull_request.number }}
cancel-in-progress: true

permissions:
contents: write

jobs:
sign-modules:
if: >-
github.event.review.state == 'approved' &&
(github.event.pull_request.base.ref == 'dev' || github.event.pull_request.base.ref == 'main') &&
github.event.pull_request.head.repo.full_name == github.repository
runs-on: ubuntu-latest
env:
SPECFACT_MODULE_PRIVATE_SIGN_KEY: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY }}
SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE: ${{ secrets.SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE }}
PR_BASE_REF: ${{ github.event.pull_request.base.ref }}
PR_HEAD_REF: ${{ github.event.pull_request.head.ref }}
steps:
- name: Guard signing secrets
run: |
set -euo pipefail
if [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ] || [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE:-}" ]; then
echo "::error::SPECFACT_MODULE_PRIVATE_SIGN_KEY and SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE must be set"
exit 1
fi

- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }}
fetch-depth: 0
Comment thread
djm81 marked this conversation as resolved.

- name: Set up Python 3.12
uses: actions/setup-python@v5
with:
python-version: "3.12"

- name: Install signing dependencies
run: |
python -m pip install --upgrade pip
python -m pip install pyyaml beartype icontract cryptography cffi

- name: Discover module manifests
id: discover
run: |
set -euo pipefail
mapfile -t MANIFESTS < <(find packages -name 'module-package.yaml' -type f | sort)
echo "manifests_count=${#MANIFESTS[@]}" >> "$GITHUB_OUTPUT"
echo "Discovered ${#MANIFESTS[@]} module-package.yaml file(s) under packages/"

Comment thread
djm81 marked this conversation as resolved.
- name: Sign changed module manifests
id: sign
run: |
set -euo pipefail
git fetch origin "${PR_BASE_REF}" --no-tags
MERGE_BASE="$(git merge-base HEAD "origin/${PR_BASE_REF}")"
python scripts/sign-modules.py \
--changed-only \
--base-ref "$MERGE_BASE" \
--bump-version patch \
--payload-from-filesystem

- name: Commit and push signed manifests
id: commit
run: |
set -euo pipefail
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
if [ -z "$(git status --porcelain -- packages/)" ]; then
echo "changed=false" >> "$GITHUB_OUTPUT"
echo "No manifest changes to commit."
exit 0
fi
git add -u -- packages/
git commit -m "chore(modules): ci sign changed modules [skip ci]"
echo "changed=true" >> "$GITHUB_OUTPUT"
git push origin "HEAD:${PR_HEAD_REF}"
Comment thread
djm81 marked this conversation as resolved.

- name: Write job summary
if: always()
env:
COMMIT_CHANGED: ${{ steps.commit.outputs.changed }}
MANIFESTS_COUNT: ${{ steps.discover.outputs.manifests_count }}
run: |
{
echo "### Module signing (CI approval)"
echo "Manifests discovered under \`packages/\`: ${MANIFESTS_COUNT:-unknown}"
if [ "${COMMIT_CHANGED}" = "true" ]; then
echo "Committed signed manifest updates to ${PR_HEAD_REF}."
else
echo "No changes detected (manifests already signed or no module changes on this PR vs merge-base)."
fi
} >> "$GITHUB_STEP_SUMMARY"
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ repos:
hooks:
- id: verify-module-signatures
name: Verify module signatures and version bumps
entry: hatch run ./scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump
entry: ./scripts/pre-commit-verify-modules-signature.sh
language: system
pass_filenames: false
always_run: true
Expand Down
4 changes: 2 additions & 2 deletions docs/agent-rules/50-quality-gates-and-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,15 @@ depends_on:
3. `hatch run lint`
4. `hatch run yaml-lint`
5. `hatch run check-bundle-imports`
6. `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump`
6. `hatch run verify-modules-signature --payload-from-filesystem --enforce-version-bump` (add `--require-signature` when checking as for `main`; matches CI and `scripts/pre-commit-verify-modules-signature.sh`)
7. `hatch run contract-test`
8. `hatch run smart-test`
9. `hatch run test`
10. `hatch run specfact code review run --json --out .specfact/code-review.json` (full-repo scope when required: add `--scope full`; machine-readable evidence lives at `.specfact/code-review.json` and unresolved findings block merge unless an explicit exception is documented)

## Pre-commit order

1. Module signature verification (`.pre-commit-config.yaml`, `fail_fast: true` so a failing earlier hook never runs later stages).
1. Module signature verification via `scripts/pre-commit-verify-modules-signature.sh` (`.pre-commit-config.yaml`; `fail_fast: true` so a failing earlier hook never runs later stages). The hook adds `--require-signature` on branch `main`, or when `GITHUB_BASE_REF` is `main` (PR target in Actions).
2. **Block 1** — four separate hooks (each flushes pre-commit output when it exits, so you see progress between stages): `pre-commit-quality-checks.sh block1-format` (always), `block1-yaml` when staged `*.yaml` / `*.yml`, `block1-bundle` (always), `block1-lint` when staged `*.py` / `*.pyi`.
3. **Block 2** — `pre-commit-quality-checks.sh block2` (skipped for “safe-only” staged paths): `hatch run python scripts/pre_commit_code_review.py …` on **staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/`** (excluding `TDD_EVIDENCE.md`), then `contract-test-status` / `hatch run contract-test`.

Expand Down
87 changes: 87 additions & 0 deletions openspec/changes/marketplace-06-ci-module-signing/TDD_EVIDENCE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# TDD evidence: marketplace-06-ci-module-signing

Chain: delta specs (`ci-integration`, `ci-module-signing-on-approval`) → workflow tests → **Red** evidence →
implement `.github/workflows/sign-modules-on-approval.yml` + `pr-orchestrator.yml` → **Green** evidence.

## Red (workflow tests before `sign-modules-on-approval.yml` existed)

Artifacts: `tests/unit/workflows/test_sign_modules_on_approval.py` (expects
`.github/workflows/sign-modules-on-approval.yml`), and companion tests for
`pr-orchestrator.yml` / pre-commit parity added in the same change.

Captured **2026-04-14** by temporarily moving the workflow file aside and running one failing test:

```bash
python -m pytest \
tests/unit/workflows/test_sign_modules_on_approval.py::test_sign_modules_on_approval_trigger_and_job_filter \
-q
```

Excerpt (failure: missing `sign-modules-on-approval.yml`):

```text
tests/unit/workflows/test_sign_modules_on_approval.py F [100%]

=================================== FAILURES ===================================
_____________ test_sign_modules_on_approval_trigger_and_job_filter _____________

def test_sign_modules_on_approval_trigger_and_job_filter() -> None:
> workflow = _workflow_text()
...
E FileNotFoundError: [Errno 2] No such file or directory: '.../.github/workflows/sign-modules-on-approval.yml'

=========================== short test summary info ============================
FAILED tests/unit/workflows/test_sign_modules_on_approval.py::test_sign_modules_on_approval_trigger_and_job_filter
============================== 1 failed in 0.22s ===============================
```

## Green (verification)

Run from worktree `feature/marketplace-06-ci-module-signing` (repo root of this change):

```bash
python -m pytest tests/unit/workflows/ tests/unit/test_pre_commit_quality_parity.py \
tests/unit/test_pre_commit_verify_modules_signature_script.py -q
# workflow + pre-commit parity tests (2026-04-14)

hatch run contract-test
# 555 passed (2026-04-14, after sign-modules-on-approval hardening)

hatch run lint
# All checks passed

hatch run yaml-lint
# Validated manifests / registry

actionlint .github/workflows/pr-orchestrator.yml .github/workflows/sign-modules-on-approval.yml
# exit 0
```

### PR review follow-up (sign-modules-on-approval)

- Same-repo PRs only: job `if` adds `github.event.pull_request.head.repo.full_name == github.repository` so fork PRs are not pushed to the wrong remote.
- Checkout uses `head.ref` (branch tip) to align with `git push origin "HEAD:${PR_HEAD_REF}"`.
- Signing uses `MERGE_BASE="$(git merge-base HEAD "origin/${PR_BASE_REF}")"` after `git fetch origin "${PR_BASE_REF}" --no-tags` so `--changed-only` is PR-scoped vs the merge-base, not the moving base-branch tip.

SpecFact code review (use Hatch so `semgrep` resolves to the project venv; system `specfact` may
invoke a broken `semgrep` shim):

```bash
hatch run specfact code review run --json --out .specfact/code-review.json --include-tests \
tests/unit/workflows/test_pr_orchestrator_signing.py \
tests/unit/workflows/test_sign_modules_on_approval.py
# exit 0 (2026-04-14)
```

## Pre-commit note

`scripts/pre_commit_code_review.py` now skips `openspec/changes/**/*.md` (including `tasks.md`) so
the SpecFact review gate does not parse Markdown task lists as Python (false positives).

## PR

- Opened: [specfact-cli-modules#188](https://github.com/nold-ai/specfact-cli-modules/pull/188) (target `dev`).

## Cleanup (post-merge, human)

Tasks 6.3–6.4 (remove worktree / branch after merge, record cleanup): pending merge.
10 changes: 6 additions & 4 deletions openspec/changes/marketplace-06-ci-module-signing/design.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ This repo differs from `specfact-cli` in two important ways that shape the imple
`pr-orchestrator.yml` has a `verify-module-signatures` job, and it always runs
`--require-signature` regardless of target branch.

3. **No pre-commit signing check**: The modules pre-commit (`scripts/pre-commit-quality-checks.sh`)
does NOT include a module-signature verification step. No pre-commit changes are required.
3. **Pre-commit verify hook**: `.pre-commit-config.yaml` already runs `verify-modules-signature.py` on
every commit. That entry is replaced with a thin wrapper that mirrors `pr-orchestrator` policy
(`--require-signature` when the current branch is `main`, or `GITHUB_BASE_REF` is `main` in Actions).
Block 1 quality stages in `pre-commit-quality-checks.sh` are unchanged.

Both repos share the same `scripts/sign-modules.py` logic (or a copy of it) and the same GitHub
secrets (`SPECFACT_MODULE_PRIVATE_SIGN_KEY`, `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE`), which
Expand All @@ -28,8 +30,8 @@ are already configured via `publish-modules.yml`.
- Enable non-interactive development on feature/dev branches without a local private key.

**Non-Goals:**
- Adding a pre-commit signature verification step (none exists today; introducing one would
recreate the problem).
- Adding *new* mandatory local signing with a private key (the wrapper relaxes `--require-signature`
off `main` instead).
- Adding a `sign-modules.yml` hardening workflow (out of scope; this repo's release flow differs
from specfact-cli).
- Changing `publish-modules.yml` or the registry index logic.
Expand Down
11 changes: 8 additions & 3 deletions openspec/changes/marketplace-06-ci-module-signing/proposal.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ policy set by `specfact-cli/marketplace-06-ci-module-signing`.
via CI secrets, and commits signed manifests back to the PR branch.
- **MODIFY**: `.github/workflows/pr-orchestrator.yml` `verify-module-signatures` job — drop
`--require-signature` for PRs and pushes targeting `dev`; keep it for `main`-targeting events.
- **NO CHANGE**: `scripts/pre-commit-quality-checks.sh` — the modules pre-commit does not include
a module-signature verification step; no pre-commit changes are needed.
- **MODIFY**: `.pre-commit-config.yaml` verify hook — run `scripts/pre-commit-verify-modules-signature.sh`
so local verification matches CI branch policy (`--require-signature` only on `main`; checksum +
version enforcement elsewhere). `scripts/pre-commit-quality-checks.sh` itself is unchanged.
- **NO CHANGE**: `publish-modules.yml` — handles signing at publish/registry-update time;
unchanged.
- **NO CHANGE**: Module install-time verification (end users always install from the main registry;
Expand All @@ -36,17 +37,21 @@ policy set by `specfact-cli/marketplace-06-ci-module-signing`.
- `ci-integration`: The `verify-module-signatures` job in `pr-orchestrator.yml` applies a
branch-aware policy — checksum-only for dev-targeting events, full `--require-signature` for
main-targeting events.
- `modules-pre-commit-quality-parity`: The always-run pre-commit verify step matches that policy
(`main` only: `--require-signature`; other branches: checksum + version bump only).

## Impact

- **Affected workflows**: `.github/workflows/pr-orchestrator.yml`
- **New workflow**: `.github/workflows/sign-modules-on-approval.yml`
- **Pre-commit**: `.pre-commit-config.yaml`, new `scripts/pre-commit-verify-modules-signature.sh`
- **GitHub secrets used** (already configured via publish-modules.yml):
`SPECFACT_MODULE_PRIVATE_SIGN_KEY`, `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE`
- **Permissions added**: `contents: write` on the new signing workflow
- **Module manifest paths**: `packages/*/module-package.yaml` (found under `packages/` in this
repo, not `src/` or `modules/` as in specfact-cli)
- **No Python source changes**: all modifications are to YAML workflows only
- **Supporting changes**: shell wrapper + unit tests for pre-commit parity; main spec
`openspec/specs/modules-pre-commit-quality-parity/spec.md` updated to match behavior
- **Paired change**: `specfact-cli/marketplace-06-ci-module-signing` — covers the pre-commit hook,
`sign-modules.yml`, and pr-orchestrator changes in the core CLI repo
- **Source Tracking**:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,20 @@ for pull requests or pushes targeting `dev`; it SHALL enforce `--require-signatu
- **WHEN** a commit is pushed to `main` (post-merge)
- **THEN** the `verify-module-signatures` job SHALL run with `--require-signature`
- **AND** fail if any `packages/*/module-package.yaml` lacks a valid signature

### 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
Comment on lines +38 to +53
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ manifests back to the PR branch.
- **WHEN** a pull request targeting `dev` is approved by a reviewer
- **AND** the PR contains changes to one or more files under `packages/`
- **THEN** the CI signing workflow SHALL discover all `packages/*/module-package.yaml` manifests
whose payload changed relative to `origin/dev`
whose payload changed on the PR branch since the merge-base with `origin/dev` (not merely
divergent from the moving `origin/dev` tip)
- **AND** SHALL sign them using `SPECFACT_MODULE_PRIVATE_SIGN_KEY` and
`SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE`
- **AND** SHALL commit the updated manifests back to the PR branch
Expand All @@ -22,7 +23,8 @@ manifests back to the PR branch.

- **WHEN** a pull request targeting `main` is approved
- **AND** the PR contains changes to one or more files under `packages/`
- **THEN** the CI signing workflow SHALL sign all changed manifests relative to `origin/main`
- **THEN** the CI signing workflow SHALL sign all changed manifests relative to the merge-base
between the PR head and `origin/main`
- **AND** SHALL commit the signed manifests back to the PR branch before merge

#### Scenario: PR approved with no package changes
Expand All @@ -38,6 +40,13 @@ manifests back to the PR branch.
- **THEN** the workflow SHALL fail with a clear error naming the missing secret
- **AND** SHALL NOT commit partial changes

#### Scenario: Fork PR is out of scope for automated signing

- **WHEN** a pull request targets `dev` or `main` but the head branch lives in a fork
(`head.repo` differs from the base repository)
- **THEN** the signing workflow SHALL NOT run (the default `GITHUB_TOKEN` cannot push to the
contributor fork; maintainers sign or merge via same-repo branches instead)

### Requirement: Manifest discovery covers packages directory

The signing workflow SHALL discover module manifests from the `packages/` directory tree, not only
Expand Down
Loading
Loading