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
31 changes: 15 additions & 16 deletions .github/workflows/pr-orchestrator.yml
Original file line number Diff line number Diff line change
Expand Up @@ -80,29 +80,28 @@ jobs:
- 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

if [ "${{ github.event_name }}" = "pull_request" ]; then
BASE_REF="origin/${{ github.event.pull_request.base.ref }}"
TARGET_BRANCH="${{ github.event.pull_request.base.ref }}"
VERIFY_CMD+=(--version-check-base "$BASE_REF")
if [ "$TARGET_BRANCH" = "dev" ]; then
VERIFY_CMD+=(--metadata-only)
elif [ "$TARGET_BRANCH" = "main" ] && \
[ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]; then
VERIFY_CMD+=(--require-signature)
fi
else
if [ "${{ github.ref_name }}" = "main" ]; then
VERIFY_CMD+=(--require-signature)
fi
fi

"${VERIFY_CMD[@]}"

quality:
Expand Down
53 changes: 43 additions & 10 deletions .github/workflows/sign-modules-on-approval.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,40 @@ permissions:

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: Eligibility gate (required status check)
id: gate
run: |
set -euo pipefail
if [ "${{ github.event.review.state }}" != "approved" ]; then
echo "sign=false" >> "$GITHUB_OUTPUT"
echo "::notice::Skipping module signing: review state is not approved."
exit 0
fi
base_ref="${{ github.event.pull_request.base.ref }}"
if [ "$base_ref" != "dev" ] && [ "$base_ref" != "main" ]; then
echo "sign=false" >> "$GITHUB_OUTPUT"
echo "::notice::Skipping module signing: base branch is not dev or main."
exit 0
fi
head_repo="${{ github.event.pull_request.head.repo.full_name }}"
this_repo="${{ github.repository }}"
if [ "$head_repo" != "$this_repo" ]; then
echo "sign=false" >> "$GITHUB_OUTPUT"
echo "::notice::Skipping module signing: fork PR (head repo differs from target repo)."
exit 0
fi
echo "sign=true" >> "$GITHUB_OUTPUT"
echo "Eligible for module signing (approved, same-repo PR to dev or main)."

- name: Guard signing secrets
if: steps.gate.outputs.sign == 'true'
run: |
set -euo pipefail
if [ -z "${SPECFACT_MODULE_PRIVATE_SIGN_KEY:-}" ]; then
Expand All @@ -38,21 +60,25 @@ jobs:
fi

- uses: actions/checkout@v4
if: steps.gate.outputs.sign == 'true'
with:
ref: ${{ github.event.pull_request.head.sha }}
fetch-depth: 0

- name: Set up Python 3.12
if: steps.gate.outputs.sign == 'true'
uses: actions/setup-python@v5
with:
python-version: "3.12"

- name: Install signing dependencies
if: steps.gate.outputs.sign == 'true'
run: |
python -m pip install --upgrade pip
python -m pip install pyyaml beartype icontract cryptography cffi

- name: Discover module manifests
if: steps.gate.outputs.sign == 'true'
id: discover
run: |
set -euo pipefail
Expand All @@ -61,6 +87,7 @@ jobs:
echo "Discovered ${#MANIFESTS[@]} module-package.yaml file(s) under packages/"

- name: Sign changed module manifests
if: steps.gate.outputs.sign == 'true'
id: sign
run: |
set -euo pipefail
Expand All @@ -73,6 +100,7 @@ jobs:
--payload-from-filesystem

- name: Commit and push signed manifests
if: steps.gate.outputs.sign == 'true'
id: commit
run: |
set -euo pipefail
Expand All @@ -94,15 +122,20 @@ jobs:
- name: Write job summary
if: always()
env:
COMMIT_CHANGED: ${{ steps.commit.outputs.changed }}
MANIFESTS_COUNT: ${{ steps.discover.outputs.manifests_count }}
GATE_SIGN: ${{ steps.gate.outputs.sign }}
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}."
if [ "${GATE_SIGN}" != "true" ]; then
echo "Signing skipped (eligibility gate: not approved, wrong base branch, or fork PR)."
else
echo "No changes detected (manifests already signed or no module changes on this PR vs merge-base)."
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
fi
} >> "$GITHUB_STEP_SUMMARY"
5 changes: 5 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ repos:
pass_filenames: false
always_run: true
verbose: true
# pass_filenames: false — same chunking issue as lint; script runs repo-wide yaml-lint once.
- id: modules-block1-yaml
name: "Block 1 — stage 2/4 — yaml-lint (when YAML staged)"
entry: ./scripts/pre-commit-quality-checks.sh block1-yaml
language: system
pass_filenames: false
files: \.(yaml|yml)$
verbose: true
- id: modules-block1-bundle
Expand All @@ -32,10 +34,13 @@ repos:
pass_filenames: false
always_run: true
verbose: true
# pass_filenames: false — otherwise pre-commit re-invokes this hook per filename chunk (ARG_MAX),
# and each run still executes full-repo `hatch run lint` (wasteful duplicate output).
- id: modules-block1-lint
name: "Block 1 — stage 4/4 — lint (when Python staged)"
entry: ./scripts/pre-commit-quality-checks.sh block1-lint
language: system
pass_filenames: false
files: \.(py|pyi)$
verbose: true
- id: modules-block2
Expand Down
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ and this project follows SemVer for bundle versions.

- Refresh the canonical `specfact-code-review` house-rules skill to a compact
clean-code charter and bump the bundle metadata for the signed 0.45.1 release.
- Document CI module verification: **`pr-orchestrator`** PR checks run
`verify-modules-signature` with **`--payload-from-filesystem --enforce-version-bump`**
and omit **`--require-signature` by default**; **`--require-signature`** is enforced
when the target is **`main`** (including pushes to **`main`**). **`sign-modules.py`**
in approval workflows continues to use **`--payload-from-filesystem`**. Sign bundled
manifests before merging release PRs or address post-merge verification failures by
re-signing and bumping versions as required.

## [0.44.0] - 2026-03-17

Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ hatch run test
hatch run specfact code review run --json --out .specfact/code-review.json
```

**Module signatures:** `pr-orchestrator` enforces `--require-signature` only for events targeting **`main`**; for **`dev`** (and feature branches) CI checks checksums and version bumps without requiring a cryptographic signature yet. Add `--require-signature` to the `verify-modules-signature` command when you want the same bar as **`main`** (for example before merging to `main`). Pre-commit runs `scripts/pre-commit-verify-modules-signature.sh`, which mirrors that policy (signatures required on branch `main`, or when `GITHUB_BASE_REF=main` in Actions).
**Module signatures:** For pull request verification, `pr-orchestrator` runs `verify-modules-signature` with **`--payload-from-filesystem --enforce-version-bump`** and does **not** pass **`--require-signature` by default** (checksum + version bump only). **Strict `--require-signature`** applies when the integration target is **`main`** (pushes to `main` and PRs whose base is `main`). Add `--require-signature` locally when you want the same bar as **`main`** before promotion. Approval-time **`sign-modules-on-approval`** signs with `scripts/sign-modules.py` using **`--payload-from-filesystem`** among other flags; if verification fails after merge, re-sign affected **`module-package.yaml`** files and bump versions as needed. Pre-commit runs `scripts/pre-commit-verify-modules-signature.sh`, mirroring CI (**`--require-signature`** on branch **`main`** or when **`GITHUB_BASE_REF=main`** in Actions).

**CI signing:** Approved PRs to `dev` or `main` from **this repository** (not forks) run `.github/workflows/sign-modules-on-approval.yml`, which can commit signed manifests using repository secrets. See [Module signing](./docs/authoring/module-signing.md).

Expand All @@ -59,7 +59,7 @@ pre-commit install
pre-commit run --all-files
```

**Code review gate (matches specfact-cli core):** runs in **Block 2** after the module verify hook and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/` are eligible; `openspec/changes/**/TDD_EVIDENCE.md` is excluded from the gate. OpenSpec Markdown other than evidence files is not passed to SpecFact (the review CLI treats paths as Python). The helper runs `specfact code review run --json --out .specfact/code-review.json` on the remaining paths and prints only a short findings summary and copy-paste prompts on stderr. Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`).
**Code review gate (matches specfact-cli core):** runs in **Block 2** after the module verify hook and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged paths under `packages/`, `registry/`, `scripts/`, `tools/`, `tests/`, and `openspec/changes/` are eligible; `openspec/changes/**/TDD_EVIDENCE.md` is excluded from the gate. Only staged **`.py` / `.pyi`** files are forwarded to SpecFact (YAML, registry tarballs, and similar are skipped). The hook blocks the commit when the JSON report contains **error**-severity findings; warning-only outcomes do not block. The helper runs `specfact code review run --json --out .specfact/code-review.json` on those Python paths and prints a short findings summary on stderr. Full CLI options (`--mode`, `--focus`, `--level`, `--bug-hunt`, etc.) are documented under [Code review module](./docs/modules/code-review.md). Block 1 is split into separate pre-commit hooks so output appears between stages instead of buffering until the end. Requires a local **specfact-cli** install (`hatch run dev-deps` resolves sibling `../specfact-cli` or `SPECFACT_CLI_REPO`).

Scope notes:
- Pre-commit runs `hatch run lint` in the **Block 1 — lint** hook when any staged path matches `*.py` / `*.pyi`, matching the CI quality job (Ruff alone does not run pylint).
Expand Down
53 changes: 43 additions & 10 deletions docs/modules/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,22 @@ Options:
findings such as test-scope contract noise
- `--interactive`: ask whether changed test files should be included before
auto-detected review runs
- `--bug-hunt`: use longer CrossHair budgets (`--per_path_timeout 10`, subprocess
timeout 120s) for deeper counterexample search; other tools keep normal speed
- `--mode shadow|enforce`: **enforce** (default) keeps today’s non-zero process
exit when the governed report says the run failed; **shadow** still runs the
full toolchain and preserves `overall_verdict` in JSON, but forces
`ci_exit_code` and the process exit code to `0` so CI or hooks can log signal
without blocking
- `--focus`: repeatable facet filter applied after scope resolution; values are
`source` (non-test, non-docs Python), `tests` (paths with a `tests/` segment),
and `docs` (Python under a `docs/` directory segment). Multiple `--focus`
values **union** their file sets, then intersect with the resolved scope. When
any `--focus` is set, **`--include-tests` and `--exclude-tests` are rejected**
(use focus alone to express test intent)
- `--level error|warning`: drop findings below the chosen severity **before**
scoring and report construction so JSON, tables, score, verdict, and
`ci_exit_code` all match the filtered list. Omit to keep all severities

When `FILES` is omitted, the command falls back to:

Expand Down Expand Up @@ -80,8 +96,10 @@ findings such as:

### Exit codes

- `0`: `PASS` or `PASS_WITH_ADVISORY`
- `1`: `FAIL`
- `0`: `PASS` or `PASS_WITH_ADVISORY`, or any outcome under **`--mode shadow`**
(shadow forces success at the process level even when `overall_verdict` is
`FAIL`)
- `1`: `FAIL` under default **enforce** semantics
- `2`: invalid CLI usage, such as a missing file path or incompatible options

### Output modes
Expand Down Expand Up @@ -249,6 +267,14 @@ Additional behavior:
- semgrep rule IDs emitted with path prefixes are normalized back to the governed rule IDs above
- malformed output, a missing `results` list, or a missing Semgrep executable yields a single `tool_error` finding

### Semgrep bug-rules pass

After the clean-code Semgrep pass, the orchestrator runs
`specfact_code_review.tools.semgrep_runner.run_semgrep_bugs(files)`, which uses
`packages/specfact-code-review/.semgrep/bugs.yaml` when present. Findings are
mapped to `security` or `correctness`. If the config file is missing, the pass
is skipped with no error.

### Contract runner

`specfact_code_review.tools.contract_runner.run_contract_check(files)` combines two
Expand All @@ -261,20 +287,26 @@ AST scan behavior:

- only public module-level and class-level functions are checked
- functions prefixed with `_` are treated as private and skipped
- the AST scan for `MISSING_ICONTRACT` runs **only when the file imports
`icontract`** (`from icontract …` or `import icontract`). Files that never
reference icontract skip the decorator scan and rely on CrossHair only
- missing icontract decorators become `contracts` findings with rule
`MISSING_ICONTRACT`
`MISSING_ICONTRACT` when the scan runs
- unreadable or invalid Python files degrade to a single `tool_error` finding instead
of raising

CrossHair behavior:

```bash
crosshair check --per_path_timeout 2 <files...>
crosshair check --per_path_timeout 2 <files...> # default
crosshair check --per_path_timeout 10 <files...> # with CLI --bug-hunt
```

- CrossHair counterexamples map to `contracts` warnings with tool `crosshair`
- timeouts are skipped so the AST scan can still complete
- missing CrossHair binaries degrade to a single `tool_error` finding
- with **`--bug-hunt`**, the per-path timeout is **10** seconds and the
subprocess budget is **120** seconds instead of **2** / **30**

Operational note:

Expand Down Expand Up @@ -374,12 +406,13 @@ bundle runners in this order:

1. Ruff
2. Radon
3. Semgrep
4. AST clean-code checks
5. basedpyright
6. pylint
7. contract runner
8. TDD gate, unless `no_tests=True`
3. Semgrep (clean-code ruleset)
4. Semgrep bug rules (`.semgrep/bugs.yaml`, skipped if absent)
5. AST clean-code checks
6. basedpyright
7. pylint
8. contract runner (AST + CrossHair; optional bug-hunt timeouts)
9. TDD gate, unless `no_tests=True`

When `SPECFACT_CODE_REVIEW_PR_MODE=1` is present, the runner also evaluates a
bundle-local advisory PR checklist from `SPECFACT_CODE_REVIEW_PR_TITLE`,
Expand Down
9 changes: 5 additions & 4 deletions docs/reference/module-security.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ Module packages carry **publisher** and **integrity** metadata so installation,
- **CI secrets**:
- `SPECFACT_MODULE_PRIVATE_SIGN_KEY`
- `SPECFACT_MODULE_PRIVATE_SIGN_KEY_PASSPHRASE`
- **Verification command**:
- Default strict local / **main** check: `scripts/verify-modules-signature.py --require-signature --payload-from-filesystem --enforce-version-bump`
- **Dev / feature parity with CI** (checksum + version bump, signature optional): omit `--require-signature` (see `pr-orchestrator` and `scripts/pre-commit-verify-modules-signature.sh`).
- `--version-check-base <git-ref>` is used for PR comparisons in CI.
- **Verification command** (`scripts/verify-modules-signature.py`):
- **Baseline (PR/CI and local hook)**: `--payload-from-filesystem --enforce-version-bump` — full payload checksum verification plus version-bump enforcement. This is the default integration path **without** `--require-signature` when the target branch is **`dev`** (pull requests to `dev`, or pushes to `dev`).
- **Strict mode**: add `--require-signature` so every manifest must include a verifiable `integrity.signature`. In `.github/workflows/pr-orchestrator.yml` this is appended for **pull requests whose base is `main`** and for **pushes to `main`**, in addition to the baseline flags. Locally, `scripts/pre-commit-verify-modules-signature.sh` adds `--require-signature` only when the checkout (or `GITHUB_BASE_REF` in Actions) is **`main`**; otherwise it runs the same baseline flags only.
- **Pull request CI** also passes `--version-check-base <git-ref>` (typically `origin/<base branch>`) so version rules compare against the PR base.
- **CI uses the full verifier** (payload digest + rules above). It does **not** pass `--metadata-only`. The script still supports `--metadata-only` for optional tooling that only needs manifest shape and checksum format checks.
- **CI signing**: Approved same-repo PRs to `dev` or `main` may receive automated signing commits via `sign-modules-on-approval.yml` (repository secrets; merge-base scoped `--changed-only`).

## Public key and key rotation
Expand Down
Loading
Loading