From 85477fd8a22fa3454c4960429700e16e7b47e13a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 13 Apr 2026 10:11:18 +0000 Subject: [PATCH 1/3] fix: address Codex/CodeRabbit review on governance and cache tooling - Add beartype to default Hatch env so standalone scripts import cleanly. - Hierarchy cache: paginate subIssues; fingerprint rendered fields (not updated_at). - Pre-commit Block 2: narrow safe-change skip; review gate uses staged contract paths. - apply_specfact_workspace_env always pins SPECFACT_* to current checkout. - validate_agent_rule_applies_when: robust reads; strict applies_when list types. - contract_first_smart_test: treat contracts/ as contract-relevant. - Docs/openspec: quality gate order, cache freshness, CHANGE_ORDER, backlog-sync paths. Co-authored-by: Dom --- README.md | 4 +- .../50-quality-gates-and-review.md | 11 +- .../60-github-change-governance.md | 2 +- .../agent-rules/70-release-commit-and-docs.md | 2 +- openspec/CHANGE_ORDER.md | 4 +- .../specs/backlog-sync/spec.md | 8 ++ .../CHANGE_VALIDATION.md | 2 +- .../TDD_EVIDENCE.md | 4 + .../proposal.md | 2 + .../specs/agent-governance-loading/spec.md | 2 + .../specs/backlog-add/spec.md | 4 + .../specs/backlog-sync/spec.md | 4 + .../tasks.md | 7 +- openspec/specs/backlog-sync/spec.md | 16 ++- openspec/specs/github-hierarchy-cache/spec.md | 6 +- pyproject.toml | 1 + scripts/pre-commit-quality-checks.sh | 34 +++-- scripts/pre_commit_code_review.py | 32 +++-- scripts/sync_github_hierarchy_cache.py | 129 +++++++++++++++++- scripts/validate_agent_rule_applies_when.py | 13 +- src/specfact_cli_modules/dev_bootstrap.py | 4 +- .../scripts/test_pre_commit_code_review.py | 70 +++++----- .../test_sync_github_hierarchy_cache.py | 45 +++++- .../test_validate_agent_rule_applies_when.py | 16 +++ tests/unit/test_dev_bootstrap.py | 15 +- .../tools/test_contract_first_smart_test.py | 12 ++ tools/contract_first_smart_test.py | 1 + 27 files changed, 354 insertions(+), 96 deletions(-) diff --git a/README.md b/README.md index b2aaed30..42bcd6b5 100644 --- a/README.md +++ b/README.md @@ -40,10 +40,12 @@ hatch run format hatch run type-check hatch run lint hatch run yaml-lint +hatch run check-bundle-imports hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump hatch run contract-test hatch run smart-test hatch run test +hatch run specfact code review run --json --out .specfact/code-review.json ``` To mirror CI locally with git hooks, enable pre-commit: @@ -53,7 +55,7 @@ pre-commit install pre-commit run --all-files ``` -**Code review gate (matches specfact-cli core):** runs in **Block 2** after module signatures and Block 1 quality hooks (`pre-commit-quality-checks.sh block2`, which calls `scripts/pre_commit_code_review.py`). Staged `*.py` / `*.pyi` files run `specfact code review run --json --out .specfact/code-review.json`. The helper prints only a short findings summary and copy-paste prompts on stderr (not the nested CLI’s full tool output). 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 module signatures 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/` (excluding `TDD_EVIDENCE.md`) are forwarded to the helper, which runs `specfact code review run --json --out .specfact/code-review.json` with that path list. The helper prints only a short findings summary and copy-paste prompts on stderr (not the nested CLI’s full tool output). 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). diff --git a/docs/agent-rules/50-quality-gates-and-review.md b/docs/agent-rules/50-quality-gates-and-review.md index 3d2cf535..84c16735 100644 --- a/docs/agent-rules/50-quality-gates-and-review.md +++ b/docs/agent-rules/50-quality-gates-and-review.md @@ -42,16 +42,17 @@ depends_on: 2. `hatch run type-check` 3. `hatch run lint` 4. `hatch run yaml-lint` -5. `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump` -6. `hatch run contract-test` -7. `hatch run smart-test` -8. `hatch run test` +5. `hatch run check-bundle-imports` +6. `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump` +7. `hatch run contract-test` +8. `hatch run smart-test` +9. `hatch run test` ## Pre-commit order 1. Module signature verification (`.pre-commit-config.yaml`, `fail_fast: true` so a failing earlier hook never runs later stages). 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 `*.py` and `*.pyi`** (same glob as `staged_python_files()` in the script—type stubs count), then `contract-test-status` / `hatch run contract-test`. +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`. Run the full pipeline manually with `./scripts/pre-commit-quality-checks.sh` or `… all`. diff --git a/docs/agent-rules/60-github-change-governance.md b/docs/agent-rules/60-github-change-governance.md index 869de6cd..2df65588 100644 --- a/docs/agent-rules/60-github-change-governance.md +++ b/docs/agent-rules/60-github-change-governance.md @@ -59,6 +59,6 @@ Before implementation on a publicly tracked change issue: If the linked GitHub issue appears to be `in progress`, do not treat that as blocking until you have a current view of GitHub state: -1. If `.specfact/backlog/github_hierarchy_cache.md` is missing, or was last updated more than about five minutes ago, run `python scripts/sync_github_hierarchy_cache.py`. +1. If `.specfact/backlog/github_hierarchy_cache.md` is missing, or its hierarchy metadata is older than **300 seconds** compared to current UTC time, run `python scripts/sync_github_hierarchy_cache.py`. Treat the cache as fresh when `.specfact/backlog/github_hierarchy_cache_state.json` exists and its `generated_at` ISO-8601 timestamp is within the last 300 seconds; otherwise compare the markdown file’s last modification time (mtime) in UTC against “now” and refresh when the age exceeds 300 seconds. 2. Re-read the issue state from GitHub or the refreshed cache-backed workflow and confirm the issue is still `in progress`. 3. Only after that verification, if it remains `in progress`, pause implementation and ask the user to clarify whether the change is already being worked in another session. diff --git a/docs/agent-rules/70-release-commit-and-docs.md b/docs/agent-rules/70-release-commit-and-docs.md index 145d2fbf..939913a1 100644 --- a/docs/agent-rules/70-release-commit-and-docs.md +++ b/docs/agent-rules/70-release-commit-and-docs.md @@ -42,7 +42,7 @@ depends_on: ## Registry and publish flow -1. Branch from `origin/dev` into a feature or hotfix branch. +1. Branch from `origin/dev` into a worktree branch whose name uses one of: `feature/*`, `bugfix/*`, `hotfix/*`, or `chore/*` (expected sibling worktrees live under `../specfact-cli-modules-worktrees/` per session-bootstrap rules). 2. Bump the bundle version in `packages//module-package.yaml`. 3. Run `python scripts/publish_module.py --bundle ` as the publish pre-check. 4. Publish with the project tooling wrapper when release work is actually intended. diff --git a/openspec/CHANGE_ORDER.md b/openspec/CHANGE_ORDER.md index 61c22353..5220103a 100644 --- a/openspec/CHANGE_ORDER.md +++ b/openspec/CHANGE_ORDER.md @@ -32,6 +32,7 @@ | ✅ speckit-03-change-proposal-bridge | archived 2026-04-05 | | ✅ packaging-01-bundle-resource-payloads | archived 2026-04-05 | | ✅ module-bundle-deps-auto-install | archived 2026-04-05 | +| ✅ governance-03-github-hierarchy-cache | archived 2026-04-09 | ## Pending @@ -75,8 +76,7 @@ These changes are the modules-side runtime companions to split core governance a |--------|-------|---------------|----------|------------| | governance | 01 | governance-01-evidence-output | [#169](https://github.com/nold-ai/specfact-cli-modules/issues/169) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); core counterpart `specfact-cli#247`; validation runtime `#171` | | governance | 02 | governance-02-exception-management | [#167](https://github.com/nold-ai/specfact-cli-modules/issues/167) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); core counterpart `specfact-cli#248`; policy runtime `#158` | -| governance | 03 | governance-03-github-hierarchy-cache | [#178](https://github.com/nold-ai/specfact-cli-modules/issues/178) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); paired core `governance-02-github-hierarchy-cache` [specfact-cli#491](https://github.com/nold-ai/specfact-cli/issues/491) | -| governance | 04 | governance-04-deterministic-agent-governance-loading | [#181](https://github.com/nold-ai/specfact-cli-modules/issues/181) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); paired core [specfact-cli#494](https://github.com/nold-ai/specfact-cli/issues/494); baseline [#178](https://github.com/nold-ai/specfact-cli-modules/issues/178) | +| governance | 04 | governance-04-deterministic-agent-governance-loading | [#181](https://github.com/nold-ai/specfact-cli-modules/issues/181) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); paired core [specfact-cli#494](https://github.com/nold-ai/specfact-cli/issues/494); baseline [#178](https://github.com/nold-ai/specfact-cli-modules/issues/178) (implements archived `governance-03-github-hierarchy-cache`, paired core [specfact-cli#491](https://github.com/nold-ai/specfact-cli/issues/491)) | | validation | 02 | validation-02-full-chain-engine | [#171](https://github.com/nold-ai/specfact-cli-modules/issues/171) | Parent Feature: [#163](https://github.com/nold-ai/specfact-cli-modules/issues/163); core counterpart `specfact-cli#241`; runtime inputs from `#164` and `#165`; policy semantics from `#158` | ### Documentation restructure diff --git a/openspec/changes/archive/2026-04-09-governance-03-github-hierarchy-cache/specs/backlog-sync/spec.md b/openspec/changes/archive/2026-04-09-governance-03-github-hierarchy-cache/specs/backlog-sync/spec.md index cd330199..179d2c51 100644 --- a/openspec/changes/archive/2026-04-09-governance-03-github-hierarchy-cache/specs/backlog-sync/spec.md +++ b/openspec/changes/archive/2026-04-09-governance-03-github-hierarchy-cache/specs/backlog-sync/spec.md @@ -1,6 +1,9 @@ +# Archived backlog-sync delta (governance-03) + ## MODIFIED Requirements ### Requirement: Restore backlog sync command functionality + The system SHALL provide `specfact backlog sync` command for bidirectional backlog synchronization, and related governance workflows SHALL be able to resolve current Epic and Feature planning metadata from the repo-local hierarchy cache before performing manual GitHub lookups. #### Scenario: Sync from OpenSpec to backlog @@ -10,25 +13,30 @@ The system SHALL provide `specfact backlog sync` command for bidirectional backl - **AND** state mapping preserves status semantics #### Scenario: Bidirectional sync with cross-adapter + - **WHEN** the user runs sync with cross-adapter configuration - **THEN** state is mapped between adapters using canonical status - **AND** lossless round-trip preserves content #### Scenario: Sync with bundle integration + - **WHEN** sync is run within an OpenSpec bundle context - **THEN** synced items update bundle state and source tracking #### Scenario: Ceremony alias works + - **WHEN** the user runs `specfact backlog ceremony sync` - **THEN** the command forwards to `specfact backlog sync` #### Scenario: Cache-first hierarchy lookup for parent issue assignment + - **GIVEN** a contributor needs a parent Feature or Epic while preparing GitHub sync metadata - **WHEN** the local hierarchy cache is present and current - **THEN** the contributor can resolve the parent relationship from the cache without an additional GitHub lookup - **AND** the sync script is rerun only when the cache is stale or missing ### Requirement: Backlog sync checks for existing external issue mappings before creation + The backlog sync system SHALL check for existing issue mappings from external tools (including spec-kit extensions) before creating new backlog issues, to prevent duplicates. #### Scenario: Backlog sync with spec-kit extension mappings available diff --git a/openspec/changes/governance-04-deterministic-agent-governance-loading/CHANGE_VALIDATION.md b/openspec/changes/governance-04-deterministic-agent-governance-loading/CHANGE_VALIDATION.md index 9f1782cb..a0259e9e 100644 --- a/openspec/changes/governance-04-deterministic-agent-governance-loading/CHANGE_VALIDATION.md +++ b/openspec/changes/governance-04-deterministic-agent-governance-loading/CHANGE_VALIDATION.md @@ -2,7 +2,7 @@ - **Validated on (local):** 2026-04-12 - **Strict command:** `openspec validate governance-04-deterministic-agent-governance-loading --strict` -- **Result:** PASS +- **Result:** **PENDING / BLOCKED** — `openspec validate … --strict` passed, but the mandatory full-repo SpecFact code review gate is still **FAIL** (`.specfact/code-review.json` reports **934** findings). Do not mark this change fully validated until `hatch run specfact code review run --json --out .specfact/code-review.json --scope full` exits **PASS** or an explicit, approved exception is recorded. ## Scope summary diff --git a/openspec/changes/governance-04-deterministic-agent-governance-loading/TDD_EVIDENCE.md b/openspec/changes/governance-04-deterministic-agent-governance-loading/TDD_EVIDENCE.md index da6ff978..f5557020 100644 --- a/openspec/changes/governance-04-deterministic-agent-governance-loading/TDD_EVIDENCE.md +++ b/openspec/changes/governance-04-deterministic-agent-governance-loading/TDD_EVIDENCE.md @@ -52,3 +52,7 @@ - `hatch run specfact module install nold-ai/specfact-codebase nold-ai/specfact-code-review --scope project --source bundled --repo . --reinstall` → bundled module artifacts not found for those ids - Local worktree note: - After relocating the worktree directory, the ignored `.venv/bin/semgrep` and `.venv/bin/pysemgrep` entrypoints had stale absolute shebangs. Those local launchers were repaired in-place so changed-scope code review could run successfully from the corrected worktree path. + +## Change validation / release note + +- **Full-repo review gate:** Until `hatch run specfact code review run --json --out .specfact/code-review.json --scope full` passes (or an approved exception is documented for task **4.2** with the report artifact attached), promotion trains that require a green full review remain blocked. Triage should start with high-severity items (for example legacy complexity in `packages/specfact-backlog/src/specfact_backlog/backlog/commands.py`). diff --git a/openspec/changes/governance-04-deterministic-agent-governance-loading/proposal.md b/openspec/changes/governance-04-deterministic-agent-governance-loading/proposal.md index 4be0f217..a8cb6c5d 100644 --- a/openspec/changes/governance-04-deterministic-agent-governance-loading/proposal.md +++ b/openspec/changes/governance-04-deterministic-agent-governance-loading/proposal.md @@ -1,3 +1,5 @@ +# Proposal: Deterministic agent governance loading + ## Why `AGENTS.md` in **specfact-cli-modules** mixes bootstrap policy, quality gates, and long-form workflow detail in one surface. That raises token cost for every session and makes cross-model behavior less deterministic around worktrees, OpenSpec, cache-first GitHub hierarchy, TDD, and PR completion gates. This change aligns the modules repo with the **deterministic agent-governance** model already shipped in **specfact-cli** ([specfact-cli#494](https://github.com/nold-ai/specfact-cli/issues/494)) so both repositories share the same loading semantics and stop conditions. diff --git a/openspec/changes/governance-04-deterministic-agent-governance-loading/specs/agent-governance-loading/spec.md b/openspec/changes/governance-04-deterministic-agent-governance-loading/specs/agent-governance-loading/spec.md index c9786ac1..1a38e10e 100644 --- a/openspec/changes/governance-04-deterministic-agent-governance-loading/specs/agent-governance-loading/spec.md +++ b/openspec/changes/governance-04-deterministic-agent-governance-loading/specs/agent-governance-loading/spec.md @@ -1,3 +1,5 @@ +# Specification: Agent governance loading + ## ADDED Requirements ### Requirement: Compact AGENTS bootstrap contract diff --git a/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-add/spec.md b/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-add/spec.md index 2ffc8551..37b52a3e 100644 --- a/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-add/spec.md +++ b/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-add/spec.md @@ -1,9 +1,13 @@ +# Backlog add requirements + ## ADDED Requirements ### Requirement: Backlog add local artifact helpers SHALL preserve user-managed content + Any `specfact backlog add` helper flow that writes local project artifacts SHALL use the runtime safe-write contract and preserve unrelated user-managed content. #### Scenario: Existing local config is not silently replaced + - **WHEN** a backlog-add related local helper targets an existing user-project artifact - **THEN** the helper SHALL skip, merge, or require explicit replacement according to declared ownership - **AND** SHALL NOT silently overwrite the existing file diff --git a/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-sync/spec.md b/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-sync/spec.md index 291a12ff..7d257b20 100644 --- a/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-sync/spec.md +++ b/openspec/changes/project-runtime-01-safe-artifact-write-policy/specs/backlog-sync/spec.md @@ -1,9 +1,13 @@ +# Backlog sync requirements + ## ADDED Requirements ### Requirement: Backlog sync local export paths SHALL avoid silent overwrite + Any `specfact backlog sync` local export or artifact materialization path SHALL avoid silent overwrites of existing user-project artifacts. #### Scenario: Existing export target produces conflict or safe merge + - **WHEN** backlog sync would write to a local artifact path that already exists - **THEN** the command SHALL use the runtime safe-write contract to merge, skip, or require explicit replacement - **AND** SHALL surface the chosen behavior in command output diff --git a/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md b/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md index 6b94603c..6923a5fb 100644 --- a/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md +++ b/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md @@ -20,7 +20,6 @@ - [ ] 4.1 Re-run the targeted runtime tests and any broader affected package coverage, then record passing evidence in `TDD_EVIDENCE.md`. - [ ] 4.2 Update affected modules docs to explain preservation guarantees and explicit replacement semantics for adopted commands. -- [ ] 4.3 Run quality gates: `hatch run format`, `hatch run type-check`, `hatch run lint`, `hatch run yaml-lint`, `hatch run contract-test`, and the relevant `smart-test`/`test` coverage for changed packages. -- [ ] 4.4 Run module signature verification, bump package versions where required, re-sign changed manifests if needed, and verify registry consistency. -- [ ] 4.5 Ensure `.specfact/code-review.json` is fresh, remediate all findings, and record the final review command/timestamp in `TDD_EVIDENCE.md` or PR notes. -- [ ] 4.6 Open the modules PR to `dev`, cross-link the paired core PR, and note any deferred runtime adoption paths as follow-up issues if the initial scope is intentionally limited. +- [ ] 4.3 Run quality gates in order: `hatch run format`, `hatch run type-check`, `hatch run lint`, `hatch run yaml-lint`, `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump`, then bump package versions and re-sign changed manifests if verification failed for that reason, then `hatch run contract-test`, then the relevant `hatch run smart-test` / `hatch run test` coverage for changed packages. +- [ ] 4.4 Ensure `.specfact/code-review.json` is fresh, remediate all findings, and record the final review command/timestamp in `TDD_EVIDENCE.md` or PR notes (last governance action before merge). +- [ ] 4.5 Open the modules PR to `dev`, cross-link the paired core PR, and note any deferred runtime adoption paths as follow-up issues if the initial scope is intentionally limited. diff --git a/openspec/specs/backlog-sync/spec.md b/openspec/specs/backlog-sync/spec.md index eb18c84d..920e0b8e 100644 --- a/openspec/specs/backlog-sync/spec.md +++ b/openspec/specs/backlog-sync/spec.md @@ -1,10 +1,14 @@ # backlog-sync Specification ## Purpose + TBD - created by archiving change backlog-02-migrate-core-commands. Update Purpose after archive. + ## Requirements + ### Requirement: Restore backlog sync command functionality -The system SHALL provide `specfact backlog sync` command for bidirectional backlog synchronization, and related governance workflows SHALL be able to resolve current Epic and Feature planning metadata from the repo-local hierarchy cache before performing manual GitHub lookups. + +The system SHALL provide `specfact backlog sync` command for bidirectional backlog synchronization, and related governance workflows SHALL resolve current Epic and Feature planning metadata from **`.specfact/backlog/github_hierarchy_cache.md`**, with deterministic sync state in **`.specfact/backlog/github_hierarchy_cache_state.json`**, before performing manual GitHub lookups. Tools that participate in backlog or OpenSpec workflows MUST read and write those exact paths (or invoke `python scripts/sync_github_hierarchy_cache.py`, which uses them by default) and MUST fall back to live GitHub lookup only when the files are missing, unreadable, or stale per governance rules. #### Scenario: Sync from OpenSpec to backlog @@ -13,25 +17,30 @@ The system SHALL provide `specfact backlog sync` command for bidirectional backl - **AND** state mapping preserves status semantics #### Scenario: Bidirectional sync with cross-adapter + - **WHEN** the user runs sync with cross-adapter configuration - **THEN** state is mapped between adapters using canonical status - **AND** lossless round-trip preserves content #### Scenario: Sync with bundle integration + - **WHEN** sync is run within an OpenSpec bundle context - **THEN** synced items update bundle state and source tracking #### Scenario: Ceremony alias works + - **WHEN** the user runs `specfact backlog ceremony sync` - **THEN** the command forwards to `specfact backlog sync` #### Scenario: Cache-first hierarchy lookup for parent issue assignment + - **GIVEN** a contributor needs a parent Feature or Epic while preparing GitHub sync metadata -- **WHEN** the local hierarchy cache is present and current +- **WHEN** `.specfact/backlog/github_hierarchy_cache.md` is present and current (per `.specfact/backlog/github_hierarchy_cache_state.json` / refresh rules) - **THEN** the contributor can resolve the parent relationship from the cache without an additional GitHub lookup -- **AND** the sync script is rerun only when the cache is stale or missing +- **AND** `specfact backlog sync` (and the alias `specfact backlog ceremony sync`) rely on that cache-first contract; the sync script is rerun only when the cache is stale or missing ### Requirement: Backlog sync checks for existing external issue mappings before creation + The backlog sync system SHALL check for existing issue mappings from external tools (including spec-kit extensions) before creating new backlog issues, to prevent duplicates. #### Scenario: Backlog sync with spec-kit extension mappings available @@ -50,4 +59,3 @@ The backlog sync system SHALL check for existing issue mappings from external to - **WHEN** `specfact backlog sync` runs - **THEN** the sync creates issues for all tasks as before (no behavior change) - **AND** no spec-kit extension detection is attempted - diff --git a/openspec/specs/github-hierarchy-cache/spec.md b/openspec/specs/github-hierarchy-cache/spec.md index 8822e5f6..2a13536e 100644 --- a/openspec/specs/github-hierarchy-cache/spec.md +++ b/openspec/specs/github-hierarchy-cache/spec.md @@ -1,8 +1,11 @@ # github-hierarchy-cache Specification ## Purpose -TBD - created by archiving change governance-03-github-hierarchy-cache. Update Purpose after archive. + +This capability standardizes a **repo-local, deterministic GitHub Epic/Feature hierarchy cache** for SpecFact modules and paired `specfact-cli` workflows. It defines how contributors sync hierarchy metadata from GitHub into ignored `.specfact/backlog/` files, how fingerprints detect unchanged trees, and how governance (for example `AGENTS.md` and `docs/agent-rules/`) MUST consult that cache before manual GitHub parent lookups. It builds on the archived OpenSpec change **`governance-03-github-hierarchy-cache`**; shipped behavior and drift against `openspec/**/*.md` remain governed by `openspec/CHANGE_ORDER.md` and the modules release checklist. + ## Requirements + ### Requirement: Repository hierarchy cache sync The repository SHALL provide a deterministic sync mechanism that retrieves GitHub Epic and Feature issues for the current repository and writes a local hierarchy cache under ignored `.specfact/backlog/`. @@ -30,4 +33,3 @@ Repository governance instructions SHALL direct contributors and agents to consu - **THEN** the instructions tell them to consult the local hierarchy cache first - **AND** the instructions define when the sync script must be rerun to refresh stale hierarchy metadata - **AND** the instructions state that the cache is local ephemeral state and must not be committed - diff --git a/pyproject.toml b/pyproject.toml index 4f07f633..259a54bd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -16,6 +16,7 @@ dev = [] type = "virtual" path = ".venv" dependencies = [ + "beartype>=0.22.0", "json5>=0.9.28", "icontract>=2.7.1", "pytest>=8.4.2", diff --git a/scripts/pre-commit-quality-checks.sh b/scripts/pre-commit-quality-checks.sh index 8693ed85..e85820c1 100755 --- a/scripts/pre-commit-quality-checks.sh +++ b/scripts/pre-commit-quality-checks.sh @@ -37,7 +37,7 @@ print_block2_overview() { echo "" >&2 echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" >&2 echo " modules pre-commit — Block 2: code review + contract tests (2 stages)" >&2 - echo " 1/2 code review gate (hatch run python scripts/pre_commit_code_review.py)" >&2 + echo " 1/2 code review gate (staged paths under packages/, registry/, scripts/, tools/, tests/, openspec/changes/)" >&2 echo " 2/2 contract-first tests (contract-test-status → hatch run contract-test)" >&2 echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" >&2 echo "" >&2 @@ -59,6 +59,20 @@ staged_python_files() { staged_files | grep -E '\.pyi?$' || true } +# Paths passed to scripts/pre_commit_code_review.py (packages/, registry/, scripts/, tools/, tests/, openspec/changes/). +staged_review_gate_files() { + local line + while IFS= read -r line; do + [ -z "${line}" ] && continue + case "${line}" in + */TDD_EVIDENCE.md|TDD_EVIDENCE.md) continue ;; + packages/*|registry/*|scripts/*|tools/*|tests/*|openspec/changes/*) + printf '%s\n' "${line}" + ;; + esac + done < <(staged_files) +} + check_safe_change() { local files files=$(staged_files) @@ -77,7 +91,7 @@ check_safe_change() { ;; pyproject.toml|README.md|CHANGELOG.md|.pre-commit-config.yaml) ;; - scripts/pre-commit-quality-checks.sh) + scripts/pre-commit-quality-checks.sh|scripts/pre_commit_code_review.py|tools/contract_first_smart_test.py) ;; *) other_changes=$((other_changes + 1)) @@ -147,23 +161,23 @@ run_lint_if_staged_python() { } run_code_review_gate() { - local py_array=() + local review_array=() while IFS= read -r line; do [ -z "${line}" ] && continue - py_array+=("${line}") - done < <(staged_python_files) + review_array+=("${line}") + done < <(staged_review_gate_files) - if [ ${#py_array[@]} -eq 0 ]; then - info "📦 Block 2 — stage 1/2: code review — skipped (no staged *.py / *.pyi)" + if [ ${#review_array[@]} -eq 0 ]; then + info "📦 Block 2 — stage 1/2: code review — skipped (no staged paths under packages/, registry/, scripts/, tools/, tests/, or openspec/changes/)" return fi - info "📦 Block 2 — stage 1/2: code review — running \`hatch run python scripts/pre_commit_code_review.py\` (${#py_array[@]} file(s))" - if hatch run python scripts/pre_commit_code_review.py "${py_array[@]}"; then + info "📦 Block 2 — stage 1/2: code review — running \`hatch run python scripts/pre_commit_code_review.py\` (${#review_array[@]} path(s))" + if hatch run python scripts/pre_commit_code_review.py "${review_array[@]}"; then success "✅ Block 2 — stage 1/2: code review gate passed" else error "❌ Block 2 — stage 1/2: code review gate failed" - warn "💡 Fix blocking review findings or run: hatch run python scripts/pre_commit_code_review.py " + warn "💡 Fix blocking review findings or run: hatch run python scripts/pre_commit_code_review.py " exit 1 fi } diff --git a/scripts/pre_commit_code_review.py b/scripts/pre_commit_code_review.py index 506aaf78..63480274 100755 --- a/scripts/pre_commit_code_review.py +++ b/scripts/pre_commit_code_review.py @@ -44,21 +44,34 @@ def _load_dev_bootstrap() -> Any: apply_specfact_workspace_env = _dev_bootstrap.apply_specfact_workspace_env -PYTHON_SUFFIXES = {".py", ".pyi"} - # Default matches dogfood / OpenSpec: machine-readable report under ignored ``.specfact/``. REVIEW_JSON_OUT = ".specfact/code-review.json" +def _is_review_gate_path(path: str) -> bool: + """Return whether a repo-relative path should participate in the pre-commit review gate.""" + normalized = path.replace("\\", "/").strip() + if not normalized or normalized.endswith(("/TDD_EVIDENCE.md", "TDD_EVIDENCE.md")): + return False + prefixes = ( + "packages/", + "registry/", + "scripts/", + "tools/", + "tests/", + "openspec/changes/", + ) + return any(normalized.startswith(prefix) for prefix in prefixes) + + @require(lambda paths: paths is not None) @ensure(lambda result: len(result) == len(set(result))) -@ensure(lambda result: all(Path(path).suffix.lower() in PYTHON_SUFFIXES for path in result)) -def filter_review_files(paths: Sequence[str]) -> list[str]: - """Return only staged Python source files relevant to code review.""" +def filter_review_gate_paths(paths: Sequence[str]) -> list[str]: + """Return staged paths under contract- and tooling-heavy trees for the review gate.""" seen: set[str] = set() filtered: list[str] = [] for path in paths: - if Path(path).suffix.lower() not in PYTHON_SUFFIXES: + if not _is_review_gate_path(path): continue if path in seen: continue @@ -250,9 +263,12 @@ def ensure_runtime_available() -> tuple[bool, str | None]: def main(argv: Sequence[str] | None = None) -> int: """Run the code review gate; write JSON under ``.specfact/`` and return CLI exit code.""" apply_specfact_workspace_env(REPO_ROOT) - files = filter_review_files(list(argv or [])) + files = filter_review_gate_paths(list(argv or [])) if len(files) == 0: - sys.stdout.write("No staged Python files to review; skipping code review gate.\n") + sys.stdout.write( + "No staged review-relevant files under packages/, registry/, scripts/, tools/, tests/, " + "or openspec/changes/; skipping code review gate.\n" + ) return 0 available, guidance = ensure_runtime_available() diff --git a/scripts/sync_github_hierarchy_cache.py b/scripts/sync_github_hierarchy_cache.py index 740b5aa2..73747e05 100755 --- a/scripts/sync_github_hierarchy_cache.py +++ b/scripts/sync_github_hierarchy_cache.py @@ -78,6 +78,19 @@ def _default_repo_name_from_git(script_dir: Path) -> str | None: SUPPORTED_ISSUE_TYPES_ORDER: tuple[str, ...] = ("Epic", "Feature") _SUMMARY_SKIP_LINES = {"why", "scope", "summary", "changes", "capabilities", "impact"} _GH_GRAPHQL_TIMEOUT_SEC = 120 +_SUBISSUES_PAGE_SIZE = 100 + +_SUBISSUES_PAGE_QUERY = f""" +query($owner: String!, $name: String!, $issueNumber: Int!, $after: String) {{ + repository(owner: $owner, name: $name) {{ + issue(number: $issueNumber) {{ + subIssues(first: {_SUBISSUES_PAGE_SIZE}, after: $after) {{ + pageInfo {{ hasNextPage endCursor }} + nodes {{ number title url issueType {{ name }} }} }} + }} + }} +}} +""".strip() @beartype @@ -97,7 +110,6 @@ def _build_hierarchy_issues_query(*, include_body: bool) -> str: {body_field} issueType {{ name }} labels(first: 100) {{ nodes {{ name }} }} parent {{ number title url }} - subIssues(first: 100) {{ nodes {{ number title url issueType {{ name }} }} }} }} }} }} @@ -221,7 +233,12 @@ def _child_links(subissue_nodes: list[Mapping[str, Any]]) -> list[IssueLink]: @beartype -def _parse_issue_node(node: Mapping[str, Any], *, include_body: bool) -> HierarchyIssue | None: +def _parse_issue_node( + node: Mapping[str, Any], + *, + include_body: bool, + children: list[IssueLink], +) -> HierarchyIssue | None: """Convert a GraphQL issue node to HierarchyIssue when supported.""" issue_type_node = _mapping_value(node, "issueType") issue_type_name = str(issue_type_node["name"]) if issue_type_node and issue_type_node.get("name") else None @@ -238,10 +255,96 @@ def _parse_issue_node(node: Mapping[str, Any], *, include_body: bool) -> Hierarc summary=summary, updated_at=str(node["updatedAt"]), parent=_parse_issue_link(_mapping_value(node, "parent")), - children=_child_links(_mapping_nodes(_mapping_value(node, "subIssues"))), + children=children, ) +@beartype +def _fetch_all_subissue_nodes( + *, + repo_owner: str, + repo_name: str, + issue_number: int, + issue_url: str, +) -> list[Mapping[str, Any]]: + """Fetch every sub-issue node for one parent issue (paginated).""" + collected: list[Mapping[str, Any]] = [] + after: str | None = None + while True: + command = [ + "gh", + "api", + "graphql", + "-f", + f"query={_SUBISSUES_PAGE_QUERY}", + "-F", + f"owner={repo_owner}", + "-F", + f"name={repo_name}", + "-F", + f"issueNumber={issue_number}", + ] + if after is not None: + command.extend(["-F", f"after={after}"]) + + try: + completed = subprocess.run( + command, + check=False, + capture_output=True, + text=True, + timeout=_GH_GRAPHQL_TIMEOUT_SEC, + ) + except subprocess.TimeoutExpired as exc: + detail = ( + f"GitHub GraphQL subIssues pagination timed out after {_GH_GRAPHQL_TIMEOUT_SEC}s " + f"for issue #{issue_number} ({issue_url})" + ) + out = (exc.stdout or "").strip() + err = (exc.stderr or "").strip() + if out or err: + detail = f"{detail}; stdout={out!r}; stderr={err!r}" + raise RuntimeError(detail) from exc + + if completed.returncode != 0: + msg = completed.stderr.strip() or completed.stdout.strip() or "GitHub GraphQL query failed" + raise RuntimeError(f"subIssues fetch failed for issue #{issue_number} ({issue_url}): {msg}") + + payload = json.loads(completed.stdout) + if "errors" in payload: + raise RuntimeError( + f"subIssues GraphQL errors for issue #{issue_number} ({issue_url}): " + f"{json.dumps(payload['errors'], indent=2)}" + ) + + repository = payload.get("data", {}).get("repository") + if not isinstance(repository, Mapping): + raise RuntimeError(f"subIssues: missing repository for issue #{issue_number} ({issue_url})") + + issue_payload = repository.get("issue") + if not isinstance(issue_payload, Mapping): + raise RuntimeError(f"subIssues: missing issue #{issue_number} ({issue_url})") + + sub_connection = issue_payload.get("subIssues") + if not isinstance(sub_connection, Mapping): + raise RuntimeError(f"subIssues: missing connection for issue #{issue_number} ({issue_url})") + + page_nodes = _mapping_nodes(sub_connection) + collected.extend(page_nodes) + + page_info = sub_connection.get("pageInfo") + if not isinstance(page_info, Mapping): + raise RuntimeError(f"subIssues: missing pageInfo for issue #{issue_number} ({issue_url})") + + if not page_info.get("hasNextPage"): + break + after = page_info.get("endCursor") + if not isinstance(after, str) or not after: + raise RuntimeError(f"subIssues: hasNextPage without endCursor for issue #{issue_number} ({issue_url})") + + return collected + + @beartype def _run_graphql_query(query: str, *, repo_owner: str, repo_name: str, after: str | None) -> Mapping[str, Any]: """Run a GitHub GraphQL query through `gh`.""" @@ -320,7 +423,22 @@ def fetch_hierarchy_issues(*, repo_owner: str, repo_name: str, fingerprint_only: for node in nodes: if not isinstance(node, Mapping): continue - parsed = _parse_issue_node(node, include_body=not fingerprint_only) + issue_type_node = _mapping_value(node, "issueType") + issue_type_name = str(issue_type_node["name"]) if issue_type_node and issue_type_node.get("name") else None + if issue_type_name not in SUPPORTED_ISSUE_TYPES: + continue + + issue_number = int(node["number"]) + issue_url = str(node["url"]) + sub_nodes = _fetch_all_subissue_nodes( + repo_owner=repo_owner, + repo_name=repo_name, + issue_number=issue_number, + issue_url=issue_url, + ) + children = _child_links(sub_nodes) + + parsed = _parse_issue_node(node, include_body=not fingerprint_only, children=children) if parsed is not None: issues.append(parsed) page_info = issue_connection.get("pageInfo", {}) @@ -341,9 +459,10 @@ def compute_hierarchy_fingerprint(issues: list[HierarchyIssue]) -> str: { "number": issue.number, "title": issue.title, + "url": issue.url, "issue_type": issue.issue_type, - "updated_at": issue.updated_at, "labels": sorted(issue.labels, key=str.lower), + "summary": issue.summary, "parent_number": issue.parent.number if issue.parent else None, "child_numbers": [child.number for child in sorted(issue.children, key=lambda item: item.number)], } diff --git a/scripts/validate_agent_rule_applies_when.py b/scripts/validate_agent_rule_applies_when.py index 8d6db4c2..f831b85c 100644 --- a/scripts/validate_agent_rule_applies_when.py +++ b/scripts/validate_agent_rule_applies_when.py @@ -52,7 +52,11 @@ def _parse_frontmatter(text: str) -> dict[str, object] | str: def _iter_signal_errors(rules_dir: Path) -> list[str]: errors: list[str] = [] for path in sorted(rules_dir.glob("*.md")): - text = path.read_text(encoding="utf-8") + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + errors.append(f"{path.name}: failed to read file: {exc}") + continue parsed = _parse_frontmatter(text) if isinstance(parsed, str): errors.append(f"{path.name}: {parsed}") @@ -64,7 +68,12 @@ def _iter_signal_errors(rules_dir: Path) -> list[str]: if isinstance(raw, str): signals = [raw] elif isinstance(raw, list): - signals = [str(item) for item in raw if item is not None] + invalid = [item for item in raw if item is not None and not isinstance(item, str)] + if invalid: + bad_types = sorted({type(item).__name__ for item in invalid}) + errors.append(f"{path.name}: applies_when list entries must be str or null; got types: {bad_types}") + continue + signals = [item for item in raw if isinstance(item, str)] else: errors.append(f"{path.name}: applies_when must be a list or string") continue diff --git a/src/specfact_cli_modules/dev_bootstrap.py b/src/specfact_cli_modules/dev_bootstrap.py index 4f950fdb..1af08d26 100644 --- a/src/specfact_cli_modules/dev_bootstrap.py +++ b/src/specfact_cli_modules/dev_bootstrap.py @@ -56,10 +56,10 @@ def apply_specfact_workspace_env(repo_root: Path) -> None: when both exist—remove stale user copies with ``specfact module uninstall --scope user``. """ resolved = repo_root.resolve() - os.environ.setdefault("SPECFACT_MODULES_REPO", str(resolved)) + os.environ["SPECFACT_MODULES_REPO"] = str(resolved) core = resolve_core_repo(repo_root) if core is not None: - os.environ.setdefault("SPECFACT_REPO_ROOT", str(core)) + os.environ["SPECFACT_REPO_ROOT"] = str(core) def _installed_core_exists() -> bool: diff --git a/tests/unit/scripts/test_pre_commit_code_review.py b/tests/unit/scripts/test_pre_commit_code_review.py index cf5d0188..3ce4a69e 100644 --- a/tests/unit/scripts/test_pre_commit_code_review.py +++ b/tests/unit/scripts/test_pre_commit_code_review.py @@ -25,37 +25,53 @@ def _load_script_module() -> Any: return module -def test_filter_review_files_keeps_only_python_sources() -> None: - """Only relevant staged Python files should be reviewed.""" +SAMPLE_FAIL_REVIEW_REPORT: dict[str, object] = { + "overall_verdict": "FAIL", + "findings": [ + {"severity": "error", "rule": "e1"}, + {"severity": "warning", "rule": "w1"}, + ], +} + + +def test_filter_review_gate_paths_keeps_contract_relevant_trees() -> None: + """Review gate should include staged paths under tooling and contract trees.""" module = _load_script_module() - assert module.filter_review_files(["src/app.py", "README.md", "tests/test_app.py", "notes.txt"]) == [ - "src/app.py", - "tests/test_app.py", - ] + assert module.filter_review_gate_paths( + [ + "src/app.py", + "README.md", + "tests/test_app.py", + "openspec/changes/foo/tasks.md", + "openspec/changes/foo/TDD_EVIDENCE.md", + "notes.txt", + ] + ) == ["tests/test_app.py", "openspec/changes/foo/tasks.md"] def test_build_review_command_writes_json_report() -> None: """Pre-commit gate should write ReviewReport JSON for IDE/Copilot and use exit verdict.""" module = _load_script_module() - command = module.build_review_command(["src/app.py", "tests/test_app.py"]) + command = module.build_review_command(["tests/test_app.py", "packages/specfact-spec/src/x.py"]) assert command[:5] == [sys.executable, "-m", "specfact_cli.cli", "code", "review"] assert "--json" in command assert "--out" in command assert module.REVIEW_JSON_OUT in command - assert command[-2:] == ["src/app.py", "tests/test_app.py"] + assert command[-2:] == ["tests/test_app.py", "packages/specfact-spec/src/x.py"] def test_main_skips_when_no_relevant_files(capsys: pytest.CaptureFixture[str]) -> None: - """Hook should not fail commits when no staged Python files are present.""" + """Hook should not fail commits when no staged review-relevant paths are present.""" module = _load_script_module() - exit_code = module.main(["README.md", "docs/guide.md"]) + exit_code = module.main(["README.md", "docs/guide.md", "src/app.py"]) assert exit_code == 0 - assert "No staged Python files" in capsys.readouterr().out + out = capsys.readouterr().out + assert "skipping code review gate" in out def test_main_propagates_review_gate_exit_code( @@ -64,16 +80,7 @@ def test_main_propagates_review_gate_exit_code( """Blocking review verdicts must block the commit by returning non-zero.""" module = _load_script_module() repo_root = tmp_path - _write_sample_review_report( - repo_root, - { - "overall_verdict": "FAIL", - "findings": [ - {"severity": "error", "rule": "e1"}, - {"severity": "warning", "rule": "w1"}, - ], - }, - ) + _write_sample_review_report(repo_root, SAMPLE_FAIL_REVIEW_REPORT) def _fake_root() -> Path: return repo_root @@ -86,23 +93,14 @@ def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[s assert module.REVIEW_JSON_OUT in cmd assert kwargs.get("cwd") == str(repo_root) assert kwargs.get("timeout") == 300 - _write_sample_review_report( - repo_root, - { - "overall_verdict": "FAIL", - "findings": [ - {"severity": "error", "rule": "e1"}, - {"severity": "warning", "rule": "w1"}, - ], - }, - ) + _write_sample_review_report(repo_root, SAMPLE_FAIL_REVIEW_REPORT) return subprocess.CompletedProcess(cmd, 1, stdout=".specfact/code-review.json\n", stderr="") monkeypatch.setattr(module, "_repo_root", _fake_root) monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) monkeypatch.setattr(module.subprocess, "run", _fake_run) - exit_code = module.main(["src/app.py"]) + exit_code = module.main(["tests/unit/test_app.py"]) assert exit_code == 1 captured = capsys.readouterr() @@ -160,7 +158,7 @@ def _fake_run(cmd: list[str], **_kwargs: object) -> subprocess.CompletedProcess[ monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) monkeypatch.setattr(module.subprocess, "run", _fake_run) - exit_code = module.main(["src/app.py"]) + exit_code = module.main(["tests/unit/test_app.py"]) assert exit_code == 2 err = capsys.readouterr().err @@ -186,12 +184,12 @@ def _fake_run(cmd: list[str], **kwargs: object) -> subprocess.CompletedProcess[s monkeypatch.setattr(module.subprocess, "run", _fake_run) monkeypatch.setattr(module, "_repo_root", lambda: repo_root) - exit_code = module.main(["src/app.py"]) + exit_code = module.main(["tests/unit/test_app.py"]) assert exit_code == 1 err = capsys.readouterr().err assert "timed out after 300s" in err - assert "src/app.py" in err + assert "tests/unit/test_app.py" in err def test_main_prints_actionable_setup_guidance_when_runtime_missing( @@ -205,7 +203,7 @@ def _fake_ensure() -> tuple[bool, str | None]: monkeypatch.setattr(module, "ensure_runtime_available", _fake_ensure) - exit_code = module.main(["src/app.py"]) + exit_code = module.main(["tests/unit/test_app.py"]) assert exit_code == 1 assert "dev-deps" in capsys.readouterr().out diff --git a/tests/unit/scripts/test_sync_github_hierarchy_cache.py b/tests/unit/scripts/test_sync_github_hierarchy_cache.py index f3e78704..57a7646c 100644 --- a/tests/unit/scripts/test_sync_github_hierarchy_cache.py +++ b/tests/unit/scripts/test_sync_github_hierarchy_cache.py @@ -75,6 +75,36 @@ def _make_issue( ) +def test_compute_hierarchy_fingerprint_ignores_updated_at_but_reflects_rendered_fields() -> None: + """Fingerprint should match cache markdown inputs, not GitHub activity timestamps.""" + module = _load_script_module() + + base = _make_issue( + module, + number=485, + title="[Epic] Governance", + issue_type="Epic", + options={"labels": ["Epic"], "summary": "Same summary.", "updated_at": "2026-04-09T08:00:00Z"}, + ) + moved_time = _make_issue( + module, + number=485, + title="[Epic] Governance", + issue_type="Epic", + options={"labels": ["Epic"], "summary": "Same summary.", "updated_at": "2026-04-10T12:00:00Z"}, + ) + assert module.compute_hierarchy_fingerprint([base]) == module.compute_hierarchy_fingerprint([moved_time]) + + different_summary = _make_issue( + module, + number=485, + title="[Epic] Governance", + issue_type="Epic", + options={"labels": ["Epic"], "summary": "Changed summary.", "updated_at": "2026-04-09T08:00:00Z"}, + ) + assert module.compute_hierarchy_fingerprint([base]) != module.compute_hierarchy_fingerprint([different_summary]) + + def test_compute_hierarchy_fingerprint_is_order_independent() -> None: """Fingerprinting should stay stable regardless of input ordering.""" module = _load_script_module() @@ -465,13 +495,14 @@ def _no_git(*_args: Any, **_kwargs: Any) -> Any: raise FileNotFoundError("git not found") monkeypatch.setattr(subprocess, "run", _no_git) - module = _load_script_module() - script_path = Path(__file__).resolve().parents[3] / "scripts" / "sync_github_hierarchy_cache.py" - expected_fallback = script_path.resolve().parents[1].name - assert expected_fallback == module.DEFAULT_REPO_NAME - - _load_script_module.cache_clear() - sys.modules.pop("sync_github_hierarchy_cache", None) + try: + module = _load_script_module() + script_path = Path(__file__).resolve().parents[3] / "scripts" / "sync_github_hierarchy_cache.py" + expected_fallback = script_path.resolve().parents[1].name + assert expected_fallback == module.DEFAULT_REPO_NAME + finally: + _load_script_module.cache_clear() + sys.modules.pop("sync_github_hierarchy_cache", None) def test_main_reports_runtime_error_to_stderr_and_returns_one( diff --git a/tests/unit/scripts/test_validate_agent_rule_applies_when.py b/tests/unit/scripts/test_validate_agent_rule_applies_when.py index d3ba6b99..e8019b39 100644 --- a/tests/unit/scripts/test_validate_agent_rule_applies_when.py +++ b/tests/unit/scripts/test_validate_agent_rule_applies_when.py @@ -74,6 +74,22 @@ def test_frontmatter_scalar_root_is_rejected(tmp_path: Path) -> None: assert "mapping" in errors[0].lower() +def test_applies_when_list_rejects_non_string_entries(tmp_path: Path) -> None: + rules_dir = tmp_path / "docs" / "agent-rules" + rules_dir.mkdir(parents=True) + (rules_dir / "bad_list.md").write_text( + "---\napplies_when: [session-bootstrap, 42]\n---\n# body\n", + encoding="utf-8", + ) + + mod = _load_validator_module() + errors = mod._iter_signal_errors(rules_dir) + + assert len(errors) == 1 + assert "bad_list.md" in errors[0] + assert "applies_when" in errors[0] + + def test_valid_applies_when_in_temp_rules_dir_passes(tmp_path: Path) -> None: rules_dir = tmp_path / "docs" / "agent-rules" rules_dir.mkdir(parents=True) diff --git a/tests/unit/test_dev_bootstrap.py b/tests/unit/test_dev_bootstrap.py index 22a2df60..1aed7aa7 100644 --- a/tests/unit/test_dev_bootstrap.py +++ b/tests/unit/test_dev_bootstrap.py @@ -82,14 +82,19 @@ def test_apply_specfact_workspace_env_without_core_repo(monkeypatch: pytest.Monk assert "SPECFACT_REPO_ROOT" not in os.environ -def test_apply_specfact_workspace_env_respects_existing_env(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: +def test_apply_specfact_workspace_env_overwrites_stale_exports(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: repo_root = tmp_path / "modules-repo" repo_root.mkdir() - monkeypatch.setenv("SPECFACT_MODULES_REPO", "/already/set") - monkeypatch.setenv("SPECFACT_REPO_ROOT", "/core/set") + core = _make_core_repo(tmp_path / "core") + monkeypatch.setenv("SPECFACT_MODULES_REPO", "/stale/modules") + monkeypatch.setenv("SPECFACT_REPO_ROOT", "/stale/core") + monkeypatch.setattr( + "specfact_cli_modules.dev_bootstrap.resolve_core_repo", + lambda _root: core.resolve(), + ) apply_specfact_workspace_env(repo_root) - assert os.environ["SPECFACT_MODULES_REPO"] == "/already/set" - assert os.environ["SPECFACT_REPO_ROOT"] == "/core/set" + assert os.environ["SPECFACT_MODULES_REPO"] == str(repo_root.resolve()) + assert os.environ["SPECFACT_REPO_ROOT"] == str(core.resolve()) def test_ensure_core_dependency_allows_matching_editable_core(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: diff --git a/tests/unit/tools/test_contract_first_smart_test.py b/tests/unit/tools/test_contract_first_smart_test.py index 2b9eff8e..55427743 100644 --- a/tests/unit/tools/test_contract_first_smart_test.py +++ b/tests/unit/tools/test_contract_first_smart_test.py @@ -29,6 +29,7 @@ def cfst_mod(): def test_names_require_contract_test_detects_relevant_paths(cfst_mod) -> None: + assert cfst_mod._names_require_contract_test(["contracts/some_contract.yaml"]) is True assert cfst_mod._names_require_contract_test(["tests/unit/test_x.py"]) is True assert cfst_mod._names_require_contract_test(["packages/specfact-backlog/src/x.py"]) is True assert cfst_mod._names_require_contract_test(["src/specfact_cli_modules/x.py"]) is True @@ -59,3 +60,14 @@ def test_contract_test_status_returns_zero_when_only_irrelevant_staged( lambda _root: ["docs/README.md"], ) assert cfst_mod._contract_test_status() == 0 + + +def test_contract_test_status_returns_one_when_relevant_files_are_staged( + monkeypatch: pytest.MonkeyPatch, cfst_mod +) -> None: + monkeypatch.setattr( + cfst_mod, + "_git_staged_names", + lambda _root: ["contracts/some_contract.yaml"], + ) + assert cfst_mod._contract_test_status() == 1 diff --git a/tools/contract_first_smart_test.py b/tools/contract_first_smart_test.py index f2f1049e..9fbeed4b 100755 --- a/tools/contract_first_smart_test.py +++ b/tools/contract_first_smart_test.py @@ -14,6 +14,7 @@ # Staged paths that should trigger `contract-test` in pre-commit when present. _RELEVANT_PREFIXES = ( + "contracts/", "tests/", "packages/", "src/", From 87c97435d5e2dafc589f45905d69bf80d13361c2 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 13 Apr 2026 10:21:20 +0000 Subject: [PATCH 2/3] fix(pre-commit): do not treat gate scripts as safe-change skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex P2: pre_commit_code_review.py and contract_first_smart_test.py must not bypass Block 2 when staged alone—they define the review and contract-test gates. Co-authored-by: Dom --- scripts/pre-commit-quality-checks.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/pre-commit-quality-checks.sh b/scripts/pre-commit-quality-checks.sh index e85820c1..4de311e0 100755 --- a/scripts/pre-commit-quality-checks.sh +++ b/scripts/pre-commit-quality-checks.sh @@ -91,7 +91,7 @@ check_safe_change() { ;; pyproject.toml|README.md|CHANGELOG.md|.pre-commit-config.yaml) ;; - scripts/pre-commit-quality-checks.sh|scripts/pre_commit_code_review.py|tools/contract_first_smart_test.py) + scripts/pre-commit-quality-checks.sh) ;; *) other_changes=$((other_changes + 1)) From a177e0cc09b4dd7c5ab925038d6f85517fca611c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 13 Apr 2026 10:23:06 +0000 Subject: [PATCH 3/3] fix: CodeRabbit follow-ups (gates docs, JSON errors, SPECFACT_REPO_ROOT) - project-runtime tasks: add check-bundle-imports; explicit full code review step. - Agent rules: append specfact code review to canonical gate order with evidence path. - sync_github_hierarchy_cache: wrap subIssues JSON parse in RuntimeError for main(). - dev_bootstrap: pop SPECFACT_REPO_ROOT when core is unresolved; regression test. Co-authored-by: Dom --- docs/agent-rules/50-quality-gates-and-review.md | 1 + .../tasks.md | 4 ++-- scripts/sync_github_hierarchy_cache.py | 9 ++++++++- src/specfact_cli_modules/dev_bootstrap.py | 2 ++ tests/unit/test_dev_bootstrap.py | 16 ++++++++++++++++ 5 files changed, 29 insertions(+), 3 deletions(-) diff --git a/docs/agent-rules/50-quality-gates-and-review.md b/docs/agent-rules/50-quality-gates-and-review.md index 84c16735..2ffe6b90 100644 --- a/docs/agent-rules/50-quality-gates-and-review.md +++ b/docs/agent-rules/50-quality-gates-and-review.md @@ -47,6 +47,7 @@ depends_on: 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 diff --git a/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md b/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md index 6923a5fb..90faee43 100644 --- a/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md +++ b/openspec/changes/project-runtime-01-safe-artifact-write-policy/tasks.md @@ -20,6 +20,6 @@ - [ ] 4.1 Re-run the targeted runtime tests and any broader affected package coverage, then record passing evidence in `TDD_EVIDENCE.md`. - [ ] 4.2 Update affected modules docs to explain preservation guarantees and explicit replacement semantics for adopted commands. -- [ ] 4.3 Run quality gates in order: `hatch run format`, `hatch run type-check`, `hatch run lint`, `hatch run yaml-lint`, `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump`, then bump package versions and re-sign changed manifests if verification failed for that reason, then `hatch run contract-test`, then the relevant `hatch run smart-test` / `hatch run test` coverage for changed packages. -- [ ] 4.4 Ensure `.specfact/code-review.json` is fresh, remediate all findings, and record the final review command/timestamp in `TDD_EVIDENCE.md` or PR notes (last governance action before merge). +- [ ] 4.3 Run quality gates in order: `hatch run format` → `hatch run type-check` → `hatch run lint` → `hatch run yaml-lint` → `hatch run check-bundle-imports` → `hatch run verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump`, then bump package versions and re-sign changed manifests if verification failed for that reason, then `hatch run contract-test`, then the relevant `hatch run smart-test` / `hatch run test` coverage for changed packages. +- [ ] 4.4 Run `hatch run specfact code review run --json --out .specfact/code-review.json --scope full`, remediate all findings (or document a rare approved exception), attach or cite the resulting `.specfact/code-review.json`, and record the exact review command and timestamp in `TDD_EVIDENCE.md` or PR notes (last governance action before merge). - [ ] 4.5 Open the modules PR to `dev`, cross-link the paired core PR, and note any deferred runtime adoption paths as follow-up issues if the initial scope is intentionally limited. diff --git a/scripts/sync_github_hierarchy_cache.py b/scripts/sync_github_hierarchy_cache.py index 73747e05..1206fdf3 100755 --- a/scripts/sync_github_hierarchy_cache.py +++ b/scripts/sync_github_hierarchy_cache.py @@ -310,7 +310,14 @@ def _fetch_all_subissue_nodes( msg = completed.stderr.strip() or completed.stdout.strip() or "GitHub GraphQL query failed" raise RuntimeError(f"subIssues fetch failed for issue #{issue_number} ({issue_url}): {msg}") - payload = json.loads(completed.stdout) + raw_out = completed.stdout or "" + try: + payload = json.loads(raw_out) + except json.JSONDecodeError as exc: + excerpt = raw_out[:500] + ("…" if len(raw_out) > 500 else "") + raise RuntimeError( + f"subIssues: invalid JSON for issue #{issue_number} ({issue_url}): {exc}; body_excerpt={excerpt!r}" + ) from exc if "errors" in payload: raise RuntimeError( f"subIssues GraphQL errors for issue #{issue_number} ({issue_url}): " diff --git a/src/specfact_cli_modules/dev_bootstrap.py b/src/specfact_cli_modules/dev_bootstrap.py index 1af08d26..075ae44d 100644 --- a/src/specfact_cli_modules/dev_bootstrap.py +++ b/src/specfact_cli_modules/dev_bootstrap.py @@ -60,6 +60,8 @@ def apply_specfact_workspace_env(repo_root: Path) -> None: core = resolve_core_repo(repo_root) if core is not None: os.environ["SPECFACT_REPO_ROOT"] = str(core) + else: + os.environ.pop("SPECFACT_REPO_ROOT", None) def _installed_core_exists() -> bool: diff --git a/tests/unit/test_dev_bootstrap.py b/tests/unit/test_dev_bootstrap.py index 1aed7aa7..62964870 100644 --- a/tests/unit/test_dev_bootstrap.py +++ b/tests/unit/test_dev_bootstrap.py @@ -97,6 +97,22 @@ def test_apply_specfact_workspace_env_overwrites_stale_exports(monkeypatch: pyte assert os.environ["SPECFACT_REPO_ROOT"] == str(core.resolve()) +def test_apply_specfact_workspace_env_clears_stale_repo_root_when_core_unresolved( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + repo_root = tmp_path / "modules-repo" + repo_root.mkdir() + monkeypatch.setenv("SPECFACT_MODULES_REPO", "/stale/modules") + monkeypatch.setenv("SPECFACT_REPO_ROOT", "/stale/core-from-old-shell") + monkeypatch.setattr( + "specfact_cli_modules.dev_bootstrap.resolve_core_repo", + lambda _root: None, + ) + apply_specfact_workspace_env(repo_root) + assert os.environ["SPECFACT_MODULES_REPO"] == str(repo_root.resolve()) + assert "SPECFACT_REPO_ROOT" not in os.environ + + def test_ensure_core_dependency_allows_matching_editable_core(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: repo_root = tmp_path / "isolated-modules-repo" repo_root.mkdir(parents=True)