diff --git a/.github/workflows/docs-review.yml b/.github/workflows/docs-review.yml new file mode 100644 index 00000000..a77fd145 --- /dev/null +++ b/.github/workflows/docs-review.yml @@ -0,0 +1,62 @@ +# yaml-language-server: $schema=https://json.schemastore.org/github-workflow.json +# yamllint disable rule:line-length rule:truthy +name: Docs Review + +on: + pull_request: + branches: [main, dev] + paths: + - "**/*.md" + - "**/*.mdc" + - "docs/**" + - "tests/unit/docs/test_release_docs_parity.py" + - ".github/workflows/docs-review.yml" + push: + branches: [main, dev] + paths: + - "**/*.md" + - "**/*.mdc" + - "docs/**" + - "tests/unit/docs/test_release_docs_parity.py" + - ".github/workflows/docs-review.yml" + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + docs-review: + name: Docs Review + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Set up Python 3.12 + uses: actions/setup-python@v5 + with: + python-version: "3.12" + cache: "pip" + + - name: Install docs review dependencies + run: | + python -m pip install --upgrade pip + python -m pip install pytest + + - name: Run docs review suite + run: | + mkdir -p logs/docs-review + DOCS_REVIEW_LOG="logs/docs-review/docs-review_$(date -u +%Y%m%d_%H%M%S).log" + python -m pytest tests/unit/docs/test_release_docs_parity.py -q 2>&1 | tee "$DOCS_REVIEW_LOG" + exit "${PIPESTATUS[0]:-$?}" + + - name: Upload docs review logs + if: always() + uses: actions/upload-artifact@v4 + with: + name: docs-review-logs + path: logs/docs-review/ + if-no-files-found: ignore diff --git a/docs/adapters/azuredevops.md b/docs/adapters/azuredevops.md index 95485c85..7223950b 100644 --- a/docs/adapters/azuredevops.md +++ b/docs/adapters/azuredevops.md @@ -273,10 +273,17 @@ adapter.import_artifact( ) # Access imported proposal -proposal = project_bundle.change_tracking.proposals["123"] +proposal = project_bundle.change_tracking.proposals["add-feature-x"] print(f"Imported: {proposal.title} - {proposal.status}") ``` +Selective imports preserve the native Azure DevOps payload, including +`fields`, so the imported work item remains valid input for proposal parsing and +bridge sync flows. When no OpenSpec metadata is present, imported proposal names +fall back to a title-derived slug such as `add-feature-x`; if that slug already +exists, the adapter appends the source work item ID (for example, +`add-feature-x-123`) to keep the proposal name stable and readable. + ### Status Synchronization ```python diff --git a/docs/adapters/backlog-adapter-patterns.md b/docs/adapters/backlog-adapter-patterns.md index 675ca5bc..2ad0ee73 100644 --- a/docs/adapters/backlog-adapter-patterns.md +++ b/docs/adapters/backlog-adapter-patterns.md @@ -493,4 +493,4 @@ When implementing new backlog adapters: - **[Azure DevOps Adapter Documentation](./azuredevops.md)** - Azure DevOps adapter reference - **[DevOps Adapter Integration Guide](../guides/devops-adapter-integration.md)** - Complete integration guide for GitHub and ADO - **[Validation Integration](../validation-integration.md)** - Validation with change proposals -- **[Bridge Adapter Interface](../bridge-adapter-interface.md)** - Base adapter interface +- **[Bridge Adapter Interface](../reference/architecture.md#required-adapter-interface)** - Base adapter interface diff --git a/docs/examples/integration-showcases/README.md b/docs/examples/integration-showcases/README.md index 5b062f0d..639a4fc9 100644 --- a/docs/examples/integration-showcases/README.md +++ b/docs/examples/integration-showcases/README.md @@ -39,7 +39,7 @@ This folder contains everything you need to understand and test SpecFact CLI int ### Setup Script -1. **[`setup-integration-tests.sh`](setup-integration-tests.sh)** πŸš€ **AUTOMATED SETUP** +1. **[`setup-integration-tests.sh`](https://github.com/nold-ai/specfact-cli/blob/main/docs/examples/integration-showcases/setup-integration-tests.sh)** πŸš€ **AUTOMATED SETUP** - **Purpose**: Automated script to create test cases for all examples - **Content**: Creates test directories, sample code, and configuration files @@ -59,7 +59,7 @@ This gives you a complete overview of what SpecFact can do with real examples. **Step 2**: Choose your path: -- **Want to test the examples?** β†’ Use [`setup-integration-tests.sh`](setup-integration-tests.sh) then follow [`integration-showcases-testing-guide.md`](integration-showcases-testing-guide.md) +- **Want to test the examples?** β†’ Use [`setup-integration-tests.sh`](https://github.com/nold-ai/specfact-cli/blob/main/docs/examples/integration-showcases/setup-integration-tests.sh) then follow [`integration-showcases-testing-guide.md`](integration-showcases-testing-guide.md) - **Just need quick commands?** β†’ Check [`integration-showcases-quick-reference.md`](integration-showcases-quick-reference.md) diff --git a/docs/examples/quick-examples.md b/docs/examples/quick-examples.md index 4728db31..62a93de5 100644 --- a/docs/examples/quick-examples.md +++ b/docs/examples/quick-examples.md @@ -1,7 +1,9 @@ --- layout: default title: Quick Examples -permalink: /quick-examples/ +permalink: /examples/quick-examples/ +redirect_from: + - /quick-examples/ --- # Quick Examples diff --git a/docs/getting-started/installation.md b/docs/getting-started/installation.md index 6dac2ea6..e9acfa1e 100644 --- a/docs/getting-started/installation.md +++ b/docs/getting-started/installation.md @@ -541,4 +541,4 @@ hatch run format hatch run lint ``` -See [CONTRIBUTING.md](../../CONTRIBUTING.md) for detailed contribution guidelines. +See [CONTRIBUTING.md](https://github.com/nold-ai/specfact-cli/blob/main/CONTRIBUTING.md) for detailed contribution guidelines. diff --git a/docs/getting-started/tutorial-openspec-speckit.md b/docs/getting-started/tutorial-openspec-speckit.md index eb52c134..de2ee1bf 100644 --- a/docs/getting-started/tutorial-openspec-speckit.md +++ b/docs/getting-started/tutorial-openspec-speckit.md @@ -688,4 +688,4 @@ gh repo view your-org/your-repo Copyright Β© 2025-2026 Nold AI (Owner: Dominikus Nold) -**Trademarks**: All product names, logos, and brands mentioned in this documentation are the property of their respective owners. NOLD AI (NOLDAI) is a registered trademark (wordmark) at the European Union Intellectual Property Office (EUIPO). See [TRADEMARKS.md](../../TRADEMARKS.md) for more information. +**Trademarks**: All product names, logos, and brands mentioned in this documentation are the property of their respective owners. NOLD AI (NOLDAI) is a registered trademark (wordmark) at the European Union Intellectual Property Office (EUIPO). See [TRADEMARKS.md](/TRADEMARKS/) for more information. diff --git a/docs/guides/ai-ide-workflow.md b/docs/guides/ai-ide-workflow.md index c9012ab7..f142cc09 100644 --- a/docs/guides/ai-ide-workflow.md +++ b/docs/guides/ai-ide-workflow.md @@ -1,7 +1,9 @@ --- layout: default title: AI IDE Workflow Guide -permalink: /ai-ide-workflow/ +permalink: /guides/ai-ide-workflow/ +redirect_from: + - /ai-ide-workflow/ --- # AI IDE Workflow Guide diff --git a/docs/guides/brownfield-engineer.md b/docs/guides/brownfield-engineer.md index 0f34bca1..254bae99 100644 --- a/docs/guides/brownfield-engineer.md +++ b/docs/guides/brownfield-engineer.md @@ -1,7 +1,9 @@ --- layout: default title: Modernizing Legacy Code (Brownfield Engineer Guide) -permalink: /brownfield-engineer/ +permalink: /guides/brownfield-engineer/ +redirect_from: + - /brownfield-engineer/ --- # Guide for Legacy Modernization Engineers diff --git a/docs/guides/brownfield-journey.md b/docs/guides/brownfield-journey.md index 00c5465d..e87c7c42 100644 --- a/docs/guides/brownfield-journey.md +++ b/docs/guides/brownfield-journey.md @@ -1,7 +1,9 @@ --- layout: default title: Brownfield Modernization Journey -permalink: /brownfield-journey/ +permalink: /guides/brownfield-journey/ +redirect_from: + - /brownfield-journey/ --- # Brownfield Modernization Journey diff --git a/docs/guides/common-tasks.md b/docs/guides/common-tasks.md index de640cca..fced0c7d 100644 --- a/docs/guides/common-tasks.md +++ b/docs/guides/common-tasks.md @@ -1,7 +1,9 @@ --- layout: default title: Common Tasks Quick Reference -permalink: /common-tasks/ +permalink: /guides/common-tasks/ +redirect_from: + - /common-tasks/ --- # Common Tasks Quick Reference diff --git a/docs/guides/competitive-analysis.md b/docs/guides/competitive-analysis.md index fb2f7368..aedc513d 100644 --- a/docs/guides/competitive-analysis.md +++ b/docs/guides/competitive-analysis.md @@ -1,7 +1,9 @@ --- layout: default title: Competitive Analysis -permalink: /competitive-analysis/ +permalink: /guides/competitive-analysis/ +redirect_from: + - /competitive-analysis/ --- # What You Gain with SpecFact CLI diff --git a/docs/guides/copilot-mode.md b/docs/guides/copilot-mode.md index db545729..902631c0 100644 --- a/docs/guides/copilot-mode.md +++ b/docs/guides/copilot-mode.md @@ -1,7 +1,9 @@ --- layout: default title: Using CoPilot Mode -permalink: /copilot-mode/ +permalink: /guides/copilot-mode/ +redirect_from: + - /copilot-mode/ --- # Using CoPilot Mode diff --git a/docs/guides/ide-integration.md b/docs/guides/ide-integration.md index f2abb1f4..54ba292a 100644 --- a/docs/guides/ide-integration.md +++ b/docs/guides/ide-integration.md @@ -342,4 +342,4 @@ The `specfact init` command handles all conversions automatically. --- -**Trademarks**: All product names, logos, and brands mentioned in this guide are the property of their respective owners. NOLD AI (NOLDAI) is a registered trademark (wordmark) at the European Union Intellectual Property Office (EUIPO). See [TRADEMARKS.md](../../TRADEMARKS.md) for more information. +**Trademarks**: All product names, logos, and brands mentioned in this guide are the property of their respective owners. NOLD AI (NOLDAI) is a registered trademark (wordmark) at the European Union Intellectual Property Office (EUIPO). See [TRADEMARKS.md](/TRADEMARKS/) for more information. diff --git a/docs/guides/migration-guide.md b/docs/guides/migration-guide.md index c9c080e7..5a8f68e3 100644 --- a/docs/guides/migration-guide.md +++ b/docs/guides/migration-guide.md @@ -1,7 +1,9 @@ --- layout: default title: Migration Guide -permalink: /migration-guide/ +permalink: /guides/migration-guide/ +redirect_from: + - /migration-guide/ --- # Migration Guide diff --git a/docs/guides/team-collaboration-workflow.md b/docs/guides/team-collaboration-workflow.md index 91a78479..01a40813 100644 --- a/docs/guides/team-collaboration-workflow.md +++ b/docs/guides/team-collaboration-workflow.md @@ -1,7 +1,9 @@ --- layout: default title: Team Collaboration Workflow -permalink: /team-collaboration-workflow/ +permalink: /guides/team-collaboration-workflow/ +redirect_from: + - /team-collaboration-workflow/ --- # Team Collaboration Workflow diff --git a/docs/guides/testing-terminal-output.md b/docs/guides/testing-terminal-output.md index 21b7bfd5..10a59f87 100644 --- a/docs/guides/testing-terminal-output.md +++ b/docs/guides/testing-terminal-output.md @@ -1,7 +1,9 @@ --- layout: default title: Testing Terminal Output Modes -permalink: /testing-terminal-output/ +permalink: /guides/testing-terminal-output/ +redirect_from: + - /testing-terminal-output/ --- # Testing Terminal Output Modes diff --git a/docs/guides/troubleshooting.md b/docs/guides/troubleshooting.md index e1eb0614..e7dc04f7 100644 --- a/docs/guides/troubleshooting.md +++ b/docs/guides/troubleshooting.md @@ -1,7 +1,9 @@ --- layout: default title: Troubleshooting -permalink: /troubleshooting/ +permalink: /guides/troubleshooting/ +redirect_from: + - /troubleshooting/ --- # Troubleshooting diff --git a/docs/guides/use-cases.md b/docs/guides/use-cases.md index 4faed708..193b3289 100644 --- a/docs/guides/use-cases.md +++ b/docs/guides/use-cases.md @@ -1,7 +1,9 @@ --- layout: default title: Use Cases -permalink: /use-cases/ +permalink: /guides/use-cases/ +redirect_from: + - /use-cases/ --- # Use Cases diff --git a/docs/guides/ux-features.md b/docs/guides/ux-features.md index 86d50959..1c79a680 100644 --- a/docs/guides/ux-features.md +++ b/docs/guides/ux-features.md @@ -1,7 +1,9 @@ --- layout: default title: UX Features Guide -permalink: /ux-features/ +permalink: /guides/ux-features/ +redirect_from: + - /ux-features/ --- # UX Features Guide diff --git a/docs/openspec-opsx-migration.md b/docs/openspec-opsx-migration.md index 16dff6a6..0149eee4 100644 --- a/docs/openspec-opsx-migration.md +++ b/docs/openspec-opsx-migration.md @@ -20,7 +20,7 @@ SpecFact CLI has migrated to **OpenSpec OPSX** (fluid, action-based workflow). P - **Project context import**: When syncing OpenSpec project context, the bridge resolves `openspec/config.yaml` first; if absent, it uses `openspec/project.md`. No code changes required in repos that still have `project.md`. - **Detection**: OpenSpec is detected if `openspec/config.yaml`, `openspec/project.md`, or `openspec/specs/` exists. -- **Config format**: See [openspec/config.yaml](../openspec/config.yaml) in this repo for an example. The `context:` block is injected into planning; `rules:` apply per-artifact. +- **Config format**: See [openspec/config.yaml](https://github.com/nold-ai/specfact-cli/blob/main/openspec/config.yaml) in this repo for an example. The `context:` block is injected into planning; `rules:` apply per-artifact. ## References diff --git a/docs/reference/architecture.md b/docs/reference/architecture.md index da2ec213..dca80dec 100644 --- a/docs/reference/architecture.md +++ b/docs/reference/architecture.md @@ -1,7 +1,7 @@ --- layout: default title: Architecture -permalink: /architecture/ +permalink: /reference/architecture/ --- # Architecture diff --git a/docs/reference/debug-logging.md b/docs/reference/debug-logging.md index 0063a798..76dba83c 100644 --- a/docs/reference/debug-logging.md +++ b/docs/reference/debug-logging.md @@ -1,7 +1,9 @@ --- layout: default title: Debug Logging -permalink: /debug-logging/ +permalink: /reference/debug-logging/ +redirect_from: + - /debug-logging/ --- diff --git a/docs/reference/directory-structure.md b/docs/reference/directory-structure.md index 3673ddf7..20a7217c 100644 --- a/docs/reference/directory-structure.md +++ b/docs/reference/directory-structure.md @@ -1,7 +1,9 @@ --- layout: default title: SpecFact CLI Directory Structure -permalink: /directory-structure/ +permalink: /reference/directory-structure/ +redirect_from: + - /directory-structure/ --- # SpecFact CLI Directory Structure diff --git a/docs/reference/modes.md b/docs/reference/modes.md index 36a1528f..9414975a 100644 --- a/docs/reference/modes.md +++ b/docs/reference/modes.md @@ -1,7 +1,9 @@ --- layout: default title: Operational Modes -permalink: /modes/ +permalink: /reference/modes/ +redirect_from: + - /modes/ --- # Operational Modes diff --git a/docs/reference/parameter-standard.md b/docs/reference/parameter-standard.md index 4d2fad77..583715dd 100644 --- a/docs/reference/parameter-standard.md +++ b/docs/reference/parameter-standard.md @@ -237,9 +237,9 @@ def generate_contracts( ## πŸ”— Related Documentation -- **[CLI Reorganization Implementation Plan](../../specfact-cli-internal/docs/internal/implementation/CLI_REORGANIZATION_IMPLEMENTATION_PLAN.md)** - Full reorganization plan -- **[Command Reference](./commands.md)** - Complete command reference -- **[Project Bundle Refactoring Plan](../../specfact-cli-internal/docs/internal/implementation/PROJECT_BUNDLE_REFACTORING_PLAN.md)** - Bundle parameter requirements +- **[Command Reference](./commands.md)** - Complete command reference and current parameter surfaces +- **[Directory Structure](./directory-structure.md)** - Repository and workspace parameter context +- **[Architecture](./architecture.md)** - Runtime and adapter architecture context for parameter design --- diff --git a/docs/reference/schema-versioning.md b/docs/reference/schema-versioning.md index 045af7d4..b3ee24ba 100644 --- a/docs/reference/schema-versioning.md +++ b/docs/reference/schema-versioning.md @@ -1,7 +1,9 @@ --- layout: default title: Schema Versioning -permalink: /schema-versioning/ +permalink: /reference/schema-versioning/ +redirect_from: + - /schema-versioning/ --- # Schema Versioning @@ -174,5 +176,5 @@ schema_metadata: ## Related Documentation - [Architecture - Change Tracking Models](../reference/architecture.md#change-tracking-models-v11-schema) - Technical details -- [Architecture - Bridge Adapter Interface](../reference/architecture.md#bridge-adapter-interface) - Adapter implementation guide +- [Architecture - Required Adapter Interface](../reference/architecture.md#required-adapter-interface) - Adapter implementation guide - [Directory Structure](directory-structure.md) - Bundle file organization diff --git a/docs/technical/README.md b/docs/technical/README.md index 4caf47c2..d3c11e3b 100644 --- a/docs/technical/README.md +++ b/docs/technical/README.md @@ -17,7 +17,7 @@ Technical documentation for contributors and developers working on SpecFact CLI. ### Maintenance Scripts -For maintenance scripts and developer utilities, see the [Contributing Guide](../../CONTRIBUTING.md#developer-tools) section on Developer Tools. This includes: +For maintenance scripts and developer utilities, see the [Contributing Guide](https://github.com/nold-ai/specfact-cli/blob/main/CONTRIBUTING.md#developer-tools) section on Developer Tools. This includes: - **Cleanup Acceptance Criteria Script** - Removes duplicate replacement instruction text from acceptance criteria - Other maintenance and development utilities in the `scripts/` directory diff --git a/openspec/CHANGE_ORDER.md b/openspec/CHANGE_ORDER.md index 10d20981..616f04a6 100644 --- a/openspec/CHANGE_ORDER.md +++ b/openspec/CHANGE_ORDER.md @@ -56,6 +56,14 @@ Only changes that are **archived**, shown as **βœ“ Complete** by `openspec list` Entries in the tables below are pending unless explicitly marked as implemented (archived). +## Dogfooding + +| Module | Order | Change folder | GitHub # | Blocked by | +|--------|-------|---------------|----------|------------| +| dogfooding | 02 | code-review-zero-findings | #423 | β€” | + +--- + ## Plan-derived addendum (2026-02-15 architecture integration plan) The source plan inventory listed 17 new changes. Two additional cross-cutting changes were intentionally added during proposal creation to close integration governance and proof gaps discovered in dependency review: @@ -82,6 +90,7 @@ These are derived extensions of the same 2026-02-15 plan and are required to ope |--------|-------|---------------|----------|------------| | docs | 01 | docs-01-core-modules-docs-alignment | [#348](https://github.com/nold-ai/specfact-cli/issues/348) | module-migration-01 βœ…; module-migration-02 βœ…; module-migration-03 βœ…; module-migration-05 βœ…; module-migration-06/07 outputs inform residual cleanup wording | | docs | 03 | βœ… docs-03-command-syntax-parity (archived 2026-03-18) | pending | docs-01 βœ…; docs-02 βœ… | +| docs | 04 | docs-04-docs-review-gate-and-link-integrity | pending | docs-03 βœ… | ### Marketplace (module distribution) @@ -149,6 +158,7 @@ These are derived extensions of the same 2026-02-15 plan and are required to ope | backlog-core | 06 | βœ… backlog-core-06-refine-custom-field-writeback (implemented 2026-03-03; archived) | [#310](https://github.com/nold-ai/specfact-cli/issues/310) | #173 | | backlog-core | 07 | backlog-core-07-ado-required-custom-fields-and-picklists | [#337](https://github.com/nold-ai/specfact-cli/issues/337) | βœ… #310 | | bugfix | 01 | bugfix-backlog-html-export-validation | TBD | β€” | +| bugfix | 02 | bugfix-02-ado-import-payload-slugging | [#427](https://github.com/nold-ai/specfact-cli/issues/427) | β€” | ### backlog-scrum diff --git a/openspec/changes/bugfix-02-ado-import-payload-slugging/TDD_EVIDENCE.md b/openspec/changes/bugfix-02-ado-import-payload-slugging/TDD_EVIDENCE.md new file mode 100644 index 00000000..83f4140d --- /dev/null +++ b/openspec/changes/bugfix-02-ado-import-payload-slugging/TDD_EVIDENCE.md @@ -0,0 +1,40 @@ +# TDD Evidence: bugfix-02-ado-import-payload-slugging + +## Failing First + +- `2026-03-20`: `hatch run pytest tests/unit/adapters/test_ado.py tests/unit/adapters/test_github.py tests/integration/sync/test_ado_backlog_sync.py tests/integration/sync/test_backlog_sync.py tests/unit/sync/test_bridge_sync_import.py -q` + failed with `5 failed, 73 passed` before the fix. The failures showed two regressions: + selective ADO fetch reduced the work item to a summary payload without native + `fields`, and shared backlog import fell back to numeric-only change IDs such + as `123` instead of title-derived slugs. +- `2026-03-20`: audit of adjacent import paths confirmed that the shared + `import_backlog_item_as_proposal()` helper is used by both the ADO and GitHub + adapters, so the title-first normalization fix had to be applied in the + shared backlog adapter rather than only in the ADO implementation. + +## Passing + +- `2026-03-20`: `hatch run pytest tests/unit/adapters/test_ado.py tests/unit/adapters/test_github.py tests/unit/sync/test_bridge_sync_import.py -q` + passed with `76 passed`. This covers native selective fetch for ADO and + GitHub, title-first imported change IDs, deterministic collision suffixes, + and the bridge selective-import contract. + +- `2026-03-20`: `openspec validate bugfix-02-ado-import-payload-slugging --strict` + passed. +- `2026-03-20`: `hatch run format` + passed after reformatting the touched files. +- `2026-03-20`: `hatch run type-check` + passed with `0 errors`. +- `2026-03-20`: `hatch run yaml-lint` + passed. +- `2026-03-20`: `hatch run contract-test` + exited `0` using cached results (`No modified files detected - using cached results`). +- `2026-03-20`: `hatch run smart-test` + exited `0`; the command skipped mapped test execution for this delta and emitted only its standard coverage warning. +- `2026-03-20`: `hatch run specfact code review run --json --out .codex-bugfix-02-review.json src/specfact_cli/adapters/ado.py src/specfact_cli/adapters/backlog_base.py src/specfact_cli/adapters/github.py tests/unit/adapters/test_ado.py tests/unit/adapters/test_github.py tests/unit/sync/test_bridge_sync_import.py tests/integration/sync/test_ado_backlog_sync.py tests/integration/sync/test_backlog_sync.py` + exported the governed review report to `.codex-bugfix-02-review.json`. The overall report still fails because the touched legacy adapter files already carry a large historical baseline, but filtering the report to the lines changed by this fix yields `0` findings. + +## Outstanding Gate Caveat + +- `2026-03-20`: `hatch run lint` + still exits non-zero in this worktree because the repository-wide lint script (`ruff format . --check && basedpyright ... && ruff check . && pylint src tests tools`) surfaces existing baseline findings outside this bugfix. The modified lines for this change were reviewed separately via `.codex-bugfix-02-review.json` and returned `0` changed-line findings. diff --git a/openspec/changes/bugfix-02-ado-import-payload-slugging/design.md b/openspec/changes/bugfix-02-ado-import-payload-slugging/design.md new file mode 100644 index 00000000..361c1142 --- /dev/null +++ b/openspec/changes/bugfix-02-ado-import-payload-slugging/design.md @@ -0,0 +1,54 @@ +# Design: Fix ADO selective bridge import payload contract and title-based change IDs + +## Overview + +The failure in issue `#425` is a contract break inside the selective bridge import path: + +1. `BridgeSync` fetches one backlog item by ID for import. +2. `AdoAdapter.fetch_backlog_item()` returns a reduced summary payload. +3. `AdoAdapter.import_artifact()` and `extract_change_proposal_data()` still expect the native ADO work item shape with `fields`. +4. Import fails before OpenSpec change creation. + +Once the raw payload is restored, the fallback naming path still degrades to the numeric ADO work item ID when no OpenSpec metadata is present. That behavior is technically unique but operationally poor. The fallback should stay readable first, with the numeric source ID kept as provenance, not as the primary OpenSpec change name. + +## Design Decisions + +### 1. Preserve the provider-native payload on selective import + +`fetch_backlog_item()` for ADO should return the native work item document needed by the import path. If older call sites benefit from convenience keys such as `title`, `state`, and `description`, those can remain as additive compatibility fields, but they must not replace or strip the provider-native shape. + +This keeps the import contract coherent: + +- `fetch_backlog_item()` returns an artifact suitable for `import_artifact()` +- `extract_change_proposal_data()` reads the same native payload +- contract tests can verify the round trip directly + +### 2. Normalize imported change IDs in one shared place + +The fallback naming rule should be centralized in the shared backlog import path rather than hidden in one adapter-specific edge case. The normalizer should: + +- prefer an existing OpenSpec change ID if found in provider metadata +- otherwise derive a kebab-case slug from the proposal title +- append a deterministic suffix such as `-` when the slug already exists or would otherwise be ambiguous +- avoid using a raw numeric source ID as the entire change name unless the source artifact truly has no usable title + +This lets ADO fix its immediate bug while also protecting similar adapters and commands. + +### 3. Audit adjacent import commands, not just the failing ADO branch + +The regression appeared because one side of the contract evolved while the other kept assuming a richer payload. The implementation should therefore audit all nearby paths that do one or more of: + +- call `fetch_backlog_item()` +- call `extract_change_proposal_data()` +- call `import_backlog_item_as_proposal()` +- translate provider IDs into OpenSpec change IDs + +The goal is not a broad refactor. The goal is to prove that similar commands either already preserve the native payload correctly or are covered by targeted defensive tests after this fix. + +## Implementation Outline + +1. Add failing tests for ADO selective import and title-based slug fallback. +2. Restore native payload preservation in ADO selective fetch. +3. Add shared title-first change-ID normalization in the backlog import path. +4. Re-run targeted tests against adjacent adapters/call sites to confirm the contract holds. +5. Update docs and release notes for the patch behavior change. diff --git a/openspec/changes/bugfix-02-ado-import-payload-slugging/proposal.md b/openspec/changes/bugfix-02-ado-import-payload-slugging/proposal.md new file mode 100644 index 00000000..6c1bb4d0 --- /dev/null +++ b/openspec/changes/bugfix-02-ado-import-payload-slugging/proposal.md @@ -0,0 +1,45 @@ +# Change: Fix ADO selective bridge import payload contract and title-based change IDs + +## Why + +Issue [#425](https://github.com/nold-ai/specfact-cli/issues/425) confirms a real regression in the ADO selective import path used by `specfact project sync bridge --adapter ado --mode bidirectional --backlog-ids `. The current bridge flow calls `fetch_backlog_item()` and then imports the returned artifact as an OpenSpec change proposal, but `AdoAdapter._get_work_item_data()` strips the provider payload down to summary fields and drops the native `fields` object that `AdoAdapter.import_artifact()` and `extract_change_proposal_data()` still expect. Valid ADO work items therefore fail import with `ADO work item must have fields`. + +After a local hotfix restores the raw payload, the follow-up behavior is still wrong: when the ADO item does not already contain OpenSpec metadata in its description or comments, the generated change ID falls back to the numeric work item ID instead of a readable slug derived from the title. That makes imported OpenSpec changes hard to review, easy to collide, and inconsistent with the rest of the backlog import workflow. + +This change fixes both defects together and explicitly audits adjacent import commands that rely on the same `fetch_backlog_item()` to `import_artifact()` contract so we do not repeat the same summary-vs-native payload mistake in nearby adapters or bridge entry points. + +## What Changes + +- **MODIFY** `AdoAdapter.fetch_backlog_item()` and its internal work-item fetch helpers so selective ADO import returns the native work item payload, including `fields`, while preserving convenience keys such as `title`, `state`, and `description` for existing callers. +- **MODIFY** the ADO change-proposal extraction path so imported proposals derive a kebab-case change ID from the work item title when no OpenSpec metadata is already embedded in the source artifact. +- **MODIFY** shared backlog import normalization so duplicate or numeric-only fallback IDs get a deterministic suffix that keeps the readable slug and reserves the provider numeric ID for source tracking metadata instead of primary naming. +- **ADD** regression coverage for selective `project sync bridge` / bridge import flows, including raw payload preservation, title-based slug generation, duplicate-slug collision handling, and cross-checks for similar adapter import commands. +- **REVIEW** adjacent bridge/adapters that call `fetch_backlog_item()`, `extract_change_proposal_data()`, or `import_backlog_item_as_proposal()` so similar commands either share the same helper or are covered by explicit contract tests. + +## Capabilities + +### Modified Capabilities + +- `devops-sync`: selective ADO bridge imports must preserve the provider-native work item payload required for OpenSpec proposal import and must create readable change IDs when no prior metadata exists. +- `backlog-adapter`: adapter import contracts must preserve required native fields during single-item import and normalize imported proposal IDs title-first instead of defaulting to raw numeric source IDs. + +## Impact + +- **Affected code**: `src/specfact_cli/adapters/ado.py`, `src/specfact_cli/adapters/backlog_base.py`, and the selective import orchestration in `src/specfact_cli/sync/bridge_sync.py`; adjacent adapter call sites may need small defensive updates if the audit finds the same contract gap elsewhere. +- **Affected tests**: targeted unit/integration coverage under `tests/unit/adapters/`, `tests/unit/specfact_cli/adapters/`, `tests/unit/specfact_cli/sync/`, and any command-audit coverage that validates bridge command surfaces. +- **Documentation**: user-facing sync and ADO adapter docs in `docs/` plus any command reference examples that show selective bridge import should be reviewed so the fixed import behavior and change-ID expectations are documented. +- **Release impact**: patch release. No new command surface, but behavior changes are user-visible because valid ADO work items will import successfully and produce stable human-readable OpenSpec change IDs. +- **Sequencing**: no hard blocker in `openspec/CHANGE_ORDER.md`; the change should still be linked under backlog feature #357 and epic #186 and back to the originating bug issue. + +## Related Issues + +- Originating bug report: [#425](https://github.com/nold-ai/specfact-cli/issues/425) +- Parent feature: [#357](https://github.com/nold-ai/specfact-cli/issues/357) +- Parent epic: [#186](https://github.com/nold-ai/specfact-cli/issues/186) + +## Source Tracking + +- **GitHub Issue**: #427 +- **Issue URL**: +- **Repository**: nold-ai/specfact-cli +- **Last Synced Status**: open diff --git a/openspec/changes/bugfix-02-ado-import-payload-slugging/specs/backlog-adapter/spec.md b/openspec/changes/bugfix-02-ado-import-payload-slugging/specs/backlog-adapter/spec.md new file mode 100644 index 00000000..6ac1f711 --- /dev/null +++ b/openspec/changes/bugfix-02-ado-import-payload-slugging/specs/backlog-adapter/spec.md @@ -0,0 +1,22 @@ +## MODIFIED Requirements + +### Requirement: BacklogAdapter Interface + +The system SHALL provide a standard `BacklogAdapter` interface that all backlog sources (GitHub, ADO, JIRA, GitLab, etc.) must implement. + +#### Scenario: Selective proposal import preserves provider-native payload + +- **GIVEN** an adapter supports selective backlog import by explicit item reference +- **WHEN** bridge sync fetches one item through `fetch_backlog_item()` and passes the result into proposal import +- **THEN** the fetched artifact preserves the provider-native fields required by `extract_change_proposal_data()` or `import_artifact()` +- **AND** adapter-specific convenience fields may be added without discarding the native structure +- **AND** contract tests cover the `fetch_backlog_item()` to `import_artifact()` round trip for supported adapters + +#### Scenario: Imported proposal IDs normalize title-first across adapters + +- **GIVEN** an imported backlog artifact has no embedded OpenSpec change ID metadata +- **AND** the source artifact has a usable human-readable title +- **WHEN** the adapter or shared backlog import path constructs the proposal change ID +- **THEN** the change ID is derived from a normalized title slug +- **AND** a numeric provider ID is used only as source tracking metadata or as a deterministic suffix when needed for uniqueness +- **AND** the system does not default to a numeric-only change name while a usable title is available diff --git a/openspec/changes/bugfix-02-ado-import-payload-slugging/specs/devops-sync/spec.md b/openspec/changes/bugfix-02-ado-import-payload-slugging/specs/devops-sync/spec.md new file mode 100644 index 00000000..634dbae6 --- /dev/null +++ b/openspec/changes/bugfix-02-ado-import-payload-slugging/specs/devops-sync/spec.md @@ -0,0 +1,29 @@ +## MODIFIED Requirements + +### Requirement: Azure DevOps Backlog Sync Support + +The system SHALL support Azure DevOps work items as a backlog adapter in the DevOps sync workflow. + +#### Scenario: Selective ADO import preserves native payload for proposal import + +- **GIVEN** a user runs `specfact project sync bridge --adapter ado --mode bidirectional --backlog-ids 123456` +- **WHEN** bridge sync fetches that single ADO work item for import as an OpenSpec change proposal +- **THEN** the adapter returns the provider-native work item payload with a populated `fields` object +- **AND** the payload may include convenience keys such as `title`, `state`, or `description` without removing the native `fields` structure +- **AND** proposal import does not fail for a valid work item with `ADO work item must have fields` + +#### Scenario: Selective ADO import derives a human-readable change ID when metadata is absent + +- **GIVEN** an imported ADO work item has no existing OpenSpec change ID embedded in its description or comments +- **AND** the work item title is `Selective import keeps ADO payload` +- **WHEN** the adapter generates the OpenSpec change proposal during import +- **THEN** the resulting change ID is derived from the title as kebab-case +- **AND** the work item numeric ID remains in source tracking metadata instead of becoming the entire change name + +#### Scenario: Duplicate title slug appends deterministic source suffix + +- **GIVEN** a title-derived slug already exists in `openspec/changes/` +- **AND** another imported ADO work item with ID `123456` resolves to the same title slug +- **WHEN** the second proposal is created +- **THEN** the final change ID keeps the readable title slug and appends a deterministic suffix such as `-123456` +- **AND** the system does not fall back to using only the raw numeric work item ID as the change name diff --git a/openspec/changes/bugfix-02-ado-import-payload-slugging/tasks.md b/openspec/changes/bugfix-02-ado-import-payload-slugging/tasks.md new file mode 100644 index 00000000..970f1fb9 --- /dev/null +++ b/openspec/changes/bugfix-02-ado-import-payload-slugging/tasks.md @@ -0,0 +1,61 @@ +## 0. GitHub sync + +- [x] 0.1 Export or sync this change to a `[Change]` GitHub issue in `nold-ai/specfact-cli` +- [x] 0.2 Update `proposal.md` Source Tracking with the synced issue number, URL, repository, and status +- [x] 0.3 Ensure the synced change issue has labels `enhancement`, `openspec`, and `change-proposal` +- [x] 0.4 Add the synced change issue to the `SpecFact CLI` GitHub project +- [x] 0.5 Link the synced change issue under parent feature `#357` (epic lineage `#186`) +- [x] 0.6 Record the relation back to originating bug `#425` + +## 1. Branch and baseline + +- [x] 1.1 Create worktree: `scripts/worktree.sh create bugfix/bugfix-02-ado-import-payload-slugging` +- [x] 1.2 Bootstrap Hatch in the worktree: `hatch env create` +- [x] 1.3 Reproduce the current selective ADO import failure and record the failing command/output in `openspec/changes/bugfix-02-ado-import-payload-slugging/TDD_EVIDENCE.md` +- [x] 1.4 Audit adjacent import paths that use `fetch_backlog_item()`, `extract_change_proposal_data()`, or `import_backlog_item_as_proposal()` and record the scoped call sites before code changes + +## 2. Write failing tests from spec scenarios + +- [x] 2.1 Add unit tests in `tests/unit/adapters/test_ado.py` proving selective `fetch_backlog_item()` preserves the native ADO payload with populated `fields` +- [x] 2.2 Add unit tests in `tests/unit/adapters/test_ado.py` proving imported ADO change IDs derive from title slugs when no OpenSpec metadata exists +- [x] 2.3 Add collision tests proving duplicate title slugs append a deterministic source-ID suffix instead of degrading to numeric-only names +- [x] 2.4 Add bridge/import contract tests in the selective import path (`tests/unit/specfact_cli/sync/` or adjacent bridge tests) proving `fetch_backlog_item()` output remains valid input for proposal import +- [x] 2.5 Add or extend audit coverage for similar adapter commands so nearby `fetch_backlog_item()` implementations are checked for the same contract assumption +- [x] 2.6 Run targeted tests and capture the failing results in `TDD_EVIDENCE.md` + +## 3. Implement payload preservation and title-first slugging + +- [x] 3.1 Update `src/specfact_cli/adapters/ado.py` so selective ADO fetch returns the native work item payload while preserving compatibility summary keys +- [x] 3.2 Add shared title-first change-ID normalization in `src/specfact_cli/adapters/backlog_base.py` or a nearby shared helper used by proposal import +- [x] 3.3 Update ADO proposal extraction/import flow to use the shared normalizer and keep numeric source IDs in source tracking instead of primary naming +- [x] 3.4 Patch any adjacent adapter or bridge call sites found in the audit where the same summary-vs-native payload mistake or numeric-only fallback can occur +- [x] 3.5 Improve diagnostics or guard rails so missing native payload structure is reported clearly if an adapter violates the import contract in future + +## 4. Verify and evidence + +- [x] 4.1 Re-run the targeted adapter and bridge tests; record passing results in `TDD_EVIDENCE.md` +- [x] 4.2 Run `hatch run format` +- [x] 4.3 Run `hatch run type-check` +- [ ] 4.4 Run `hatch run lint` +- [x] 4.5 Run `hatch run yaml-lint` +- [x] 4.6 Run `hatch run contract-test` +- [x] 4.7 Run `hatch run smart-test` + +## 5. Documentation research and update + +- [x] 5.1 Review affected docs in `docs/`, `README.md`, and command references for selective bridge import and ADO adapter behavior +- [x] 5.2 Update the relevant ADO sync/import documentation to describe the corrected selective import behavior and title-based change-ID fallback +- [ ] 5.3 If new or moved docs are required, verify front matter and sidebar navigation entries in `docs/_layouts/default.html` + +## 6. Module signing, version, and changelog + +- [ ] 6.1 Run `hatch run ./scripts/verify-modules-signature.py --require-signature` +- [ ] 6.2 If any module manifests changed, bump module versions and re-sign before re-running verification +- [ ] 6.3 Bump patch version across the required version files +- [ ] 6.4 Add a `CHANGELOG.md` entry for the bugfix release describing the ADO import contract fix and title-based slugging correction + +## 7. PR and cleanup + +- [ ] 7.1 Open a PR from `bugfix/bugfix-02-ado-import-payload-slugging` to `dev` +- [ ] 7.2 Ensure CI passes and the PR links both the synced change issue and bug `#425` +- [ ] 7.3 After merge, remove the worktree and delete the local branch diff --git a/openspec/changes/code-review-zero-findings/.openspec.yaml b/openspec/changes/code-review-zero-findings/.openspec.yaml new file mode 100644 index 00000000..3c861dd5 --- /dev/null +++ b/openspec/changes/code-review-zero-findings/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-18 diff --git a/openspec/changes/code-review-zero-findings/CHANGE_VALIDATION.md b/openspec/changes/code-review-zero-findings/CHANGE_VALIDATION.md new file mode 100644 index 00000000..f6e17430 --- /dev/null +++ b/openspec/changes/code-review-zero-findings/CHANGE_VALIDATION.md @@ -0,0 +1,120 @@ +# Change Validation Report: code-review-zero-findings + +**Validation Date**: 2026-03-18T19:15:00Z +**Change Proposal**: [proposal.md](./proposal.md) +**Validation Method**: Dry-run simulation in temporary workspace `/tmp/specfact-validation-code-review-zero-findings-1773862184` + +--- + +## Executive Summary + +- Breaking Changes: 0 detected +- Dependent Files: 261 affected (internal only β€” no public API surface) +- Impact Level: Low (internal quality improvement; no external interface changes) +- Validation Result: **Pass** +- User Decision: N/A (no breaking changes) + +--- + +## Breaking Changes Detected + +None. This change is a pure internal quality improvement: + +- Type annotations are additive (widening) β€” no callers are broken. +- `@require`/`@ensure` additions may surface latent bugs where invalid data is passed to currently-uncontracted functions β€” this is intentional and desirable. No caller of a correctly-written function will break. +- Complexity refactoring extracts private helpers only (`_`-prefixed) β€” no public interface changes. +- `print()` β†’ logging substitution affects diagnostic output routing only; no function signatures change. +- CI gate addition affects the PR merge workflow, not code interfaces. + +--- + +## Dependencies Affected + +### Critical Updates Required +None. + +### Recommended Updates +- `sync/bridge_sync.py` (205 findings): Type annotation work may require callers that rely on implicit `Any` typing to be updated if they have type-checking enabled. All callers are internal (`src/specfact_cli/`). +- `adapters/ado.py`, `adapters/github.py`: Same as above β€” internal callers only. + +### Optional +- All other 258 files: No interface changes; only internal annotation/decorator additions. + +--- + +## Impact Assessment + +- **Code Impact**: 261 files across `src/specfact_cli/`, `scripts/`, `tools/`. All changes internal. No public CLI command, option, or output format changes. +- **Test Impact**: New test file `tests/unit/specfact_cli/test_dogfood_self_review.py` added. Existing tests unaffected by interface changes. Complexity refactoring requires regression test runs after each file. +- **Documentation Impact**: `docs/` (code review guide, CI reference page) requires updates to document the self-review CI gate and zero-finding policy. Covered by tasks 9.1–9.4. +- **Release Impact**: Patch (bugfix branch β†’ patch version increment). + +--- + +## Format Validation + +### proposal.md Format: **Pass** (after fixes applied) +- βœ… Title: `# Change: Zero-finding code review β€” dogfooding specfact review on specfact-cli` +- βœ… `## Why` section present +- βœ… `## What Changes` section with NEW/EXTEND/MODIFY markers +- βœ… `## Capabilities` section with new (`dogfood-self-review`) and modified capabilities +- βœ… `## Impact` section present +- βœ… `## Source Tracking` section present (GitHub issue TBD, task 0.1) + +**Issues fixed**: Added title header; added NEW/MODIFY markers to What Changes bullets; added Source Tracking section; added documentation impact to Impact section. + +### tasks.md Format: **Pass** (after fixes applied) +- βœ… Hierarchical numbered sections (`## 0.` through `## 12.`) +- βœ… All tasks use `- [ ] X.Y Description` format +- βœ… Worktree creation first (task 1.1) +- βœ… Failing tests before implementation (Section 2) +- βœ… TDD evidence tasks (1.3, 2.6, 7.3) +- βœ… Quality gates (7.1, 7.4) +- βœ… Module signing quality gate (Section 10) +- βœ… Version and changelog bump (Section 11) +- βœ… GitHub issue creation (Section 0) +- βœ… Documentation research task (Section 9) +- βœ… PR creation last (12.2) +- βœ… Worktree cleanup after merge (12.3) + +**Issues fixed**: Added GitHub issue creation (Section 0); added documentation research section (9); added module signing quality gate (10); added version/changelog task (11); added worktree cleanup task (12.3); noted CHANGE_ORDER already done (12.1). + +### specs Format: **Pass** +- βœ… New capability `dogfood-self-review` has spec file at `specs/dogfood-self-review/spec.md` +- βœ… All modified capabilities (`code-review-module`, `debug-logging`, `review-cli-contracts`) have delta spec files +- βœ… All scenarios use `####` (4 hashtags) as required +- βœ… ADDED Requirements format used throughout +- βœ… WHEN/THEN (and GIVEN/WHEN/THEN) format in scenarios +- βœ… Each requirement has at least one scenario +- Note: `contract-runner` listed as modified capability in proposal but no delta spec created β€” justified because no spec-level behavior changes (coverage expansion only; existing spec already covers the behavior). + +### config.yaml Compliance: **Pass** +- βœ… SDD+TDD order enforced in tasks +- βœ… Contract decorator tasks included (Sections 5, 10) +- βœ… Offline-first validation scenarios in specs (review runs locally, no cloud dependency) +- βœ… Multi-repository compatibility not impacted (change is internal) + +--- + +## OpenSpec Validation + +- **Status**: Pass +- **Command**: `openspec validate code-review-zero-findings --strict` +- **Issues Found/Fixed**: 0 (validation passed both pre- and post-format-fix) + +--- + +## Simulation Workspace + +- Temporary workspace: `/tmp/specfact-validation-code-review-zero-findings-1773862184` +- Interface scaffolds examined: `bridge_sync.py` public methods (8 methods), `adapters/ado.py` (25 public functions) +- All examined interfaces are annotation-only changes β€” no signature modifications +- Breaking change scan: **0 breaking changes** + +--- + +## Decision: Safe to Implement + +This change is safe to implement immediately. No breaking changes, no external dependency updates, no API surface modifications. The phased implementation order (type safety β†’ logging β†’ contracts β†’ complexity β†’ toolchain β†’ CI gate) defined in `design.md` is the recommended implementation sequence. + +**Next step**: Run `/opsx:apply` or `openspec apply code-review-zero-findings` to start working through the tasks. diff --git a/openspec/changes/code-review-zero-findings/design.md b/openspec/changes/code-review-zero-findings/design.md new file mode 100644 index 00000000..1c8e75db --- /dev/null +++ b/openspec/changes/code-review-zero-findings/design.md @@ -0,0 +1,101 @@ +## Context + +Running `specfact review` against the specfact-cli repo (run `review-eb238209`, 2026-03-18) produced 2,539 findings and `overall_verdict: FAIL` with score 0 and reward_delta -80. The four tool categories are basedpyright (type safety, 1,616), semgrep (architecture, 352), contract_runner (contracts, 291), and radon (complexity, 279). A single pylint invocation error rounds it out. + +This is a dogfooding gap: we cannot credibly ask customers to achieve zero-finding reviews while our own codebase fails. The fix is a structured, tool-by-tool remediation campaign, run on a dedicated `bugfix/code-review-zero-findings` branch with TDD evidence captured per behavior change. + +No new user-facing CLI commands or API surface changes are introduced. This is a pure internal quality improvement. + +## Goals / Non-Goals + +**Goals:** +- Reduce basedpyright `reportUnknownMemberType` count from 1,531 to 0 by adding explicit type annotations to all untyped class members. +- Eliminate all 352 `print-in-src` semgrep findings by replacing every `print()` call with `get_bridge_logger()` or `get_logger()`. +- Add `@require` / `@ensure` / `@beartype` decorators to all 291 public functions flagged as `MISSING_ICONTRACT`. +- Refactor all 202 functions with CCβ‰₯16 to CC<16 (target CC<10 for new helpers, CC<13 for refactored legacy). +- Fix the 6 `get-modify-same-method` semgrep findings. +- Resolve the pylint invocation error. +- Establish a CI gate: `specfact review` must exit 0 on every PR targeting `dev` or `main`. + +**Non-Goals:** +- Changing any user-facing CLI command name, option, or output format. +- Introducing new runtime dependencies. +- Achieving CC=0 for orchestration scripts β€” only bring them below the error threshold (CC<16). +- Fixing every warning in third-party generated or vendored files; scope is `src/specfact_cli/`, `scripts/`, and `tools/`. + +## Decisions + +### D1: Phase-by-phase remediation order (type safety β†’ logging β†’ contracts β†’ complexity) + +**Decision**: Address phases in this order: (1) type annotations, (2) printβ†’logging, (3) contracts, (4) complexity, (5) toolchain. + +**Rationale**: Type annotation fixes cascade β€” once `bridge_sync.py` and adapter classes are properly annotated, many downstream `reportAttributeAccessIssue` and `reportUnknownMemberType` findings disappear. Contracts are easier to add once types are correct (beartype's runtime enforcement is meaningless on untyped signatures). Complexity refactoring is last because splitting functions may introduce new public APIs that then need contracts. + +**Alternative considered**: All phases in parallel β€” rejected because untyped code causes false positives in contract and complexity analysis. + +### D2: Type annotation strategy β€” Protocol + TypedDict for dynamic dicts + +**Decision**: Use `TypedDict` for structured dict shapes passed through adapters and sync, and `Protocol` for duck-typed interfaces (e.g., adapter contracts). Avoid `Any` escapes except where third-party SDKs genuinely cannot be typed. + +**Rationale**: `basedpyright` strict mode flags `Any`-typed members as `reportUnknownMemberType`. TypedDict and Protocol produce concrete types that satisfy strict mode without runtime overhead. Using `dict[str, Any]` everywhere just moves the noise downstream. + +**Alternative considered**: Suppression comments (`# type: ignore`) β€” rejected; suppressions hide real bugs and accumulate tech debt. + +### D3: Logging migration β€” use `get_bridge_logger()` uniformly + +**Decision**: Replace all `print()` calls with `get_bridge_logger()` from `specfact_cli.common`. In `scripts/` and `tools/` (which run as standalone scripts), use `logging.getLogger(__name__)` with a StreamHandler if `get_bridge_logger()` is unavailable. + +**Rationale**: Structured logging is already the project standard (CLAUDE.md). Bridge logger routes to the debug log file when `--debug` is active. Script-layer logging uses stdlib so scripts remain standalone without specfact_cli import. + +**Alternative considered**: Rich console print with stderr routing β€” rejected; semgrep rule `print-in-src` fires on any `print()` call regardless of stream. + +### D4: Contract strategy β€” minimal viable contracts, no over-specification + +**Decision**: Add `@require` for preconditions that prevent clearly invalid calls (e.g., non-empty path, non-None model) and `@ensure` only where the return value invariant is architecturally important. Do NOT add contracts that merely restate the type annotation β€” beartype handles that. + +**Rationale**: Over-contracting produces brittle code. The contract_runner rule `MISSING_ICONTRACT` fires on any public function without at least one `@require` or `@ensure`. A minimal, meaningful contract per function satisfies the rule without creating maintenance burden. + +**Alternative considered**: Auto-generate stub contracts β€” rejected; auto-generated `@require(lambda x: x is not None)` for every argument is noise and undermines the contract-first philosophy. + +### D5: Complexity refactoring β€” extract helpers, not classes + +**Decision**: Reduce cyclomatic complexity by extracting named helper functions (not new classes). Helpers live in the same module as the refactored function (private, prefixed `_`). New classes are only introduced if multiple helpers share state. + +**Rationale**: Functions with CCβ‰₯16 are typically orchestration chains (a sequence of `if`/`for`/`try` blocks). Extracting helpers with descriptive names acts as inline documentation and makes each branch independently testable. A new class for what is essentially a procedure introduces unnecessary OOP ceremony. + +**Alternative considered**: Early-return guard clauses only β€” insufficient for CCβ‰₯30 outliers; structural decomposition is required. + +### D6: CI gate implementation β€” `specfact review` in `specfact.yml` + +**Decision**: Add `specfact review` as a blocking step in `.github/workflows/specfact.yml`, running after lint and before build. Exit code must be 0. + +**Rationale**: CI enforcement prevents regressions. Running in `specfact.yml` keeps review-related CI together and avoids a new workflow file. + +**Alternative considered**: Pre-commit hook only β€” insufficient; pre-commit is local and bypassable. + +## Risks / Trade-offs + +- **[Risk] Type annotation effort is large (1,531 occurrences).** β†’ Mitigation: Prioritise the five worst files (bridge_sync.py 205, ado.py 150, github.py 139, harness_generator.py 122, smart_test_coverage.py 157). These 5 files account for ~50% of findings. Fix worst first, re-run review after each file. +- **[Risk] Adding contracts to 291 functions introduces runtime overhead.** β†’ Mitigation: `@icontract` has near-zero overhead for simple lambda preconditions. CrossHair coverage ensures contracts are sound. Contracts that add measurable overhead will be flagged in TDD evidence. +- **[Risk] Complexity refactoring in bridge_sync.py and spec_to_code.py may introduce regressions.** β†’ Mitigation: Strict TDD order β€” failing tests first, refactor, passing tests after. TDD_EVIDENCE.md required per function. +- **[Risk] Some `print()` calls in scripts are intentional progress output to stdout.** β†’ Mitigation: Scripts using `print()` for user-facing progress should use `rich.console.Console()` directly (which is not a `print()` call). The semgrep rule targets the stdlib `print` builtin only. +- **[Risk] Pylint invocation error may surface additional findings once fixed.** β†’ Mitigation: Fix pylint first, triage new findings before counting CI gate status. + +## Migration Plan + +1. Create `bugfix/code-review-zero-findings` branch from `origin/dev`. +2. **Phase 1 (type safety)**: Fix type annotations file-by-file, worst-first. Run `hatch run type-check` after each file. Track TDD evidence. +3. **Phase 2 (logging)**: Bulk-replace `print()` β†’ `get_bridge_logger()` using automated refactor. Review each replacement for intent (progress vs. debug vs. error). Run `hatch run lint` to confirm semgrep clean. +4. **Phase 3 (contracts)**: Add contracts to 291 functions, grouped by module. Run `hatch run contract-test` after each module. +5. **Phase 4 (complexity)**: Refactor CCβ‰₯16 functions. Run `hatch run smart-test` after each file. +6. **Phase 5 (toolchain)**: Fix pylint invocation, verify no new pylint findings. +7. **Phase 6 (CI gate)**: Add `specfact review` step to `specfact.yml`. Run CI and confirm exit 0. +8. Open PR to `dev`. All quality gates must pass. + +**Rollback**: All changes are on a single branch. If a refactor introduces an unexpected regression, `git revert` the specific commit. No database migrations or external state changes. + +## Open Questions + +- OQ1: Should `specfact review` be run in `--changed-only` mode in CI (comparing to `origin/dev`) or full-repo? Full-repo is the gold standard for dogfooding but may be slow. β†’ Decision deferred to CI gate implementation step. +- OQ2: Are there any `print()` calls in `src/` that intentionally write to stdout for machine-readable output (e.g., piped JSON)? These must not be migrated to the logger. β†’ Audit required in Phase 2 before bulk replacement. +- OQ3: The radon CC thresholds used by specfact review (CCβ‰₯16 = error, CC13–15 = warning) β€” are these the same thresholds we recommend to customers? If not, adjust specfact's own thresholds to match. β†’ Review `openspec/specs/clean-code-semgrep-rules/spec.md` before Phase 4. diff --git a/openspec/changes/code-review-zero-findings/proposal.md b/openspec/changes/code-review-zero-findings/proposal.md new file mode 100644 index 00000000..8353cbd8 --- /dev/null +++ b/openspec/changes/code-review-zero-findings/proposal.md @@ -0,0 +1,42 @@ +# Change: Zero-finding code review β€” dogfooding specfact review on specfact-cli + +## Why + +SpecFact CLI's `specfact review` command is our flagship code-quality enforcement tool β€” yet when run against our own repo, it reports 2,539 findings and returns `overall_verdict: FAIL` (score: 0, reward_delta: -80). This is a credibility and leadership gap: we cannot recommend customers adopt a zero-finding standard while our own codebase fails it. Fixing this now closes the dogfooding loop and proves the tool works end-to-end on a real, mature Python CLI project. + +## What Changes + +- **MODIFY β€” Type annotations**: Add explicit type annotations to 1,616 locations flagged by `basedpyright` β€” primarily untyped class member access (`reportUnknownMemberType` Γ— 1,531), attribute access on opaque objects, and `__all__` mismatches. Key files: `sync/bridge_sync.py`, `adapters/ado.py`, `adapters/github.py`, `validators/sidecar/harness_generator.py`. +- **MODIFY β€” Logging migration**: Replace 352 `print()` calls in `src/`, `scripts/`, and `tools/` with `get_bridge_logger()` structured logging. Fix 6 `get-modify-same-method` anti-patterns flagged by semgrep. +- **MODIFY β€” Contract coverage**: Add `@require` / `@ensure` (icontract) and `@beartype` decorators to 291 public functions identified by `contract_runner` as `MISSING_ICONTRACT`. +- **MODIFY β€” Complexity refactoring**: Split 202 functions with cyclomatic complexity CCβ‰₯16 into smaller, testable helpers (target CC<10 for new code, CC<13 for refactored legacy). Reduce an additional 77 functions in the CC13–CC15 warning band. Extreme outliers (CC127, CC120, CC118, CC101) in orchestration entry points must be prioritised. +- **MODIFY β€” Toolchain fix**: Resolve the single `pylint` invocation error (missing binary or Hatch env PATH issue). +- **NEW β€” CI gate**: Add `specfact review run --ci` as a blocking step in `.github/workflows/specfact.yml` (after lint, before build); exit code 0 required on every PR targeting `dev` or `main`. +- **No new user-facing CLI commands or API surface changes.** All changes are internal quality improvements. + +## Capabilities + +### New Capabilities +- `dogfood-self-review`: Specification for running and passing `specfact review` against the specfact-cli repo itself β€” defines the self-review policy, acceptance criteria (0 findings, `overall_verdict: PASS`), and the CI gate that enforces it. + +### Modified Capabilities +- `code-review-module`: The review tool must be able to scan itself; any self-referential edge cases (e.g., reviewing files that implement the reviewer) must be handled. +- `debug-logging`: Logging migration extends the `get_bridge_logger()` contract to cover all `print()` replacement sites, including adapter and scripts layers. +- `contract-runner`: The `MISSING_ICONTRACT` contract must produce actionable output for the 291 currently-uncovered public APIs. No rule changes β€” this is coverage expansion. +- `review-cli-contracts`: Contracts on the review CLI commands must be consistent with the updated type-annotated codebase (no phantom attribute access post-type-annotation). + +## Impact + +- **Files**: 261 unique files affected across `src/specfact_cli/`, `scripts/`, and `tools/`. +- **CI**: After this change, `specfact review` will be added to the CI pre-commit and PR gates as a blocking check (exit code 0 required). +- **No API breakage**: All changes are internal quality improvements with no public interface modifications. +- **Dependencies**: No new runtime dependencies. `icontract`, `beartype`, and `basedpyright` are already in the dev toolchain. +- **Sequencing**: Independent of other pending changes β€” no hard blockers. Can be implemented in parallel worktree alongside module-migration work. +- **Documentation**: `docs/` (code review guide, CI reference page) will be updated to document the self-review CI gate and zero-finding policy. + +## Source Tracking + +- **GitHub Issue**: #423 +- **Issue URL**: https://github.com/nold-ai/specfact-cli/issues/423 +- **Repository**: nold-ai/specfact-cli +- **Last Synced Status**: open diff --git a/openspec/changes/code-review-zero-findings/specs/code-review-module/spec.md b/openspec/changes/code-review-zero-findings/specs/code-review-module/spec.md new file mode 100644 index 00000000..58875ed8 --- /dev/null +++ b/openspec/changes/code-review-zero-findings/specs/code-review-module/spec.md @@ -0,0 +1,27 @@ +## ADDED Requirements + +### Requirement: Self-referential scan β€” review module can scan itself without errors +The `specfact-code-review` module SHALL be able to review its own source files (including the files that implement the reviewer) without infinite loops, false positives from meta-scanning, or unhandled exceptions. + +#### Scenario: Review run on specfact-cli repo completes without tool_error findings +- **WHEN** `specfact review` is run with the specfact-cli repo as the target +- **THEN** no finding with `tool` equal to `code-review-module` or `category` equal to `tool_error` is produced +- **AND** the run exits with code 0 (assuming all other findings are resolved) + +#### Scenario: Tool error finding is surfaced as error severity +- **WHEN** any configured tool fails to invoke (e.g., missing binary) +- **THEN** a finding with `category="tool_error"` and `severity="error"` is produced +- **AND** the finding message includes the tool name and failure reason + +### Requirement: CI gate integration β€” review must be runnable non-interactively +The review module SHALL support a `--ci` or equivalent non-interactive flag that suppresses prompts, writes machine-readable output to `.specfact/code-review.json`, and exits with code 1 on any finding at severity `error` or higher. + +#### Scenario: Non-interactive CI run writes JSON report and exits non-zero on errors +- **WHEN** `specfact review run --ci` is executed and error-severity findings exist +- **THEN** `.specfact/code-review.json` is written with `overall_verdict: "FAIL"` and `ci_exit_code: 1` +- **AND** the process exits with code 1 + +#### Scenario: Non-interactive CI run exits zero on clean codebase +- **WHEN** `specfact review run --ci` is executed and no findings exist +- **THEN** `.specfact/code-review.json` is written with `overall_verdict: "PASS"` and `ci_exit_code: 0` +- **AND** the process exits with code 0 diff --git a/openspec/changes/code-review-zero-findings/specs/debug-logging/spec.md b/openspec/changes/code-review-zero-findings/specs/debug-logging/spec.md new file mode 100644 index 00000000..1579f0fc --- /dev/null +++ b/openspec/changes/code-review-zero-findings/specs/debug-logging/spec.md @@ -0,0 +1,22 @@ +## ADDED Requirements + +### Requirement: Bridge logger used in all production source paths +Every production code path in `src/specfact_cli/` SHALL use `get_bridge_logger()` from `specfact_cli.common` for all diagnostic output, replacing any remaining `print()` builtin calls. + +#### Scenario: Adapter module writes diagnostic output via bridge logger +- **WHEN** an adapter module (e.g., `adapters/ado.py`, `adapters/github.py`) performs a network call or state change +- **THEN** diagnostic messages are written via `logger = get_bridge_logger(__name__)` and `logger.debug(...)` / `logger.info(...)` +- **AND** no `print()` call appears in the adapter module + +#### Scenario: Sync module writes diagnostic output via bridge logger +- **WHEN** `sync/bridge_sync.py` or `sync/spec_to_code.py` processes a file +- **THEN** all progress and error messages are routed through `get_bridge_logger(__name__)` +- **AND** no `print()` call appears in the sync module + +### Requirement: Script-layer logging uses stdlib or Rich, not print() +Scripts in `scripts/` and `tools/` that run as standalone CLI programs SHALL use `logging.getLogger(__name__)` with a `StreamHandler` for progress output, or `rich.console.Console()` for formatted terminal output. The stdlib `print()` builtin SHALL NOT be used. + +#### Scenario: Standalone script writes progress via logging +- **WHEN** a script in `scripts/` needs to write a status message to stdout +- **THEN** it calls `logging.getLogger(__name__).info(...)` or `console.print(...)` from a Rich Console instance +- **AND** semgrep `print-in-src` reports zero findings for that script diff --git a/openspec/changes/code-review-zero-findings/specs/dogfood-self-review/spec.md b/openspec/changes/code-review-zero-findings/specs/dogfood-self-review/spec.md new file mode 100644 index 00000000..e2036fb2 --- /dev/null +++ b/openspec/changes/code-review-zero-findings/specs/dogfood-self-review/spec.md @@ -0,0 +1,81 @@ +## ADDED Requirements + +### Requirement: Self-review policy β€” specfact-cli runs specfact review on itself +The specfact-cli repository SHALL be subject to `specfact review` as a first-class CI gate, enforcing the same zero-finding standard we recommend to customers. + +#### Scenario: Review run on own repo produces zero findings +- **WHEN** `specfact review` is executed against the specfact-cli repository root +- **THEN** `overall_verdict` is `PASS` +- **AND** the findings array is empty +- **AND** the process exits with code 0 + +#### Scenario: Review run failure blocks CI +- **WHEN** `specfact review` exits with code non-zero on a PR targeting `dev` or `main` +- **THEN** the CI pipeline marks the PR check as failed +- **AND** the PR cannot be merged until findings are resolved + +#### Scenario: Review result is machine-readable +- **WHEN** `specfact review --format json` is run in CI +- **THEN** a JSON report is written to `.specfact/code-review.json` +- **AND** the report schema_version is `1.0` +- **AND** `overall_verdict`, `score`, `findings`, and `ci_exit_code` fields are present + +### Requirement: Type-safe codebase β€” zero basedpyright findings in strict mode +All public API class members and function signatures in `src/specfact_cli/` SHALL be explicitly typed so that `basedpyright` strict mode reports zero `reportUnknownMemberType`, `reportAttributeAccessIssue`, and `reportUnsupportedDunderAll` findings. + +#### Scenario: basedpyright strict mode passes on src/ +- **WHEN** `hatch run type-check` is executed +- **THEN** basedpyright reports zero errors and zero warnings for files under `src/specfact_cli/` + +#### Scenario: Untyped class member introduced in PR fails CI +- **WHEN** a PR introduces a class member without a type annotation +- **THEN** `hatch run type-check` exits non-zero +- **AND** CI marks the type-check step as failed + +#### Scenario: TypedDict used for structured dict shapes +- **WHEN** a function accepts or returns a dict with a known schema +- **THEN** a `TypedDict` or Pydantic model is used rather than `dict[str, Any]` +- **AND** basedpyright infers the member types without `reportUnknownMemberType` + +### Requirement: Print-free source β€” all production logging via bridge logger +No `print()` builtin calls SHALL appear in files under `src/specfact_cli/`, `scripts/`, or `tools/`, as detected by the semgrep `print-in-src` rule. + +#### Scenario: Logging call replaces print in adapter layer +- **WHEN** `get_bridge_logger()` is called in an adapter module (e.g., `adapters/ado.py`) +- **THEN** structured log messages are routed to the debug log file when `--debug` is active +- **AND** no `print()` call remains in the file +- **AND** semgrep `print-in-src` reports zero findings for that file + +#### Scenario: Script-layer progress output uses Rich console or stdlib logging +- **WHEN** a script in `scripts/` or `tools/` needs to write progress to stdout +- **THEN** it uses `rich.console.Console().print()` or `logging.getLogger(__name__)`, not the stdlib `print` builtin +- **AND** semgrep `print-in-src` reports zero findings for that file + +### Requirement: Full contract coverage β€” all public APIs carry icontract decorators +Every public function (non-underscore-prefixed) in `src/specfact_cli/` SHALL have at least one `@require` or `@ensure` decorator from icontract, and a `@beartype` decorator for runtime type enforcement. + +#### Scenario: Public function without @require fails contract_runner check +- **WHEN** `contract_runner` scans a file with a public function lacking `@require`/`@ensure` +- **THEN** a `MISSING_ICONTRACT` finding is produced + +#### Scenario: Decorated public function produces no missing-contract finding +- **WHEN** a public function has both `@require` (or `@ensure`) and `@beartype` +- **THEN** `contract_runner` produces zero `MISSING_ICONTRACT` findings for that function + +#### Scenario: Minimal meaningful contract per function +- **WHEN** a `@require` precondition is added to a public function +- **THEN** the precondition checks a domain-meaningful invariant (e.g., path exists, non-empty string, valid enum) +- **AND** the precondition is NOT a trivial `lambda x: x is not None` that merely restates the type + +### Requirement: Complexity budget β€” no function exceeds CC15 +No function in `src/specfact_cli/`, `scripts/`, or `tools/` SHALL have cyclomatic complexity β‰₯16, as measured by radon. + +#### Scenario: High-complexity function split into helpers passes complexity check +- **WHEN** a function with CCβ‰₯16 is refactored into a top-level function and one or more private helpers +- **THEN** `hatch run lint` (radon check) reports no CCβ‰₯16 findings for that function +- **AND** each extracted helper has CC<10 + +#### Scenario: New code written during this change stays below threshold +- **WHEN** any new function is introduced during this change +- **THEN** its cyclomatic complexity is <10 as measured by radon +- **AND** no CCβ‰₯13 warning is raised for the new function diff --git a/openspec/changes/code-review-zero-findings/specs/review-cli-contracts/spec.md b/openspec/changes/code-review-zero-findings/specs/review-cli-contracts/spec.md new file mode 100644 index 00000000..e1cc6430 --- /dev/null +++ b/openspec/changes/code-review-zero-findings/specs/review-cli-contracts/spec.md @@ -0,0 +1,19 @@ +## ADDED Requirements + +### Requirement: Review CLI commands carry icontract and beartype decorators +All public command functions in the review module (`specfact code review run`, `ledger`, `rules`) SHALL have `@require` / `@ensure` decorators (icontract) and `@beartype` on their signatures, consistent with the project-wide contract-first standard. + +#### Scenario: review run command has precondition on repo_path +- **WHEN** `specfact code review run` is invoked with an invalid `repo_path` +- **THEN** an icontract `ViolationError` is raised before any tool runner is invoked +- **AND** the error message references the violated precondition + +#### Scenario: review CLI contracts are consistent with typed signatures +- **WHEN** `hatch run contract-test` is executed after type annotations are applied to the review CLI module +- **THEN** `contract_runner` reports zero `MISSING_ICONTRACT` findings for review command functions +- **AND** `basedpyright` reports zero type errors for the review CLI module + +#### Scenario: Contract validation scenarios cover review run with CI flag +- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml` exists +- **WHEN** a scenario exercising `review run --ci` with a clean target is added +- **THEN** the scenario validates exit code 0 and presence of `.specfact/code-review.json` diff --git a/openspec/changes/code-review-zero-findings/tasks.md b/openspec/changes/code-review-zero-findings/tasks.md new file mode 100644 index 00000000..27eaa80c --- /dev/null +++ b/openspec/changes/code-review-zero-findings/tasks.md @@ -0,0 +1,95 @@ +## 0. GitHub issue + +- [x] 0.1 Create GitHub issue with title `[Change] Zero-finding code review β€” dogfooding specfact review on specfact-cli`, labels `enhancement` and `change-proposal`, body following `.github/ISSUE_TEMPLATE/change_proposal.md` (Why and What Changes sections from proposal), footer `*OpenSpec Change Proposal: code-review-zero-findings*` +- [x] 0.2 Update `proposal.md` Source Tracking section with the new issue number, URL, and status `open` + +## 1. Branch and baseline + +- [x] 1.1 Create worktree: `git worktree add ../specfact-cli-worktrees/bugfix/code-review-zero-findings -b bugfix/code-review-zero-findings origin/dev` +- [x] 1.2 Bootstrap Hatch in worktree: `hatch env create` +- [x] 1.3 Run `specfact review` and capture baseline report to `openspec/changes/code-review-zero-findings/TDD_EVIDENCE.md` (pre-fix run β€” expected FAIL with 2539 findings) +- [x] 1.4 Fix pylint invocation error (install binary or fix PATH in Hatch env) and re-run review to confirm tool_error finding is gone + +## 2. Write failing tests from spec scenarios (TDD step 2) + +- [x] 2.1 Add test `tests/unit/specfact_cli/test_dogfood_self_review.py` β€” assert `specfact review` exits 0 on repo root (expect FAIL before fix) +- [x] 2.2 Add test asserting zero basedpyright `reportUnknownMemberType` findings in `src/` (expect FAIL before fix) +- [x] 2.3 Add test asserting zero semgrep `print-in-src` findings in `src/`, `scripts/`, `tools/` (expect FAIL before fix) +- [x] 2.4 Add test asserting zero `MISSING_ICONTRACT` findings in `src/` (expect FAIL before fix) +- [x] 2.5 Add test asserting no radon CCβ‰₯16 findings in `src/`, `scripts/`, `tools/` (expect FAIL before fix) +- [x] 2.6 Record failing test run results in `TDD_EVIDENCE.md` + +## 3. Phase 1 β€” Type annotations (basedpyright, 1,616 findings) + +- [ ] 3.1 Add type annotations to `sync/bridge_sync.py` (205 findings) β€” use `TypedDict` for dict shapes, `Protocol` for duck-typed interfaces; run `hatch run type-check` after +- [ ] 3.2 Add type annotations to `tools/smart_test_coverage.py` (157 findings) β€” run `hatch run type-check` after +- [ ] 3.3 Add type annotations to `adapters/ado.py` (150 findings) β€” run `hatch run type-check` after +- [ ] 3.4 Add type annotations to `adapters/github.py` (139 findings) β€” run `hatch run type-check` after +- [ ] 3.5 Add type annotations to `validators/sidecar/harness_generator.py` (122 findings) β€” run `hatch run type-check` after +- [ ] 3.6 Fix `reportUnsupportedDunderAll` findings (17): correct `__all__` export lists in affected modules +- [ ] 3.7 Fix remaining `reportAttributeAccessIssue`, `reportInvalidTypeForm`, `reportOptionalMemberAccess`, and `reportCallIssue` findings across all other files +- [ ] 3.8 Run `hatch run type-check` β€” confirm 0 basedpyright errors and warnings + +## 4. Phase 2 β€” Logging migration (semgrep, 352 + 6 findings) + +- [ ] 4.1 Audit `print()` calls in `src/specfact_cli/` to classify: debug/info (β†’ bridge logger) vs. intentional stdout (β†’ Rich Console) +- [ ] 4.2 Replace all `print()` calls in `src/specfact_cli/` with `get_bridge_logger(__name__)` calls; confirm no unintended output routing change +- [ ] 4.3 Replace all `print()` calls in `scripts/` with `logging.getLogger(__name__)` or `rich.console.Console().print()` +- [ ] 4.4 Replace all `print()` calls in `tools/` with `logging.getLogger(__name__)` or `rich.console.Console().print()` +- [ ] 4.5 Fix 6 `get-modify-same-method` semgrep findings β€” separate getter and modifier responsibilities +- [ ] 4.6 Run `hatch run lint` β€” confirm 0 semgrep architecture findings + +## 5. Phase 3 β€” Contract coverage (contract_runner, 291 findings) + +- [ ] 5.1 Add `@require` / `@ensure` / `@beartype` to all public functions in `src/specfact_cli/sync/` flagged by contract_runner +- [ ] 5.2 Add contracts to all public functions in `src/specfact_cli/adapters/` flagged by contract_runner +- [ ] 5.3 Add contracts to all public functions in `src/specfact_cli/validators/` flagged by contract_runner +- [ ] 5.4 Add contracts to all public functions in `src/specfact_cli/generators/` flagged by contract_runner +- [ ] 5.5 Add contracts to all remaining public functions in `src/specfact_cli/` flagged by contract_runner +- [ ] 5.6 Ensure all review CLI command functions (`code review run`, `ledger`, `rules`) have contracts (per `review-cli-contracts` spec) +- [ ] 5.7 Run `hatch run contract-test` β€” confirm 0 `MISSING_ICONTRACT` findings + +## 6. Phase 4 β€” Complexity refactoring (radon, 279 findings) + +- [ ] 6.1 Refactor functions with CCβ‰₯30 in `sync/bridge_sync.py` β€” extract private helpers; run `hatch run smart-test` after +- [ ] 6.2 Refactor functions with CCβ‰₯30 in `sync/spec_to_code.py` β€” extract private helpers; run `hatch run smart-test` after +- [ ] 6.3 Refactor functions with CCβ‰₯20 in `scripts/publish-module.py` (`publish_bundle()` and `main()`) β€” extract step functions +- [ ] 6.4 Refactor all remaining functions with CCβ‰₯16 across `src/`, `scripts/`, `tools/` β€” working through radon error-band findings systematically +- [ ] 6.5 Reduce CC13–15 warning-band functions where refactoring is safe and straightforward (target CC<13) +- [ ] 6.6 Run `hatch run lint` (radon check) β€” confirm 0 CCβ‰₯16 error findings, and 0 CCβ‰₯13 warnings + +## 7. Verify and evidence + +- [ ] 7.1 Run full quality gate: `hatch run format && hatch run type-check && hatch run lint && hatch run contract-test && hatch run smart-test` +- [ ] 7.2 Run `specfact review` β€” confirm `overall_verdict: PASS` and 0 findings +- [ ] 7.3 Record passing test and review run in `TDD_EVIDENCE.md` (post-fix run) +- [ ] 7.4 Run `hatch test --cover -v` β€” confirm no regressions + +## 8. CI gate integration + +- [ ] 8.1 Add `specfact review run --ci` as a blocking step in `.github/workflows/specfact.yml` (after lint, before build) +- [ ] 8.2 Confirm CI passes on the PR branch with the new gate active + +## 9. Documentation research and update + +- [ ] 9.1 Identify all affected docs: check `docs/` (reference, guides, CI, code-review), `README.md`, `docs/index.md` +- [ ] 9.2 Add or update a section in the code review guide (`docs/`) documenting the self-review CI gate and zero-finding policy +- [ ] 9.3 Add CI reference entry for `specfact review run --ci` gate in the CI reference page +- [ ] 9.4 Verify front-matter (layout, title, permalink, description) on any new or modified doc pages; update `docs/_layouts/default.html` sidebar if a new page is added + +## 10. Module signing quality gate + +- [ ] 10.1 Run `hatch run ./scripts/verify-modules-signature.py --require-signature`; if any module manifest changed, re-sign with `hatch run python scripts/sign-modules.py --key-file ` +- [ ] 10.2 Bump module version for any changed module (patch increment) before re-signing +- [ ] 10.3 Re-run verification until fully green: `hatch run ./scripts/verify-modules-signature.py --require-signature` + +## 11. Version and changelog + +- [ ] 11.1 Bump patch version (bugfix branch): sync across `pyproject.toml`, `setup.py`, `src/specfact_cli/__init__.py` +- [ ] 11.2 Add `CHANGELOG.md` entry under a new `[X.Y.Z] - 2026-MM-DD` section with `Fixed` (pylint invocation, type annotations, printβ†’logging) and `Changed` (contract coverage, complexity refactoring, CI gate) + +## 12. PR and cleanup + +- [ ] 12.1 Note: `openspec/CHANGE_ORDER.md` entry for `code-review-zero-findings` already added during change creation β€” verify it is present +- [ ] 12.2 Open PR from `bugfix/code-review-zero-findings` to `dev`; ensure all CI checks pass (including the new `specfact review` gate) +- [ ] 12.3 After PR is merged to `dev`: run `git worktree remove ../specfact-cli-worktrees/bugfix/code-review-zero-findings && git branch -d bugfix/code-review-zero-findings && git worktree prune` diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/.openspec.yaml b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/.openspec.yaml new file mode 100644 index 00000000..d8b0ed03 --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-20 diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/CHANGE_VALIDATION.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/CHANGE_VALIDATION.md new file mode 100644 index 00000000..8285e27b --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/CHANGE_VALIDATION.md @@ -0,0 +1,5 @@ +# Change Validation + +- Timestamp: 2026-03-20T14:42:35+01:00 +- Command: `openspec validate docs-04-docs-review-gate-and-link-integrity --strict` +- Result: `Change 'docs-04-docs-review-gate-and-link-integrity' is valid` diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/TDD_EVIDENCE.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/TDD_EVIDENCE.md new file mode 100644 index 00000000..57c11211 --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/TDD_EVIDENCE.md @@ -0,0 +1,25 @@ +# TDD Evidence + +## Pre-Implementation Failing Run + +- Timestamp: 2026-03-20T14:31:44+01:00 +- Command: `hatch run pytest tests/unit/docs/test_release_docs_parity.py -q` +- Result: failed + +### Failure Summary + +- `test_navigation_links_resolve_to_published_docs_routes` failed because the first validator pass still treated Jekyll `{{ ... | relative_url }}` expressions and non-page assets such as `feed.xml` and `assets/main.css` as literal docs routes. +- `test_authored_internal_docs_links_resolve_to_published_docs_targets` failed because authored docs still mixed true published-route drift with links to non-published repo files and missing published targets. +- The failing run surfaced the underlying docs regressions that motivated this change, including sidebar routes such as `/reference/directory-structure/`, `/reference/architecture/`, and multiple `/guides/...` links that did not match the authored page permalinks. + +## Post-Implementation Passing Run + +- Timestamp: 2026-03-20T14:40:50+01:00 +- Command: `hatch run pytest tests/unit/docs/test_release_docs_parity.py -q` +- Result: passed + +### Verification Summary + +- The route-aware docs parity suite passed after normalizing guide/reference/example permalinks to the published section routes used by sidebar navigation. +- Broken authored links to missing repo-local files were replaced with valid published docs targets or GitHub source links where the target is intentionally not a published docs page. +- The dedicated `Docs Review` workflow now provides a fast mandatory-check path for docs-only changes without waiting for the full code-oriented PR orchestrator. diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/design.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/design.md new file mode 100644 index 00000000..6830659a --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/design.md @@ -0,0 +1,66 @@ +## Context + +The docs site is published with Jekyll front matter and explicit permalinks, but authored links currently mix file-relative paths, assumed `/reference/...` routes, and page-level permalinks that do not always align. The existing docs parity tests check a few content invariants plus a minimal front-matter presence check, while the PR orchestrator treats docs-only and Markdown-only changes as ignorable. That combination allows broken published routes to ship even when the authored source page still exists. + +The change needs both content repair and an enforcement path. The enforcement must stay lightweight enough for PRs, use repository-local information only, and be clear when a failure is caused by a missing page, a wrong permalink, or incomplete front matter. + +## Goals / Non-Goals + +**Goals:** + +- Detect broken internal docs links before merge. +- Detect navigation or landing-page links that point at unpublished routes. +- Require complete Jekyll front matter for pages that are part of published docs navigation and authored-link targets. +- Ensure docs-only PRs trigger a dedicated docs review workflow without waiting for the full code-oriented pipeline. + +**Non-Goals:** + +- Building the full Jekyll site in PR orchestration. +- Validating external websites beyond basic scheme/host filtering. +- Enforcing that every Markdown page in `docs/` appears in global navigation; only navigation-owned and authored-link targets are in scope. + +## Decisions + +### Decision: Reuse the existing docs parity test module as the docs review engine + +Extend `tests/unit/docs/test_release_docs_parity.py` with helpers that parse front matter, derive published routes, and validate internal authored links. This keeps the logic in the current docs-quality test surface, makes failures readable in pytest output, and avoids inventing a separate one-off script and assertion format. + +Alternative considered: a standalone Python script under `scripts/`. Rejected because it would duplicate test harness behavior and add one more place to maintain path and parsing logic. + +### Decision: Treat published-route resolution as the source of truth, not file paths + +The validator should compute the set of valid published routes from each docs page's explicit permalink or the site default, then compare authored links against that route map. This directly models the `docs.specfact.io` behavior and catches cases where a file exists but its published URL differs from the navigation link. + +Alternative considered: checking only that a target Markdown file exists on disk. Rejected because it misses the current failure mode where the file exists but the published URL is different. + +### Decision: Scope mandatory metadata checks to published docs pages and navigation-linked targets + +Require `layout`, `title`, and `permalink` for published docs pages that are reachable from docs navigation or internal authored links, and require a valid front-matter block for all other authored-link targets. This raises the bar where missing metadata hurts users while avoiding an unnecessarily broad rule for internal planning or auxiliary Markdown. + +Alternative considered: requiring all docs Markdown files to have the full metadata set. Rejected because some auxiliary files are intentionally not part of the published navigation experience. + +### Decision: Add a dedicated docs-review workflow for docs-only changes + +Add a separate `.github/workflows/docs-review.yml` workflow that runs the targeted docs parity suite for docs and Markdown changes. Keep the heavier PR orchestrator focused on code-oriented validation so docs-only changes get a fast required check without waking the full runtime test matrix. + +Alternative considered: adding a `docs-review` job inside `.github/workflows/pr-orchestrator.yml`. Rejected because docs-only PRs would still wait on the code-oriented orchestration workflow and the user explicitly wants a lighter mandatory check. + +## Risks / Trade-offs + +- [False positives from flexible Markdown links] β†’ Limit validation to internal site-style links and normalize common relative-link forms before asserting. +- [Metadata requirements may surface many latent docs issues] β†’ Start with navigation-owned and authored-link targets, then fix the discovered set in the same change. +- [Workflow duration increases for docs-only PRs] β†’ Run only the targeted docs parity test file in the docs-review job. + +## Migration Plan + +1. Add the OpenSpec deltas and implement the stricter docs review tests. +2. Run the targeted docs parity suite and capture the failing evidence before fixing routes or workflow logic. +3. Correct broken docs permalinks and any missing navigation-linked metadata. +4. Add the dedicated `Docs Review` workflow for docs and Markdown changes. +5. Re-run the targeted docs parity suite and the affected workflow validation gates, then record passing evidence. + +Rollback strategy: revert the docs route corrections and dedicated docs-review workflow as a single change if the new validator proves too noisy; the repository will return to the prior lax behavior. + +## Open Questions + +- None at this stage; the implementation approach is straightforward and bounded to docs/test/workflow files. diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/proposal.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/proposal.md new file mode 100644 index 00000000..b4f920db --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/proposal.md @@ -0,0 +1,37 @@ +# Change: Repair Published Docs Links And Add A Docs Review Gate + +## Why + +Published docs currently contain broken links because authored navigation and landing-page links do not consistently match the pages' actual Jekyll permalinks. The PR orchestrator also ignores docs-only and Markdown-only changes, so link drift, missing front matter, and missing docs coverage can ship without any review gate. + +## What Changes + +- Audit the current published-doc navigation and landing-page links, then correct broken routes and any missing linked docs coverage in core docs. +- Add automated docs review validation that checks Jekyll front matter, published-route resolution, and authored internal links for docs pages. +- Add a dedicated docs review workflow so docs-only changes run fast validation without waiting for the full code-oriented PR orchestrator. +- Record and enforce a discoverability contract for navigation-owned docs pages so broken or missing linked pages fail fast in CI. + +## Capabilities + +### New Capabilities + +- `docs-review-gate`: validate published docs links, front matter, and navigation-owned page coverage during local and CI docs review. + +### Modified Capabilities + +- `documentation-alignment`: docs landing pages, sidebar links, and authored internal links must resolve to the actual published permalinks for the current docs site. + +## Impact + +- Affected docs: `docs/index.md`, `docs/_layouts/default.html`, `docs/reference/directory-structure.md`, and any additional docs pages found during link audit. +- Affected validation: `tests/unit/docs/test_release_docs_parity.py` and any new docs-review helpers or fixtures needed for route/link validation. +- Affected CI: `.github/workflows/docs-review.yml` provides the dedicated docs-only validation path and can be configured as a required GitHub check. +- User-facing impact: linked pages on `https://docs.specfact.io` resolve correctly, and future docs regressions fail in PRs before merge. + +## Source Tracking + + +- **GitHub Issue**: pending +- **Issue URL**: pending +- **Last Synced Status**: local-proposal +- **Sanitized**: true diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/specs/docs-review-gate/spec.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/specs/docs-review-gate/spec.md new file mode 100644 index 00000000..180f8c0e --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/specs/docs-review-gate/spec.md @@ -0,0 +1,37 @@ +## ADDED Requirements + +### Requirement: Docs review validates published route integrity + +The docs review gate SHALL derive the published route for authored docs pages from Jekyll front matter and site defaults, and SHALL fail when an internal docs link points to a route that is not published by the current docs source tree. + +#### Scenario: Sidebar or landing page links point to an unpublished route + +- **WHEN** the docs review gate evaluates links from `docs/index.md` or `docs/_layouts/default.html` +- **THEN** every internal docs route resolves to exactly one published docs page +- **AND** the failure output identifies the authored source and the unresolved route when a link is broken + +#### Scenario: Authored markdown links drift from the published permalink + +- **WHEN** the docs review gate evaluates internal Markdown links inside published docs pages +- **THEN** links to docs pages resolve by published route rather than only by source-file existence +- **AND** a page with a mismatched permalink fails validation even if the Markdown file still exists on disk + +### Requirement: Docs review validates required front matter for published docs targets + +The docs review gate SHALL fail when a published docs page that is linked from navigation or another authored docs page is missing required front matter fields needed for publishing and navigation: `layout`, `title`, and `permalink`. + +#### Scenario: Linked docs page is missing required metadata + +- **WHEN** the docs review gate evaluates a navigation-linked or authored-link target page +- **THEN** the page must declare `layout`, `title`, and `permalink` in front matter +- **AND** the failure output identifies the page and the missing keys + +### Requirement: Docs-only pull requests run a dedicated docs review workflow + +A dedicated docs review workflow SHALL run the docs review gate for pull requests or pushes that change docs or Markdown content, even when no Python source files changed. + +#### Scenario: Docs-only change triggers docs review validation + +- **WHEN** a pull request changes only `docs/**` or Markdown files +- **THEN** the dedicated docs review workflow runs the targeted docs review suite +- **AND** docs validation does not wait for the full code-oriented PR orchestrator to complete diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/specs/documentation-alignment/spec.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/specs/documentation-alignment/spec.md new file mode 100644 index 00000000..70349612 --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/specs/documentation-alignment/spec.md @@ -0,0 +1,21 @@ +## ADDED Requirements + +### Requirement: Navigation-owned docs links match published permalinks + +The docs landing page and sidebar navigation SHALL link to the actual published permalinks for their target pages, and SHALL NOT assume a section-prefixed route when the page publishes elsewhere. + +#### Scenario: Reader opens a navigation-linked reference page + +- **WHEN** a reader selects a reference or guide link from `docs/index.md` or `docs/_layouts/default.html` +- **THEN** the route resolves on `docs.specfact.io` +- **AND** the link target matches the page permalink declared in the authored docs source + +### Requirement: Broken published docs routes are corrected in authored source + +When docs review identifies a broken published route caused by authored permalink drift, the authored page or link source SHALL be corrected in the same remediation change so the published docs site remains internally consistent. + +#### Scenario: Existing docs page has a mismatched permalink + +- **WHEN** an authored docs page exists but the linked published route does not resolve because the page permalink differs +- **THEN** the remediation updates the authored permalink or the authored link source to restore route integrity +- **AND** the corrected route remains covered by docs review validation diff --git a/openspec/changes/docs-04-docs-review-gate-and-link-integrity/tasks.md b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/tasks.md new file mode 100644 index 00000000..1087e7fb --- /dev/null +++ b/openspec/changes/docs-04-docs-review-gate-and-link-integrity/tasks.md @@ -0,0 +1,21 @@ +## 1. Change Setup And Spec Deltas + +- [x] 1.1 Update `openspec/CHANGE_ORDER.md` with the new `docs-04-docs-review-gate-and-link-integrity` entry +- [x] 1.2 Add the `docs-review-gate` capability spec for published-route, front-matter, and docs-only CI validation +- [x] 1.3 Add the `documentation-alignment` delta covering navigation-owned permalink integrity and same-change remediation of broken routes + +## 2. Validation First + +- [x] 2.1 Extend `tests/unit/docs/test_release_docs_parity.py` to validate internal docs routes, linked-page metadata, and navigation-owned links +- [x] 2.2 Run the targeted docs parity suite and capture a failing result in `openspec/changes/docs-04-docs-review-gate-and-link-integrity/TDD_EVIDENCE.md` + +## 3. Remediation And Workflow Enforcement + +- [x] 3.1 Fix broken docs permalinks and any linked-page front-matter gaps discovered by the new validation +- [x] 3.2 Add `.github/workflows/docs-review.yml` so docs-only and Markdown-only changes run a dedicated docs-review workflow + +## 4. Verification And Delivery + +- [x] 4.1 Re-run the targeted docs parity suite and record the passing result in `openspec/changes/docs-04-docs-review-gate-and-link-integrity/TDD_EVIDENCE.md` +- [x] 4.2 Run `openspec validate docs-04-docs-review-gate-and-link-integrity --strict` and save the result to `CHANGE_VALIDATION.md` +- [x] 4.3 Run the affected repo quality gates for touched docs/test/workflow files diff --git a/src/specfact_cli/adapters/ado.py b/src/specfact_cli/adapters/ado.py index 5fbfeae6..adafab50 100644 --- a/src/specfact_cli/adapters/ado.py +++ b/src/specfact_cli/adapters/ado.py @@ -426,7 +426,7 @@ def extract_change_proposal_data(self, item_data: dict[str, Any]) -> dict[str, A Change ID extraction priority: 1. Description footer (legacy format): *OpenSpec Change Proposal: `id`* 2. Comments (new format): **Change ID**: `id` in OpenSpec Change Proposal Reference comment - 3. Work item ID (fallback) + 3. Work item ID (fallback, normalized during shared proposal import) """ if not isinstance(item_data, dict): msg = "ADO work item data must be dict" @@ -689,7 +689,15 @@ def import_artifact( pass # Path operations will respect external_base_path in OpenSpec adapter # Import ADO work item as change proposal using backlog adapter pattern - proposal = self.import_backlog_item_as_proposal(artifact_path, "ado", bridge_config) + existing_proposals = ( + dict(project_bundle.change_tracking.proposals) if getattr(project_bundle, "change_tracking", None) else {} + ) + proposal = self.import_backlog_item_as_proposal( + artifact_path, + "ado", + bridge_config, + existing_proposals=existing_proposals, + ) if not proposal: msg = "Failed to import ADO work item as change proposal" @@ -1533,12 +1541,14 @@ def _get_work_item_data(self, work_item_id: int | str, org: str, project: str) - try: response = self._ado_get(url, headers=headers, timeout=10) work_item_data = response.json() + if not isinstance(work_item_data, dict): + return None fields = work_item_data.get("fields", {}) - return { - "title": fields.get("System.Title", ""), - "state": fields.get("System.State", ""), - "description": fields.get("System.Description", ""), - } + if isinstance(fields, dict): + work_item_data.setdefault("title", fields.get("System.Title", "")) + work_item_data.setdefault("state", fields.get("System.State", "")) + work_item_data.setdefault("description", fields.get("System.Description", "")) + return work_item_data except requests.HTTPError as e: if e.response is not None and e.response.status_code == 404: return None diff --git a/src/specfact_cli/adapters/backlog_base.py b/src/specfact_cli/adapters/backlog_base.py index 25b94275..7639f949 100644 --- a/src/specfact_cli/adapters/backlog_base.py +++ b/src/specfact_cli/adapters/backlog_base.py @@ -11,6 +11,7 @@ from __future__ import annotations +import re import time from abc import ABC, abstractmethod from datetime import UTC, datetime @@ -238,6 +239,163 @@ def extract_change_proposal_data(self, item_data: dict[str, Any]) -> dict[str, A tool-specific data formats (GitHub issue body, ADO work item fields, etc.). """ + @beartype + @ensure(lambda result: isinstance(result, str), "Must return string") + def _slugify_imported_change_title(self, title: str) -> str: + """Return a stable kebab-case slug for imported backlog titles.""" + normalized = re.sub(r"[^a-z0-9]+", "-", title.lower()).strip("-") + normalized = re.sub(r"-{2,}", "-", normalized) + return normalized or "change" + + @beartype + @ensure(lambda result: isinstance(result, str) and len(result) > 0, "Must return non-empty change id") + def _normalize_imported_change_id( + self, + proposal_data: dict[str, Any], + item_data: dict[str, Any], + tool_name: str, + existing_proposals: dict[str, ChangeProposal] | None = None, + ) -> str: + """Normalize imported change IDs so title-based slugs win over numeric fallbacks.""" + raw_change_id = self._get_imported_change_id_seed(proposal_data, item_data) + title = str(proposal_data.get("title") or item_data.get("title") or "").strip() + candidate = self._prefer_imported_title_slug(raw_change_id, title) + proposals = existing_proposals or {} + return self._dedupe_imported_change_id(candidate, raw_change_id, item_data, tool_name, proposals) + + @beartype + @ensure(lambda result: isinstance(result, str) and len(result) > 0, "Must return non-empty seed") + def _get_imported_change_id_seed(self, proposal_data: dict[str, Any], item_data: dict[str, Any]) -> str: + return str(proposal_data.get("change_id") or item_data.get("id") or item_data.get("number") or "unknown") + + @beartype + @ensure(lambda result: isinstance(result, str) and len(result) > 0, "Must return non-empty candidate") + def _prefer_imported_title_slug(self, raw_change_id: str, title: str) -> str: + if raw_change_id and raw_change_id != "unknown" and not raw_change_id.isdigit(): + return raw_change_id + if not title: + return raw_change_id or "unknown" + return self._slugify_imported_change_title(title) + + @beartype + @ensure(lambda result: isinstance(result, str) and len(result) > 0, "Must return non-empty change id") + def _dedupe_imported_change_id( + self, + candidate: str, + raw_change_id: str, + item_data: dict[str, Any], + tool_name: str, + existing_proposals: dict[str, ChangeProposal], + ) -> str: + existing_change_id = self._find_existing_imported_change_id_by_source( + item_data, + tool_name, + existing_proposals, + ) + if existing_change_id: + return existing_change_id + + existing_proposal = existing_proposals.get(candidate) + if existing_proposal is None: + return candidate or raw_change_id or "unknown" + + if self._matches_existing_import_source(existing_proposal, item_data, tool_name): + return candidate + + source_id = item_data.get("id") or item_data.get("number") + if source_id is None: + return candidate or raw_change_id or "unknown" + + deduped_candidate = f"{candidate}-{source_id}" + existing_deduped = existing_proposals.get(deduped_candidate) + if existing_deduped and self._matches_existing_import_source(existing_deduped, item_data, tool_name): + return deduped_candidate + return deduped_candidate + + @beartype + @ensure(lambda result: result is None or isinstance(result, str), "Must return change id or None") + def _find_existing_imported_change_id_by_source( + self, + item_data: dict[str, Any], + tool_name: str, + existing_proposals: dict[str, ChangeProposal], + ) -> str | None: + for change_id, proposal in existing_proposals.items(): + if self._matches_existing_import_source(proposal, item_data, tool_name): + return change_id + return None + + @beartype + @ensure(lambda result: isinstance(result, str), "Must return source URL string") + def _get_import_source_url(self, item_data: dict[str, Any]) -> str: + html_url = item_data.get("html_url") + if isinstance(html_url, str) and html_url: + return html_url + + url = item_data.get("url") + if isinstance(url, str) and url: + return url + + links = item_data.get("_links") + if not isinstance(links, dict): + return "" + html_link = links.get("html") + if not isinstance(html_link, dict): + return "" + href = html_link.get("href") + return href if isinstance(href, str) else "" + + @beartype + @ensure(lambda result: isinstance(result, bool), "Must return match flag") + def _matches_existing_import_source( + self, + proposal: ChangeProposal, + item_data: dict[str, Any], + tool_name: str, + ) -> bool: + source_tracking = proposal.source_tracking + if source_tracking is None or not isinstance(source_tracking.source_metadata, dict): + return False + + source_id = item_data.get("id") or item_data.get("number") + source_id_str = str(source_id) if source_id is not None else None + source_url = self._get_import_source_url(item_data) + source_type = tool_name.lower() + source_metadata = source_tracking.source_metadata + backlog_entries = source_metadata.get("backlog_entries") + if isinstance(backlog_entries, list): + for entry in backlog_entries: + if isinstance(entry, dict) and self._source_metadata_matches( + entry, source_type, source_id_str, source_url + ): + return True + + fallback_metadata = dict(source_metadata) + fallback_metadata.setdefault("source_type", source_tracking.tool) + return self._source_metadata_matches(fallback_metadata, source_type, source_id_str, source_url) + + @beartype + @ensure(lambda result: isinstance(result, bool), "Must return match flag") + def _source_metadata_matches( + self, + source_metadata: dict[str, Any], + source_type: str, + source_id: str | None, + source_url: str, + ) -> bool: + entry_type = str(source_metadata.get("source_type") or source_metadata.get("tool") or "").lower() + if entry_type and entry_type != source_type: + return False + + entry_url = source_metadata.get("source_url") + if source_url and isinstance(entry_url, str) and entry_url == source_url: + return True + + entry_id = source_metadata.get("source_id") + if source_id is None or entry_id is None: + return False + return str(entry_id) == source_id + @beartype @require(lambda item_data: isinstance(item_data, dict), "Item data must be dict") @require(lambda tool_name: isinstance(tool_name, str) and len(tool_name) > 0, "Tool name must be non-empty") @@ -265,20 +423,16 @@ def create_source_tracking( """ source_metadata: dict[str, Any] = {} - # Extract common fields (ID, URL) if present source_id = None if tool_name.lower() == "github": source_id = item_data.get("number") or item_data.get("id") - # GitHub: convert to string for consistency (GitHub issue numbers are strings) if source_id is not None: source_metadata["source_id"] = str(source_id) else: - # For ADO and other adapters: preserve original type - # ADO work item IDs are integers, so keep as int source_id = item_data.get("id") or item_data.get("number") if source_id is not None: source_metadata["source_id"] = source_id - # Prefer html_url (user-friendly) over url (API URL) + if "html_url" in item_data: source_metadata["source_url"] = item_data.get("html_url") elif "url" in item_data: @@ -291,7 +445,6 @@ def create_source_tracking( assignees = [item_data["assignee"]] if item_data["assignee"] else [] source_metadata["assignees"] = assignees - # Add cross-repo support if bridge_config has external_base_path if bridge_config and hasattr(bridge_config, "external_base_path") and bridge_config.external_base_path: source_metadata["external_base_path"] = str(bridge_config.external_base_path) @@ -303,7 +456,8 @@ def create_source_tracking( "Status must be non-empty", ) @require( - lambda backlog_status: isinstance(backlog_status, str) and len(backlog_status) > 0, "Status must be non-empty" + lambda backlog_status: isinstance(backlog_status, str) and len(backlog_status) > 0, + "Status must be non-empty", ) @ensure(lambda result: isinstance(result, str), "Must return conflict resolution strategy name") def resolve_status_conflict( @@ -335,7 +489,6 @@ def resolve_status_conflict( if strategy == "prefer_backlog": return backlog_status if strategy == "merge": - # Status priority: applied > in-progress > proposed > deprecated > discarded status_priority = { "applied": 5, "in-progress": 4, @@ -347,7 +500,6 @@ def resolve_status_conflict( backlog_priority = status_priority.get(backlog_status, 0) return openspec_status if openspec_priority >= backlog_priority else backlog_status - # Default: prefer OpenSpec return openspec_status @beartype @@ -355,7 +507,11 @@ def resolve_status_conflict( @require(lambda tool_name: isinstance(tool_name, str) and len(tool_name) > 0, "Tool name must be non-empty") @ensure(lambda result: isinstance(result, ChangeProposal) or result is None, "Must return ChangeProposal or None") def import_backlog_item_as_proposal( - self, item_data: dict[str, Any], tool_name: str, bridge_config: Any = None + self, + item_data: dict[str, Any], + tool_name: str, + bridge_config: Any = None, + existing_proposals: dict[str, ChangeProposal] | None = None, ) -> ChangeProposal | None: """ Import backlog item as OpenSpec change proposal (reusable pattern). @@ -370,6 +526,7 @@ def import_backlog_item_as_proposal( item_data: Backlog item data (tool-specific format) tool_name: Tool identifier (e.g., "github", "ado", "jira", "linear") bridge_config: Optional bridge configuration (for cross-repo support) + existing_proposals: Existing proposals used for collision-safe and idempotent imported IDs Returns: ChangeProposal instance if successful, None if data is invalid @@ -383,22 +540,16 @@ def import_backlog_item_as_proposal( and map_backlog_status_to_openspec(). """ try: - # Extract change proposal data (tool-specific parsing) proposal_data = self.extract_change_proposal_data(item_data) - # Get status from extracted data or map from backlog item if "status" in proposal_data: openspec_status = proposal_data["status"] else: - # Map backlog status to OpenSpec status backlog_status = item_data.get("state") or item_data.get("status") or "open" openspec_status = self.map_backlog_status_to_openspec(backlog_status) - # Create source tracking source_tracking = self.create_source_tracking(item_data, tool_name, bridge_config) - - # Create change proposal - change_id = proposal_data.get("change_id") or item_data.get("id") or item_data.get("number") or "unknown" + change_id = self._normalize_imported_change_id(proposal_data, item_data, tool_name, existing_proposals) return ChangeProposal( name=change_id, title=proposal_data.get("title", "Untitled Change Proposal"), diff --git a/src/specfact_cli/adapters/github.py b/src/specfact_cli/adapters/github.py index 9962b9f3..873e45ef 100644 --- a/src/specfact_cli/adapters/github.py +++ b/src/specfact_cli/adapters/github.py @@ -265,7 +265,7 @@ def extract_change_proposal_data(self, item_data: dict[str, Any]) -> dict[str, A Change ID extraction priority: 1. Body footer (legacy format): *OpenSpec Change Proposal: `id`* 2. Comments (new format): **Change ID**: `id` in OpenSpec Change Proposal Reference comment - 3. Issue number (fallback) + 3. Issue number (fallback, normalized during shared proposal import) """ if not isinstance(item_data, dict): msg = "GitHub issue data must be dict" @@ -554,7 +554,15 @@ def import_artifact( pass # Path operations will respect external_base_path in OpenSpec adapter # Import GitHub issue as change proposal using backlog adapter pattern - proposal = self.import_backlog_item_as_proposal(artifact_path, "github", bridge_config) + existing_proposals = ( + dict(project_bundle.change_tracking.proposals) if getattr(project_bundle, "change_tracking", None) else {} + ) + proposal = self.import_backlog_item_as_proposal( + artifact_path, + "github", + bridge_config, + existing_proposals=existing_proposals, + ) if not proposal: msg = "Failed to import GitHub issue as change proposal" diff --git a/tests/integration/sync/test_ado_backlog_sync.py b/tests/integration/sync/test_ado_backlog_sync.py index 128b71fc..9bdc9c28 100644 --- a/tests/integration/sync/test_ado_backlog_sync.py +++ b/tests/integration/sync/test_ado_backlog_sync.py @@ -124,8 +124,8 @@ def test_ado_to_openspec_import( project_bundle=project_bundle, ) - assert "123" in project_bundle.change_tracking.proposals - proposal = project_bundle.change_tracking.proposals["123"] + assert "add-feature-x" in project_bundle.change_tracking.proposals + proposal = project_bundle.change_tracking.proposals["add-feature-x"] assert proposal.title == "Add Feature X" assert proposal.status == "proposed" assert proposal.source_tracking is not None diff --git a/tests/integration/sync/test_backlog_sync.py b/tests/integration/sync/test_backlog_sync.py index fcd4257e..b4560cb6 100644 --- a/tests/integration/sync/test_backlog_sync.py +++ b/tests/integration/sync/test_backlog_sync.py @@ -100,8 +100,8 @@ def test_github_to_openspec_import(self, github_adapter: GitHubAdapter, tmp_path project_bundle=project_bundle, ) - assert "123" in project_bundle.change_tracking.proposals - proposal = project_bundle.change_tracking.proposals["123"] + assert "add-feature-x" in project_bundle.change_tracking.proposals + proposal = project_bundle.change_tracking.proposals["add-feature-x"] assert proposal.title == "Add Feature X" assert proposal.status == "proposed" assert proposal.source_tracking is not None diff --git a/tests/integration/sync/test_bridge_sync_import.py b/tests/integration/sync/test_bridge_sync_import.py new file mode 100644 index 00000000..bdc6555f --- /dev/null +++ b/tests/integration/sync/test_bridge_sync_import.py @@ -0,0 +1,66 @@ +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock, patch + +from beartype import beartype + +from specfact_cli.adapters.ado import AdoAdapter +from specfact_cli.models.bridge import BridgeConfig +from specfact_cli.models.plan import Product +from specfact_cli.models.project import BundleManifest, ProjectBundle +from specfact_cli.sync.bridge_sync import BridgeSync +from specfact_cli.utils.bundle_loader import save_project_bundle + + +def _create_sample_bundle(base_path: Path, bundle_name: str = "demo-bundle") -> Path: + projects_dir = base_path / ".specfact" / "projects" + projects_dir.mkdir(parents=True, exist_ok=True) + + bundle_dir = projects_dir / bundle_name + manifest = BundleManifest(schema_metadata=None, project_metadata=None) + bundle = ProjectBundle(manifest=manifest, bundle_name=bundle_name, product=Product(themes=["Testing"])) + save_project_bundle(bundle, bundle_dir, atomic=True) + return bundle_dir + + +class TestBridgeSyncImport: + @beartype + def test_import_selected_ado_backlog_item_uses_native_payload_for_import(self, tmp_path: Path) -> None: + _create_sample_bundle(tmp_path) + sync = BridgeSync(tmp_path, bridge_config=BridgeConfig.preset_openspec()) + adapter = AdoAdapter(org="test-org", project="test-project", api_token="test-token") + + work_item_payload = { + "id": 123, + "fields": { + "System.Title": "Add Feature X", + "System.Description": "## Why\n\nNeeded\n\n## What Changes\n\nImplement", + "System.State": "New", + "System.CreatedDate": "2025-01-01T10:00:00Z", + "System.WorkItemType": "User Story", + }, + "_links": { + "html": {"href": "https://dev.azure.com/test-org/test-project/_workitems/edit/123"}, + }, + } + mock_response = MagicMock() + mock_response.json.return_value = work_item_payload + mock_response.raise_for_status = MagicMock() + + with ( + patch.object(adapter, "_ado_get", return_value=mock_response), + patch.object(adapter, "generate_bridge_config", return_value=BridgeConfig.preset_ado()), + patch.object(adapter, "_get_work_item_comments", return_value=[]), + patch("specfact_cli.sync.bridge_sync.AdapterRegistry.get_adapter", return_value=adapter), + patch.object(sync, "_write_openspec_change_from_proposal", return_value=[]), + ): + result = sync.import_backlog_items_to_bundle( + adapter_type="ado", + bundle_name="demo-bundle", + backlog_items=["123"], + ) + + assert result.success is True + assert result.errors == [] + assert len(result.operations) == 1 diff --git a/tests/unit/adapters/test_ado.py b/tests/unit/adapters/test_ado.py index 917bc891..016513a8 100644 --- a/tests/unit/adapters/test_ado.py +++ b/tests/unit/adapters/test_ado.py @@ -16,6 +16,8 @@ from specfact_cli.adapters.ado import AdoAdapter from specfact_cli.models.bridge import AdapterType, BridgeConfig +from specfact_cli.models.change import ChangeProposal, ChangeTracking +from specfact_cli.models.source_tracking import SourceTracking @pytest.fixture @@ -362,8 +364,8 @@ def test_import_artifact_ado_work_item(self, ado_adapter: AdoAdapter, tmp_path: project_bundle=project_bundle, ) - assert "123" in project_bundle.change_tracking.proposals - proposal = project_bundle.change_tracking.proposals["123"] + assert "add-feature-x" in project_bundle.change_tracking.proposals + proposal = project_bundle.change_tracking.proposals["add-feature-x"] assert proposal.title == "Add Feature X" assert proposal.status == "proposed" assert proposal.source_tracking is not None @@ -809,3 +811,225 @@ def test_verify_branch_exists(self, ado_adapter: AdoAdapter, tmp_path: Path) -> assert ado_adapter._verify_branch_exists("feature/test", tmp_path) is True assert ado_adapter._verify_branch_exists("nonexistent", tmp_path) is False + + @beartype + @patch.object(AdoAdapter, "_ado_get") + def test_fetch_backlog_item_preserves_native_payload( # pylint: disable=redefined-outer-name + self, + mock_ado_get: MagicMock, + ado_adapter: AdoAdapter, + ) -> None: + """Selective fetch should keep the native ADO payload for proposal import.""" + work_item_data = { + "id": 123, + "fields": { + "System.Title": "Add Feature X", + "System.Description": "

Implement feature X

", + "System.State": "New", + }, + "_links": { + "html": {"href": "https://dev.azure.com/test-org/test-project/_workitems/edit/123"}, + }, + } + mock_response = MagicMock() + mock_response.json.return_value = work_item_data + mock_response.raise_for_status = MagicMock() + mock_ado_get.return_value = mock_response + + result = ado_adapter.fetch_backlog_item("123") + + assert result["id"] == 123 + assert result["fields"]["System.Title"] == "Add Feature X" + assert result["title"] == "Add Feature X" + assert result["state"] == "New" + assert result["description"] == "

Implement feature X

" + + @beartype + @patch.object(AdoAdapter, "_get_work_item_comments", return_value=[]) + def test_import_artifact_ado_work_item_collision_uses_source_suffix( # pylint: disable=redefined-outer-name + self, + _mock_get_comments: MagicMock, + ado_adapter: AdoAdapter, + tmp_path: Path, + ) -> None: + """Duplicate imported slugs should keep the title and append the source ID.""" + project_bundle = MagicMock() + project_bundle.change_tracking = ChangeTracking( + proposals={ + "add-feature-x": ChangeProposal( + name="add-feature-x", + title="Existing Feature X", + description="Existing", + rationale="Existing rationale", + timeline=None, + owner=None, + status="proposed", + created_at="2025-01-01T10:00:00+00:00", + applied_at=None, + archived_at=None, + source_tracking=SourceTracking(tool="ado"), + ) + } + ) + project_bundle.bundle_dir = tmp_path + + work_item_data = { + "id": 123, + "fields": { + "System.Title": "Add Feature X", + "System.Description": "## Why\n\nNeeded\n\n## What Changes\n\nImplement", + "System.State": "New", + "System.CreatedDate": "2025-01-01T10:00:00Z", + "System.WorkItemType": "User Story", + }, + "_links": { + "html": {"href": "https://dev.azure.com/test-org/test-project/_workitems/edit/123"}, + }, + } + + ado_adapter.import_artifact( + artifact_key="ado_work_item", + artifact_path=work_item_data, + project_bundle=project_bundle, + ) + + assert "add-feature-x-123" in project_bundle.change_tracking.proposals + proposal = project_bundle.change_tracking.proposals["add-feature-x-123"] + assert proposal.title == "Add Feature X" + assert proposal.source_tracking is not None + assert proposal.source_tracking.source_metadata["backlog_entries"][0]["source_id"] == "123" + + @beartype + @patch.object(AdoAdapter, "_get_work_item_comments", return_value=[]) + def test_import_artifact_ado_work_item_reimport_keeps_original_slug( + self, + _mock_get_comments: MagicMock, + ado_adapter: AdoAdapter, + tmp_path: Path, + ) -> None: + """Re-importing the same work item should keep the original proposal name.""" + project_bundle = MagicMock() + project_bundle.change_tracking = ChangeTracking( + proposals={ + "add-feature-x": ChangeProposal( + name="add-feature-x", + title="Add Feature X", + description="Existing", + rationale="Existing rationale", + timeline=None, + owner=None, + status="proposed", + created_at="2025-01-01T10:00:00+00:00", + applied_at=None, + archived_at=None, + source_tracking=SourceTracking( + tool="ado", + source_metadata={ + "source_id": 123, + "source_url": "https://dev.azure.com/test-org/test-project/_workitems/edit/123", + "source_type": "ado", + "backlog_entries": [ + { + "source_id": "123", + "source_url": "https://dev.azure.com/test-org/test-project/_workitems/edit/123", + "source_type": "ado", + } + ], + }, + ), + ) + } + ) + project_bundle.bundle_dir = tmp_path + + work_item_data = { + "id": 123, + "fields": { + "System.Title": "Add Feature X", + "System.Description": "## Why\n\nNeeded\n\n## What Changes\n\nImplement", + "System.State": "New", + "System.CreatedDate": "2025-01-01T10:00:00Z", + "System.WorkItemType": "User Story", + }, + "_links": { + "html": {"href": "https://dev.azure.com/test-org/test-project/_workitems/edit/123"}, + }, + } + + ado_adapter.import_artifact( + artifact_key="ado_work_item", + artifact_path=work_item_data, + project_bundle=project_bundle, + ) + + assert "add-feature-x" in project_bundle.change_tracking.proposals + assert "add-feature-x-123" not in project_bundle.change_tracking.proposals + assert len(project_bundle.change_tracking.proposals) == 1 + + @beartype + @patch.object(AdoAdapter, "_get_work_item_comments", return_value=[]) + def test_import_artifact_ado_work_item_reimport_with_renamed_title_keeps_original_slug( + self, + _mock_get_comments: MagicMock, + ado_adapter: AdoAdapter, + tmp_path: Path, + ) -> None: + """Re-importing the same work item with a new title should keep the original proposal name.""" + project_bundle = MagicMock() + project_bundle.change_tracking = ChangeTracking( + proposals={ + "add-feature-x": ChangeProposal( + name="add-feature-x", + title="Add Feature X", + description="Existing", + rationale="Existing rationale", + timeline=None, + owner=None, + status="proposed", + created_at="2025-01-01T10:00:00+00:00", + applied_at=None, + archived_at=None, + source_tracking=SourceTracking( + tool="ado", + source_metadata={ + "source_id": 123, + "source_url": "https://dev.azure.com/test-org/test-project/_workitems/edit/123", + "source_type": "ado", + "backlog_entries": [ + { + "source_id": "123", + "source_url": "https://dev.azure.com/test-org/test-project/_workitems/edit/123", + "source_type": "ado", + } + ], + }, + ), + ) + } + ) + project_bundle.bundle_dir = tmp_path + + work_item_data = { + "id": 123, + "fields": { + "System.Title": "Rename Feature X", + "System.Description": "## Why\n\nNeeded\n\n## What Changes\n\nImplement", + "System.State": "New", + "System.CreatedDate": "2025-01-01T10:00:00Z", + "System.WorkItemType": "User Story", + }, + "_links": { + "html": {"href": "https://dev.azure.com/test-org/test-project/_workitems/edit/123"}, + }, + } + + ado_adapter.import_artifact( + artifact_key="ado_work_item", + artifact_path=work_item_data, + project_bundle=project_bundle, + ) + + assert "add-feature-x" in project_bundle.change_tracking.proposals + assert "rename-feature-x" not in project_bundle.change_tracking.proposals + assert "rename-feature-x-123" not in project_bundle.change_tracking.proposals + assert len(project_bundle.change_tracking.proposals) == 1 diff --git a/tests/unit/adapters/test_github.py b/tests/unit/adapters/test_github.py index 594cb6e1..709cdc67 100644 --- a/tests/unit/adapters/test_github.py +++ b/tests/unit/adapters/test_github.py @@ -657,8 +657,8 @@ def test_import_artifact_github_issue(self, github_adapter: GitHubAdapter, tmp_p project_bundle=project_bundle, ) - assert "123" in project_bundle.change_tracking.proposals - proposal = project_bundle.change_tracking.proposals["123"] + assert "add-feature-x" in project_bundle.change_tracking.proposals + proposal = project_bundle.change_tracking.proposals["add-feature-x"] assert proposal.title == "Add Feature X" assert proposal.status == "proposed" assert proposal.source_tracking is not None @@ -811,3 +811,30 @@ def test_create_source_tracking(self, github_adapter: GitHubAdapter) -> None: assert source_tracking.source_metadata["source_url"] == "https://github.com/test-owner/test-repo/issues/123" assert source_tracking.source_metadata["source_state"] == "open" assert len(source_tracking.source_metadata["assignees"]) == 1 + + @beartype + @patch("specfact_cli.adapters.github.requests.get") + def test_fetch_backlog_item_preserves_native_issue_payload( # pylint: disable=redefined-outer-name + self, + mock_get: MagicMock, + github_adapter: GitHubAdapter, + ) -> None: + """Similar selective fetch path should keep the native GitHub issue payload.""" + issue_data = { + "number": 123, + "title": "Add Feature X", + "body": "## Why\n\nNeeded\n\n## What Changes\n\nImplement", + "state": "open", + "html_url": "https://github.com/test-owner/test-repo/issues/123", + } + mock_response = MagicMock() + mock_response.json.return_value = issue_data + mock_response.raise_for_status = MagicMock() + mock_response.ok = True + mock_get.return_value = mock_response + + result = github_adapter.fetch_backlog_item("123") + + assert result == issue_data + assert result["title"] == "Add Feature X" + assert result["body"].startswith("## Why") diff --git a/tests/unit/docs/test_release_docs_parity.py b/tests/unit/docs/test_release_docs_parity.py index 12d59235..5bbaddef 100644 --- a/tests/unit/docs/test_release_docs_parity.py +++ b/tests/unit/docs/test_release_docs_parity.py @@ -1,13 +1,28 @@ from __future__ import annotations +import re from pathlib import Path +from urllib.parse import unquote, urlparse MODULES_DOCS_HOST = "modules.specfact.io" +DOCS_HOST = "docs.specfact.io" +MARKDOWN_LINK_RE = re.compile(r"(? Path: + return Path(__file__).resolve().parents[3] def _repo_file(path: str) -> Path: - return Path(__file__).resolve().parents[3] / path + return _repo_root() / path + + +def _docs_root() -> Path: + return _repo_root() / "docs" def _assert_mentions_modules_docs_site(content: str) -> None: @@ -17,6 +32,208 @@ def _assert_mentions_modules_docs_site(content: str) -> None: assert content[host_index + len(MODULES_DOCS_HOST)] == "/" +def _is_docs_markdown(path: Path) -> bool: + return path.suffix == ".md" and "_site" not in path.parts and "vendor" not in path.parts + + +def _iter_docs_markdown_paths() -> list[Path]: + return sorted(path.resolve() for path in _docs_root().rglob("*.md") if _is_docs_markdown(path)) + + +def _read_text(path: Path) -> str: + return path.read_text(encoding="utf-8") + + +def _split_front_matter(text: str) -> tuple[dict[str, str], str]: + if not text.startswith("---\n"): + return {}, text + + lines = text.splitlines() + end_index = None + for index in range(1, len(lines)): + if lines[index].strip() == "---": + end_index = index + break + if end_index is None: + return {}, text + + metadata: dict[str, str] = {} + for raw_line in lines[1:end_index]: + if ":" not in raw_line: + continue + key, value = raw_line.split(":", 1) + metadata[key.strip()] = value.strip().strip('"').strip("'") + + body = "\n".join(lines[end_index + 1 :]) + return metadata, body + + +def _normalize_route(route: str) -> str: + cleaned = unquote(route.strip()) + if not cleaned: + return "/" + if not cleaned.startswith("/"): + cleaned = "/" + cleaned + cleaned = re.sub(r"/{2,}", "/", cleaned) + if cleaned != "/" and not cleaned.endswith("/"): + cleaned += "/" + return cleaned + + +def _published_route_for_path(path: Path, metadata: dict[str, str]) -> str: + permalink = metadata.get("permalink") + if permalink: + return _normalize_route(permalink) + return _normalize_route(f"/{path.stem}/") + + +def _build_published_docs_index() -> tuple[dict[str, Path], dict[Path, dict[str, str]], dict[Path, str]]: + route_to_path: dict[str, Path] = {} + path_to_metadata: dict[Path, dict[str, str]] = {} + path_to_route: dict[Path, str] = {} + + for path in _iter_docs_markdown_paths(): + metadata, _ = _split_front_matter(_read_text(path)) + route = _published_route_for_path(path, metadata) + route_to_path[route] = path + path_to_metadata[path] = metadata + path_to_route[path] = route + + return route_to_path, path_to_metadata, path_to_route + + +def _extract_links(source: Path, content: str) -> list[str]: + if source.suffix == ".html": + return HTML_HREF_RE.findall(content) + return MARKDOWN_LINK_RE.findall(content) + + +def _normalize_jekyll_relative_url(link: str) -> str: + match = JEKYLL_RELATIVE_URL_RE.fullmatch(link.strip()) + if match: + return match.group(1) + return link + + +def _is_published_docs_route_candidate(route: str) -> bool: + return route not in {"/assets/main.css/", "/feed.xml/"} + + +def _resolve_internal_docs_target( + source: Path, + raw_link: str, + route_to_path: dict[str, Path], + path_to_route: dict[Path, str], +) -> tuple[str | None, Path | None, str | None]: + stripped = _normalize_jekyll_relative_url(raw_link.strip()) + if not stripped or stripped.startswith("#"): + return None, None, None + + parsed = urlparse(stripped) + if parsed.scheme in {"mailto", "javascript", "tel"}: + return None, None, None + if parsed.scheme in {"http", "https"}: + if parsed.netloc != DOCS_HOST: + return None, None, None + route = _normalize_route(parsed.path or "/") + if not _is_published_docs_route_candidate(route): + return None, None, None + target = route_to_path.get(route) + if target is None: + return route, None, f"{source.relative_to(_repo_root())} -> {route}" + return route, target, None + if parsed.scheme: + return None, None, None + + target_value = unquote(parsed.path) + if not target_value: + return None, None, None + + if target_value.startswith("/"): + route = _normalize_route(target_value) + if not _is_published_docs_route_candidate(route): + return None, None, None + target = route_to_path.get(route) + if target is None: + return route, None, f"{source.relative_to(_repo_root())} -> {route}" + return route, target, None + + candidate = (source.parent / target_value).resolve() + if candidate.is_dir(): + readme_candidate = (candidate / "README.md").resolve() + if readme_candidate.is_file() and _is_docs_markdown(readme_candidate): + route = path_to_route.get(readme_candidate) + if route is None: + return None, None, f"{source.relative_to(_repo_root())} -> {target_value}" + return route, readme_candidate, None + return None, None, None + + if candidate.is_file() and _is_docs_markdown(candidate): + route = path_to_route.get(candidate) + if route is None: + return None, None, f"{source.relative_to(_repo_root())} -> {target_value}" + return route, candidate, None + + if not candidate.suffix: + markdown_candidate = candidate.with_suffix(".md") + if markdown_candidate.is_file() and _is_docs_markdown(markdown_candidate): + resolved_candidate = markdown_candidate.resolve() + route = path_to_route.get(resolved_candidate) + if route is None: + return None, None, f"{source.relative_to(_repo_root())} -> {target_value}" + return route, resolved_candidate, None + + route = _normalize_route(target_value) + if not _is_published_docs_route_candidate(route): + return None, None, None + target = route_to_path.get(route) + if target is None: + return route, None, f"{source.relative_to(_repo_root())} -> {target_value} (normalized: {route})" + return route, target, None + + +def _navigation_sources() -> list[Path]: + return [ + _repo_file("docs/index.md").resolve(), + _repo_file("docs/_layouts/default.html").resolve(), + ] + + +def _scan_navigation_targets() -> tuple[list[str], set[Path]]: + route_to_path, _, path_to_route = _build_published_docs_index() + failures: list[str] = [] + targets: set[Path] = set() + + for source in _navigation_sources(): + for link in _extract_links(source, _read_text(source)): + _, target, failure = _resolve_internal_docs_target(source, link, route_to_path, path_to_route) + if failure: + failures.append(failure) + if target is not None: + targets.add(target) + + return failures, targets + + +def _scan_authored_doc_link_failures() -> tuple[list[str], set[Path]]: + route_to_path, _, path_to_route = _build_published_docs_index() + failures: list[str] = [] + targets: set[Path] = set() + + for source in _iter_docs_markdown_paths(): + metadata, body = _split_front_matter(_read_text(source)) + if not metadata: + continue + for link in _extract_links(source, body): + _, target, failure = _resolve_internal_docs_target(source, link, route_to_path, path_to_route) + if failure: + failures.append(failure) + if target is not None: + targets.add(target) + + return failures, targets + + def test_changelog_has_single_0340_release_header() -> None: changelog = _repo_file("CHANGELOG.md").read_text(encoding="utf-8") assert changelog.count("## [0.34.0] - 2026-02-18") == 1 @@ -105,12 +322,12 @@ def _scan_authored_docs(pattern: str) -> list[tuple[str, int, str]]: - Any line where the pattern co-occurs with the word "removed" or "(removed)" """ hits: list[tuple[str, int, str]] = [] - repo_root = Path(__file__).resolve().parents[3] + repo_root = _repo_root() sources: list[Path] = [repo_root / "README.md"] docs_dir = repo_root / "docs" - for p in docs_dir.rglob("*.md"): - if "_site" not in p.parts and "vendor" not in p.parts: - sources.append(p) + for path in docs_dir.rglob("*.md"): + if "_site" not in path.parts and "vendor" not in path.parts: + sources.append(path) for src in sources: if not src.exists(): continue @@ -118,7 +335,6 @@ def _scan_authored_docs(pattern: str) -> list[tuple[str, int, str]]: if pattern not in line: continue stripped = line.strip() - # Skip code-block comment lines, but do not ignore Markdown headings. if stripped.startswith("#") and not stripped.startswith( ("# ", "## ", "### ", "#### ", "##### ", "###### ") ): @@ -133,7 +349,7 @@ def _scan_authored_docs(pattern: str) -> list[tuple[str, int, str]]: def _fmt_hits(hits: list[tuple[str, int, str]]) -> str: - return "\n".join(f" {p}:{n} {line}" for p, n, line in hits) + return "\n".join(f" {path}:{lineno} {line}" for path, lineno, line in hits) def test_removed_project_plan_syntax_absent_from_authored_docs() -> None: @@ -199,13 +415,38 @@ def test_current_backlog_subcommands_documented_in_commands_reference() -> None: def test_all_published_docs_markdown_files_have_jekyll_front_matter() -> None: - repo_root = Path(__file__).resolve().parents[3] - docs_root = repo_root / "docs" missing: list[str] = [] - for path in sorted(docs_root.rglob("*.md")): - if "_site" in path.parts or "vendor" in path.parts: - continue - first_line = path.read_text(encoding="utf-8").splitlines()[0] if path.read_text(encoding="utf-8") else "" + for path in _iter_docs_markdown_paths(): + first_line = _read_text(path).splitlines()[0] if _read_text(path) else "" if first_line != "---": - missing.append(str(path.relative_to(repo_root))) + missing.append(str(path.relative_to(_repo_root()))) assert not missing, "Docs files missing front matter:\n" + "\n".join(missing) + + +# --------------------------------------------------------------------------- +# docs-04-docs-review-gate-and-link-integrity +# --------------------------------------------------------------------------- + + +def test_navigation_links_resolve_to_published_docs_routes() -> None: + failures, _ = _scan_navigation_targets() + assert not failures, "Broken navigation docs links:\n" + "\n".join(sorted(failures)) + + +def test_authored_internal_docs_links_resolve_to_published_docs_targets() -> None: + failures, _ = _scan_authored_doc_link_failures() + assert not failures, "Broken authored docs links:\n" + "\n".join(sorted(failures)) + + +def test_navigation_link_targets_have_required_front_matter_keys() -> None: + _, targets = _scan_navigation_targets() + _, path_to_metadata, _ = _build_published_docs_index() + missing: list[str] = [] + + for target in sorted(targets): + metadata = path_to_metadata[target] + missing_keys = [key for key in REQUIRED_NAV_FRONT_MATTER_KEYS if not metadata.get(key)] + if missing_keys: + missing.append(f"{target.relative_to(_repo_root())}: missing {', '.join(missing_keys)}") + + assert not missing, "Navigation-linked docs missing required front matter keys:\n" + "\n".join(missing)