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
2 changes: 1 addition & 1 deletion doc/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
2 changes: 1 addition & 1 deletion doc/mcp_interface.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 8 additions & 7 deletions specs/deepwork/jobs/JOBS-REQ-004-quality-review-system.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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

Expand Down
24 changes: 17 additions & 7 deletions src/deepwork/jobs/mcp/quality_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand Down
191 changes: 188 additions & 3 deletions tests/unit/jobs/mcp/test_quality_gate.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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]],
Expand Down Expand Up @@ -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
Expand Down
Loading