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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,20 @@ All notable changes to this repository will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
and this project follows SemVer for bundle versions.

## [0.44.0] - 2026-03-17

### Added

- Add `--scope changed|full` and repeatable repo-relative `--path` filters to
`specfact code review run` for deterministic changed-only, full-repository,
and subtree-limited review selection.

### Changed

- Keep changed-only auto-discovery as the default, allow explicit test subtrees
to opt matching tests back into scope, and extend the review-run docs plus
cli-contract scenarios to cover the new targeting controls.

## [0.43.0] - 2026-03-16

### Added
Expand Down
26 changes: 25 additions & 1 deletion docs/modules/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ Options:
- `--score-only`: print only the integer `reward_delta`
- `--fix`: apply Ruff autofixes and re-run the review before printing results
- `--no-tests`: skip the targeted TDD gate
- `--scope changed|full`: choose changed-only or full-repository auto-discovery
when positional files are omitted
- `--path PATH`: repeatable repo-relative subtree filter for auto-discovered
review targets
- `--include-tests/--exclude-tests`: include or exclude changed test files when
review scope is auto-detected from `git diff`
- `--include-noise/--suppress-noise`: include or suppress known low-signal
Expand All @@ -34,7 +38,27 @@ git ls-files --others --exclude-standard
Only existing Python files from tracked or untracked workspace changes are
reviewed. Test files under `tests/` are excluded by default for auto-detected
review scope unless you pass `--include-tests` or answer yes in
`--interactive` mode.
`--interactive` mode. Explicit `--path tests/...` filters count as intentional
targeting and keep matching tests in scope even with the default test
exclusion.

Use `--scope full` to review the governed repository file set instead of just
current changes:

```bash
specfact code review run --scope full
```

Use repeatable `--path` filters to limit either scope to one package or a
focused source-plus-test slice:

```bash
specfact code review run --scope full --path packages/specfact-code-review
specfact code review run --scope changed --path packages/specfact-code-review --path tests/unit/specfact_code_review
```

Positional `FILES...` cannot be mixed with `--scope` or `--path`. Choose one
targeting style per invocation.

With default noise suppression, the review also hides known low-signal test
findings such as:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-03-17
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
## Verification Report: code-review-10-review-scope-modes

### Summary

| Dimension | Status |
| --- | --- |
| Completeness | 4/4 apply-ready artifacts present (`proposal`, `design`, `specs`, `tasks`) |
| Correctness | Scope-mode and path-filter requirements align with upstream `code-review-10-review-scope-modes` |
| Coherence | Proposal, design, and tasks consistently keep changed-only as the default auto-scope |

### Validation Checks

- Reviewed upstream `specfact-cli` proposal, design, and spec delta for `code-review-10-review-scope-modes`.
- Reviewed local artifacts: `proposal.md`, `design.md`, `tasks.md`, and delta specs under `specs/`.
- Checked current modules-repo review-run capabilities from `code-review-08-review-run-integration` to ensure this is a requirement change on an existing command, not a new command surface.

### CRITICAL

None.

### WARNING

- The derived change assumes the bundle command can identify a stable governed
repository file set for `--scope full`. If existing runtime logic only
supports changed-file discovery, implementation may need a new helper to keep
full-review selection deterministic across unit and e2e tests.
Recommendation: verify current target-resolution boundaries before coding and
centralize scope resolution rather than duplicating it across command tests
and runtime entry points.

### SUGGESTION

- Keep cli-val scenarios narrow and representative. One changed-only example,
one full-review example, one subtree-filter example, and one invalid
invocation are enough to prove the contract without making scenario YAML noisy.

### Dependency Analysis

- Modified capabilities: `review-run-command`, `review-cli-contracts`
- Primary implementation touchpoint: `packages/specfact-code-review/src/specfact_code_review/run/`
- Primary regression surface: `tests/unit/`, `tests/e2e/`, and `tests/cli-contracts/`
- Upstream source dependency: `nold-ai/specfact-cli` change `code-review-10-review-scope-modes`

### Final Assessment

No critical issues found. The derived change is internally consistent, matches
the upstream intent, and is ready for implementation after strict OpenSpec
validation passes.

### OpenSpec Validation

- **Status**: Pass
- **Command**: `openspec validate code-review-10-review-scope-modes --strict`
- **Issues Found/Fixed**: 0
106 changes: 106 additions & 0 deletions openspec/changes/code-review-10-review-scope-modes/TDD_EVIDENCE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# TDD Evidence: code-review-10 review scope modes

## Failing

### 2026-03-17 `tests/unit/specfact_code_review/run/test_commands.py`

Command:

```bash
HATCH_DATA_DIR=/tmp/hatch-data \
HATCH_CACHE_DIR=/tmp/hatch-cache \
VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \
hatch run pytest tests/unit/specfact_code_review/run/test_commands.py -q
```

Result: `5 failed, 14 passed`

Key failures before implementation:

- `test_run_command_supports_full_scope_and_path_filters`
- exit code was `2` because `--scope` was not supported yet
- `test_run_command_supports_changed_scope_with_repeatable_path_filters`
- exit code was `2` because `--scope`/`--path` were not supported yet
- `test_run_command_rejects_scope_mixed_with_positional_files`
- CLI returned the generic option error instead of the governed mixed-targeting message
- `test_run_command_rejects_path_mixed_with_positional_files`
- CLI returned the generic option error instead of the governed mixed-targeting message
- `test_run_command_fails_when_scope_and_paths_match_no_files`
- CLI returned the generic option error instead of an actionable empty-scope failure

## Passing

### 2026-03-17 Focused scope-mode tests

Commands:

```bash
HATCH_DATA_DIR=/tmp/hatch-data \
HATCH_CACHE_DIR=/tmp/hatch-cache \
VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \
hatch run pytest tests/unit/specfact_code_review/run/test_commands.py tests/unit/specfact_code_review/run/test_runner.py -q

HATCH_DATA_DIR=/tmp/hatch-data \
HATCH_CACHE_DIR=/tmp/hatch-cache \
VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \
hatch run pytest tests/e2e/specfact_code_review/test_review_run_e2e.py -q

HATCH_DATA_DIR=/tmp/hatch-data \
HATCH_CACHE_DIR=/tmp/hatch-cache \
VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \
hatch run validate-cli-contracts
```

Results:

- `tests/unit/specfact_code_review/run/test_commands.py` and `tests/unit/specfact_code_review/run/test_runner.py`: `36 passed`
- `tests/e2e/specfact_code_review/test_review_run_e2e.py`: `2 passed`
- CLI contract validation: `Validated 3 CLI contract scenario files.`

### 2026-03-17 SpecFact dogfood review

Command:

```bash
SPECFACT_ALLOW_UNSIGNED=1 \
HATCH_DATA_DIR=/tmp/hatch-data \
HATCH_CACHE_DIR=/tmp/hatch-cache \
VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata \
hatch run specfact code review run \
--scope changed \
--path packages/specfact-code-review \
--path tests/unit/specfact_code_review \
--json \
--out /tmp/code-review-10-report.json
```

Result:

- verdict: `PASS`
- score: `115`
- findings: `0`

### 2026-03-17 Worktree quality gates

Commands completed successfully:

```bash
HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run format
HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run type-check
HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run lint
HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run check-bundle-imports
HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run smart-test
HATCH_DATA_DIR=/tmp/hatch-data HATCH_CACHE_DIR=/tmp/hatch-cache VIRTUALENV_OVERRIDE_APP_DATA=/tmp/virtualenv-appdata hatch run contract-test
```

Results:

- `type-check`: `0 errors, 0 warnings, 0 notes`
- `lint`: `10.00/10`
- `smart-test`: `383 passed`
- `contract-test`: `384 passed`

Pending:

- `verify-modules-signature --require-signature --payload-from-filesystem --enforce-version-bump`
- blocked until the final `packages/specfact-code-review/module-package.yaml` changes in this worktree are re-signed
71 changes: 71 additions & 0 deletions openspec/changes/code-review-10-review-scope-modes/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# Design: code-review scope modes for modules repo

## Context

`code-review-08-review-run-integration` gave the modules repo a working
`specfact code review run` command, but its implicit changed-file discovery is
too narrow for stepwise review debt cleanup. The upstream `specfact-cli`
contract now defines two auto-discovery modes, `changed` and `full`, plus
repo-relative path filters that can recursively narrow either mode.

This repository owns the runtime command implementation, test fixtures, cli-val
scenario YAML, and bundle docs. The design therefore focuses on deterministic
file selection inside the bundle rather than on higher-level GitHub or
automation wiring.

## Goals / Non-Goals

**Goals:**
- Keep default no-argument behavior aligned with existing changed-file review
- Add `--scope changed|full` and repeatable `--path` filters at the bundle
command boundary
- Make subtree filtering recursive so users can review one package or test area
at a time
- Cover the new behavior with unit, e2e, and cli-val fixtures

**Non-Goals:**
- Changing scoring, verdict mapping, or ledger integration
- Adding globbing or exclusion syntax beyond repo-relative path prefixes
- Reworking the internal runner ordering established in `code-review-08`

## Decisions

### Decision: Resolve scope before filtering by path

The bundle should first compute the candidate file set from the selected scope,
then filter that set by any `--path` prefixes. That keeps the behavior stable
between tests and runtime, and makes it easy to explain to users.

### Decision: Reject positional files mixed with scope controls

Positional files already provide an explicit review target. Mixing them with
`--scope` or `--path` would create ambiguous precedence rules, so the command
should fail fast and tell the user to choose one targeting style.

### Decision: Treat `--path` values as repo-relative recursive path segments

Users asked for folder and subfolder limiting. Prefix-based matching is enough
for that need, but matching must happen on normalized path boundaries rather
than raw strings. That keeps `--path packages/specfact-code-review` scoped to
that package and its descendants instead of accidentally including sibling
paths such as `packages/specfact-code-review-old`, while still avoiding new
pattern syntax in the CLI contract.

### Decision: Explicit path filters may opt matched tests back in

The existing changed-only workflow excludes tests by default unless users pass
`--include-tests`. For scope-mode filtering, explicit `--path tests/...`
selection should count as intentional targeting, so matching test files remain
reviewable even when the global include-tests toggle stays at its default.

## Risks / Trade-offs

- [Risk] Full-review mode may surface far more findings than users expect.
Mitigation: keep it opt-in and document subtree filtering prominently.
- [Risk] Scope/path selection may vary between unit tests and e2e execution if
repo root detection is inconsistent.
Mitigation: centralize target resolution in one helper and exercise it
through both direct tests and installed-command tests.
- [Risk] cli-val scenarios could lag behind actual command behavior.
Mitigation: update review-run scenario YAML in the same change as the command
implementation and validate them together.
57 changes: 57 additions & 0 deletions openspec/changes/code-review-10-review-scope-modes/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Change: Reapply code-review-10 review scope modes in modules repo

## Why

The upstream `specfact-cli` change `code-review-10-review-scope-modes` defines
the governed contract for explicit changed-only vs. full review modes plus
repo-relative path filtering. The modules repository needs a matching active
change because the bundle-owned `specfact code review run` implementation,
runtime fixtures, and cli-val scenarios live here.

Without this derived change, the upstream contract would exist only on paper:
the installed `specfact-code-review` bundle would still lack the runtime
behavior needed to review a full repo or a limited subtree in a controlled way.

## What Changes

- Extend the `specfact-code-review` bundle command to support `--scope changed`
and `--scope full`.
- Add repeatable `--path` filtering for repo-relative files and subfolders in
auto-discovery mode.
- Keep changed-only auto-discovery as the default behavior when users do not
pass positional files or an explicit scope.
- Treat explicit `--path tests/...` filters as an intentional opt-in for those
matched test files without changing the default auto-scope exclusion of tests.
- Add targeted tests, fixtures, and cli-val scenarios for changed-only reviews,
full reviews, filtered subtree reviews, and invalid scope combinations.
- Update bundle docs, changelog, and signed package metadata to describe the
new review-scope controls.

## Capabilities

### New Capabilities

None.

### Modified Capabilities

- `review-run-command`: add explicit review-scope selection and repo-relative
path filtering to the bundle command
- `review-cli-contracts`: extend review-run scenario coverage for changed-only,
full-review, and subtree-limited invocations

## Impact

- Affects `packages/specfact-code-review/src/specfact_code_review/run/` command
parsing and target-file resolution
- Expands runtime validation in `tests/unit/`, `tests/e2e/`, and
`tests/cli-contracts/`
- Requires documentation, changelog, and module metadata updates for the
`specfact-code-review` bundle

## Source Tracking

<!-- source_repo: nold-ai/specfact-cli -->
- **Source Change**: `code-review-10-review-scope-modes`
- **Repository**: nold-ai/specfact-cli
- **Last Synced Status**: re-applied in modules repo
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
## MODIFIED Requirements

### Requirement: cli-val scenarios exist for review command groups
The modules repository SHALL define cli-val-compatible scenario YAML files for
the `specfact code review run`, `ledger`, and `rules` command groups, including
scope-mode and path-filter coverage for the review run command.

#### Scenario: review-run scenarios cover success, scope selection, and error paths
- **GIVEN** `tests/cli-contracts/specfact-code-review-run.scenarios.yaml`
- **WHEN** it is validated
- **THEN** it includes success coverage, changed-only and full-review scope examples, subtree-filtered examples, and an error or anti-pattern scenario

#### Scenario: ledger scenarios cover update, status, and reset guardrails
- **GIVEN** `tests/cli-contracts/specfact-code-review-ledger.scenarios.yaml`
- **WHEN** it is validated
- **THEN** it includes `update`, `status`, and reset guardrail coverage

#### Scenario: rules scenarios cover init, show, and update
- **GIVEN** `tests/cli-contracts/specfact-code-review-rules.scenarios.yaml`
- **WHEN** it is validated
- **THEN** it includes the supported rules subcommands
Loading
Loading