diff --git a/doc/architecture.md b/doc/architecture.md index a7ff1fc9..d10db53c 100644 --- a/doc/architecture.md +++ b/doc/architecture.md @@ -1001,7 +1001,7 @@ The quality gate integrates with the DeepWork Reviews infrastructure rather than 3. **Process requirements**: If the step defines `process_requirements`, a review is created that evaluates the `work_summary` against those requirements using RFC 2119 semantics. MUST/SHALL violations cause failure; SHOULD/RECOMMENDED violations fail only if easily achievable; other requirements produce feedback without failure. -4. **Merge with `.deepreview` rules**: The dynamically built rules are merged with any `.deepreview` file-defined rules that match the output files. The changed file list comes from the `outputs` parameter (not git diff). +4. **Merge with `.deepreview` rules**: `.deepreview` file-defined rules and DeepSchema-generated synthetic rules are matched against output files that are *actually changed* (via git diff). Output files that were provided as unchanged references are excluded from `.deepreview` matching. Dynamic rules (from step output `review` blocks) still run against all output files regardless of git status. 5. **Apply review strategies**: Review strategies (`individual`, `matches_together`, etc.) work normally on the merged rule set. diff --git a/doc/mcp_interface.md b/doc/mcp_interface.md index ac8ee00e..ea516956 100644 --- a/doc/mcp_interface.md +++ b/doc/mcp_interface.md @@ -390,7 +390,7 @@ Steps may define quality reviews that outputs must pass. When `finished_step` is 1. JSON schema validation runs first (if any outputs have `json_schema` defined) 2. Dynamic review rules are built from step output `review` blocks and `process_requirements` -3. `.deepreview` rules and DeepSchema-generated synthetic review rules are also loaded and matched against output files +3. `.deepreview` rules and DeepSchema-generated synthetic review rules are loaded and matched against output files that are actually changed (via git diff). Dynamic rules from step `review` blocks run against all output files regardless of git status. 4. If any reviews are needed, `status = "needs_work"` with review instructions 5. If all reviews pass (or no reviews defined), workflow advances 6. There is no maximum attempt limit — the agent can retry `finished_step` indefinitely diff --git a/specs/deepwork/jobs/JOBS-REQ-004-quality-review-system.md b/specs/deepwork/jobs/JOBS-REQ-004-quality-review-system.md index ab68742b..2d9dfa6a 100644 --- a/specs/deepwork/jobs/JOBS-REQ-004-quality-review-system.md +++ b/specs/deepwork/jobs/JOBS-REQ-004-quality-review-system.md @@ -29,7 +29,7 @@ The quality review system evaluates step outputs against defined quality criteri 4. Each rule's `instructions` MUST be prefixed with a preamble containing workflow `common_job_info` and input context (if available). 5. Outputs with no review blocks MUST be skipped. 6. Outputs with `None` values MUST be skipped. -7. Only `file_path` type arguments with actual file paths generate `ReviewRule` objects. +7. Only `file_path` type arguments with actual file paths MUST generate `ReviewRule` objects. ### JOBS-REQ-004.4: Process Requirements @@ -41,12 +41,13 @@ The quality review system evaluates step outputs against defined quality criteri ### JOBS-REQ-004.5: Review Pipeline Integration 1. `run_quality_gate()` MUST load `.deepreview` rules via `load_all_rules()`. -2. `.deepreview` rules MUST be matched against output file paths via `match_files_to_rules()`. -3. Dynamic rules (from step reviews) MUST also be matched via `match_files_to_rules()`. -4. All matched tasks (dynamic + `.deepreview`) MUST be combined. -5. Combined tasks MUST be passed to `write_instruction_files()`, which honors `.passed` marker files. -6. If `write_instruction_files()` returns no task files (all already passed), `run_quality_gate()` MUST return `None`. -7. Remaining task files MUST be formatted via `format_for_claude()`. +2. `.deepreview` rules MUST be matched only against output file paths that also appear in `get_changed_files()` (i.e., files actually modified in git). Unchanged reference files in outputs MUST NOT trigger `.deepreview` rules. +3. Dynamic rules (from step reviews) MUST be matched against all output file paths via `match_files_to_rules()`, regardless of git change status. +4. If `get_changed_files()` fails, `.deepreview` matching MUST be skipped (no `.deepreview` tasks produced). Dynamic rules MUST be unaffected. +5. All matched tasks (dynamic + `.deepreview`) MUST be combined. +6. Combined tasks MUST be passed to `write_instruction_files()`, which honors `.passed` marker files. +7. If `write_instruction_files()` returns no task files (all already passed), `run_quality_gate()` MUST return `None`. +8. Remaining task files MUST be formatted via `format_for_claude()`. ### JOBS-REQ-004.6: Review Guidance Output diff --git a/src/deepwork/jobs/mcp/quality_gate.py b/src/deepwork/jobs/mcp/quality_gate.py index e21d0790..cb655578 100644 --- a/src/deepwork/jobs/mcp/quality_gate.py +++ b/src/deepwork/jobs/mcp/quality_gate.py @@ -26,7 +26,7 @@ from deepwork.review.instructions import ( write_instruction_files, ) -from deepwork.review.matcher import match_files_to_rules +from deepwork.review.matcher import get_changed_files, match_files_to_rules from deepwork.utils.validation import ValidationError, validate_against_schema logger = logging.getLogger("deepwork.jobs.mcp.quality_gate") @@ -328,17 +328,27 @@ def run_quality_gate( schema_rules, _schema_errors = gen_schema_rules(project_root) deepreview_rules.extend(schema_rules) - # 4. Get the "changed files" list = output file paths + # 4. Collect output file paths output_files = _collect_output_file_paths(outputs, job) - # 5. Match .deepreview rules against output files + # 5. Match .deepreview rules against output files that are actually changed. + # Output files may include unchanged reference files — .deepreview rules + # should only fire on files that were actually modified (git diff). deepreview_tasks: list[ReviewTask] = [] if deepreview_rules and output_files: - deepreview_tasks = match_files_to_rules( - output_files, deepreview_rules, project_root, platform - ) + try: + git_changed = get_changed_files(project_root) + except Exception: + git_changed = [] + output_set = set(output_files) + changed_output_files = [f for f in git_changed if f in output_set] + if changed_output_files: + deepreview_tasks = match_files_to_rules( + changed_output_files, deepreview_rules, project_root, platform + ) - # 6. Match dynamic rules against output files + # 6. Match dynamic rules (step-specific reviews) against all output files. + # These are explicitly defined for specific outputs and should always run. dynamic_tasks: list[ReviewTask] = [] if dynamic_rules and output_files: dynamic_tasks = match_files_to_rules(output_files, dynamic_rules, project_root, platform) diff --git a/tests/unit/jobs/mcp/test_quality_gate.py b/tests/unit/jobs/mcp/test_quality_gate.py index b7d9fcb0..959dc5d4 100644 --- a/tests/unit/jobs/mcp/test_quality_gate.py +++ b/tests/unit/jobs/mcp/test_quality_gate.py @@ -518,7 +518,7 @@ def test_returns_feedback_when_json_schema_fails(self, tmp_path: Path) -> None: assert "JSON schema validation failed" in result assert "finished_step" in result - # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.1.3, JOBS-REQ-004.5.7, JOBS-REQ-004.6.1). + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.1.3, JOBS-REQ-004.5.8, JOBS-REQ-004.6.1). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_returns_review_instructions_when_reviews_exist(self, tmp_path: Path) -> None: """When dynamic rules produce tasks, review instructions are returned.""" @@ -573,7 +573,7 @@ def test_returns_review_instructions_when_reviews_exist(self, tmp_path: Path) -> assert "Quality reviews are required" in result assert "step_write_output_report" in result - # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.6). + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.7). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_returns_none_when_all_reviews_already_passed(self, tmp_path: Path) -> None: """If write_instruction_files returns empty (all .passed), result is None.""" @@ -616,7 +616,7 @@ def test_returns_none_when_all_reviews_already_passed(self, tmp_path: Path) -> N assert result is None - # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.4). + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.5). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_merges_deepreview_and_dynamic_tasks(self, tmp_path: Path) -> None: """Both .deepreview rules and dynamic rules are processed together.""" @@ -664,6 +664,10 @@ def test_merges_deepreview_and_dynamic_tasks(self, tmp_path: Path) -> None: "deepwork.jobs.mcp.quality_gate.load_all_rules", return_value=([deepreview_rule], []), ), + patch( + "deepwork.jobs.mcp.quality_gate.get_changed_files", + return_value=["report.md"], + ), patch( "deepwork.jobs.mcp.quality_gate.match_files_to_rules", side_effect=[[deepreview_task], [dynamic_task]], @@ -697,6 +701,187 @@ def test_merges_deepreview_and_dynamic_tasks(self, tmp_path: Path) -> None: assert all_tasks[1].rule_name == "external_rule" assert result is not None + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_deepreview_rules_skip_unchanged_output_files(self, tmp_path: Path) -> None: + """Deepreview rules should only match output files that are actually changed in git.""" + arg = StepArgument(name="refs", description="Reference files", type="file_path") + output_ref = StepOutputRef(argument_name="refs", required=False) + step = WorkflowStep(name="explore", outputs={"refs": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + deepreview_rule = ReviewRule( + name="python_lint", + description="Lint Python files", + include_patterns=["**/*.py"], + exclude_patterns=[], + strategy="matches_together", + instructions="Run linting", + agent=None, + all_changed_filenames=False, + unchanged_matching_files=False, + precomputed_info_bash_command=None, + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=1, + ) + + with ( + patch( + "deepwork.jobs.mcp.quality_gate.load_all_rules", + return_value=([deepreview_rule], []), + ), + # git says no files changed — output files are just references + patch( + "deepwork.jobs.mcp.quality_gate.get_changed_files", + return_value=[], + ), + patch( + "deepwork.jobs.mcp.quality_gate.match_files_to_rules", + ) as mock_match, + ): + result = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"refs": ["src/foo.py", "src/bar.py"]}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + + # match_files_to_rules should not be called for deepreview since + # no output files are in the git changed set + assert mock_match.call_count == 0 + assert result is None + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_dynamic_rules_match_all_outputs_regardless_of_git(self, tmp_path: Path) -> None: + """Dynamic rules run against all output files even when git says nothing changed.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + arg = StepArgument(name="report", description="Report", type="file_path") + output_ref = StepOutputRef(argument_name="report", required=True, review=review) + step = WorkflowStep(name="write", outputs={"report": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + dynamic_task = ReviewTask( + rule_name="step_write_output_report", + files_to_review=["report.md"], + instructions="Check it", + agent_name=None, + ) + instruction_path = tmp_path / ".deepwork" / "tmp" / "instr.md" + instruction_path.parent.mkdir(parents=True, exist_ok=True) + instruction_path.write_text("content") + + with ( + patch( + "deepwork.jobs.mcp.quality_gate.load_all_rules", + return_value=([], []), + ), + patch( + "deepwork.jobs.mcp.quality_gate.match_files_to_rules", + return_value=[dynamic_task], + ) as mock_match, + patch( + "deepwork.jobs.mcp.quality_gate.write_instruction_files", + return_value=[(dynamic_task, instruction_path)], + ), + patch( + "deepwork.jobs.mcp.quality_gate.format_for_claude", + return_value="formatted", + ), + ): + result = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"report": "report.md"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + + # Dynamic rules matched even though no deepreview rules exist and + # get_changed_files was never called (no deepreview rules to trigger it) + mock_match.assert_called_once() + assert result is not None + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.5.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_deepreview_skipped_when_get_changed_files_fails(self, tmp_path: Path) -> None: + """If get_changed_files() fails, .deepreview matching is skipped; dynamic rules unaffected.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + arg = StepArgument(name="report", description="Report", type="file_path") + output_ref = StepOutputRef(argument_name="report", required=True, review=review) + step = WorkflowStep(name="write", outputs={"report": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + deepreview_rule = ReviewRule( + name="lint", + description="Lint", + include_patterns=["*.md"], + exclude_patterns=[], + strategy="individual", + instructions="Lint it", + agent=None, + all_changed_filenames=False, + unchanged_matching_files=False, + precomputed_info_bash_command=None, + source_dir=tmp_path, + source_file=tmp_path / ".deepreview", + source_line=1, + ) + + dynamic_task = ReviewTask( + rule_name="step_write_output_report", + files_to_review=["report.md"], + instructions="Check it", + agent_name=None, + ) + instruction_path = tmp_path / ".deepwork" / "tmp" / "instr.md" + instruction_path.parent.mkdir(parents=True, exist_ok=True) + instruction_path.write_text("content") + + from deepwork.review.matcher import GitDiffError + + with ( + patch( + "deepwork.jobs.mcp.quality_gate.load_all_rules", + return_value=([deepreview_rule], []), + ), + patch( + "deepwork.jobs.mcp.quality_gate.get_changed_files", + side_effect=GitDiffError("git not available"), + ), + patch( + "deepwork.jobs.mcp.quality_gate.match_files_to_rules", + return_value=[dynamic_task], + ) as mock_match, + patch( + "deepwork.jobs.mcp.quality_gate.write_instruction_files", + return_value=[(dynamic_task, instruction_path)], + ), + patch( + "deepwork.jobs.mcp.quality_gate.format_for_claude", + return_value="formatted", + ), + ): + result = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"report": "report.md"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + + # match_files_to_rules called only once (for dynamic rules, not deepreview) + mock_match.assert_called_once() + assert result is not None + # --------------------------------------------------------------------------- # TestValidateJsonSchemas — additional coverage