diff --git a/.deepwork/jobs/test_job/fixtures/report.md b/.deepwork/jobs/test_job/fixtures/report.md new file mode 100644 index 00000000..9c3036fe --- /dev/null +++ b/.deepwork/jobs/test_job/fixtures/report.md @@ -0,0 +1,4 @@ +# Test Job Report + +This is a short markdown report produced by the test_job workflow to exercise +the file_path review path of the DeepWork quality-gate review system. diff --git a/.deepwork/jobs/test_job/job.yml b/.deepwork/jobs/test_job/job.yml new file mode 100644 index 00000000..cdf175a2 --- /dev/null +++ b/.deepwork/jobs/test_job/job.yml @@ -0,0 +1,74 @@ +name: test_job +summary: "Exercise the DeepWork quality-gate review system with string and file_path outputs to verify string-output reviews run" + +step_arguments: + - name: report_file + description: "A short markdown report file produced by the step — exercises the file_path review path." + type: file_path + + - name: summary_text + description: "A 1-2 sentence narrative summary of the step's work — exercises a string output review defined at the step output-ref level." + type: string + + - name: metrics_text + description: "A concise metrics blurb containing at least one numeric value — exercises a string output review defined at the step_argument level (inherited by any step that outputs it)." + type: string + review: + strategy: individual + instructions: | + Verify the value is a concise metrics summary of 1-3 sentences that contains at + least one numeric value. Flag it if the value is empty, a placeholder, or does + not mention any numbers. + + - name: notes_text + description: "A freeform notes string with no review attached — control case that should produce no review task." + type: string + +workflows: + test_review_system: + summary: "Produce four outputs of mixed types and review levels to verify the quality gate reviews them correctly." + common_job_info_provided_to_all_steps_at_runtime: | + This bespoke test job verifies that the DeepWork quality-gate review system + executes reviews on string outputs as well as file_path outputs. Four outputs are + produced in a single step, each hitting a distinct review code path. The control + case (notes_text) must NOT produce a review task. + steps: + - name: produce_outputs + instructions: | + # Produce test outputs for the quality gate + + Create each of the four outputs below, then call `finished_step` with all four + values. Keep the content short. + + 1. **report_file**: write a small markdown file at + `.deepwork/jobs/test_job/fixtures/report.md` (create the parent directory + first if needed). Two or three lines — a heading plus a sentence is fine. + + 2. **summary_text**: a 1-2 sentence narrative summary describing the work + performed in this step. + + 3. **metrics_text**: a 1-3 sentence metrics blurb that includes at least one + number (e.g., "Produced 4 outputs, 3 reviewed, in under 1 second."). + + 4. **notes_text**: any freeform string — this one has no review attached. + + Expected quality-gate result: three review tasks (one per output that has a + review configured). `notes_text` should not produce a review task. + outputs: + report_file: + review: + strategy: individual + instructions: | + Verify the file is a valid markdown document with a recognisable heading + or non-empty first line. Flag it if the file is empty or contains no + markdown structure. + summary_text: + review: + strategy: individual + instructions: | + Verify the value is a non-empty narrative summary of 1-2 sentences + written in plain English that describes some concrete outcome of the + step. Flag it if the value is empty, a placeholder, or does not describe + anything. + metrics_text: {} + notes_text: {} diff --git a/CHANGELOG.md b/CHANGELOG.md index 0a96e2a8..6afde9b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - DeepSchema PostToolUse hook (`deepschema_write`) no longer reports `File is not valid JSON` for YAML files whose name has no extension (e.g. `.deepreview`). The hook now parses target files and the referenced JSON Schema as YAML, which is a superset of JSON, so both formats are accepted regardless of file extension. DW-REQ-011.7.3 updated to match. (Mirrors the fix shipped in #338 for the workflow quality gate.) +- `review:` blocks declared on `type: string` step outputs are now actually executed. Previously they were silently dropped because the review pipeline only matched against file paths, leaving authors with misconfigured-but-silent quality gates. String output reviews now produce synthetic `ReviewTask` objects with the string value carried on a new `ReviewTask.inline_content` field and rendered into the instruction file as a "Content to Review" section. New requirements: JOBS-REQ-004.8, REVIEW-REQ-005.1.8, REVIEW-REQ-009.1.7. (#350) ### Removed ## [0.13.1] - 2026-04-06 diff --git a/Makefile b/Makefile index cb29a792..93f3d6db 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,17 @@ +# `make lint` mirrors CI (.github/workflows/validate.yml) in check-only mode +# so that a clean local run guarantees CI will pass. Use `make lint-fix` to +# auto-fix formatter and linter issues locally. + +.PHONY: lint lint-fix + lint: - @echo "## make lint output" + @echo "## make lint output (check-only — matches CI)" + uv run ruff format --check src/ tests/ + uv run ruff check src/ tests/ + uv run mypy src/ + +lint-fix: + @echo "## make lint-fix output (auto-fix)" uv run ruff format src/ tests/ uv run ruff check --fix src/ tests/ uv run mypy src/ 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 e26de88b..7d5b7408 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 MUST generate `ReviewRule` objects. +7. Only `file_path` type arguments with actual file paths MUST generate `ReviewRule` objects. Reviews declared on `type: string` outputs are handled separately per JOBS-REQ-004.8. ### JOBS-REQ-004.4: Process Requirements @@ -64,3 +64,12 @@ The quality review system evaluates step outputs against defined quality criteri 3. For `string` inputs, values MUST be shown inline. 4. Inputs with no value MUST be shown as "not available". 5. If the step has no inputs, an empty string MUST be returned. + +### JOBS-REQ-004.8: String Output Review Tasks + +1. When a step output has `type: string` and a review block (either on the `StepOutputRef` or inherited from the `StepArgument`), `run_quality_gate()` MUST produce a synthetic `ReviewTask` whose content-to-review is the string value itself. Such reviews MUST NOT be silently ignored. +2. The synthetic task MUST bypass file-pattern matching; the string value MUST be carried on `ReviewTask.inline_content` so the reviewer agent sees it inlined in its instruction file (per REVIEW-REQ-005.1.8). +3. If a string output has reviews at both the `StepOutputRef` level and the `StepArgument` level, both MUST produce separate synthetic tasks, suffixed `_arg` for the argument-level task to match the file_path naming convention. +4. The synthetic task's instructions MUST be prefixed with the same preamble used for file-based reviews (workflow `common_job_info` and input context). +5. String outputs with `None` values MUST be skipped. +6. The synthetic task's `review_id` MUST incorporate the string value into its content hash so that distinct string values produce distinct cache keys (per REVIEW-REQ-009.1.7). diff --git a/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md b/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md index 974c8e39..4c36a702 100644 --- a/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md +++ b/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md @@ -11,10 +11,11 @@ For each `ReviewTask`, the system generates a self-contained markdown instructio 1. Each instruction file MUST be a valid markdown document. 2. The file MUST begin with a heading identifying the review rule and scope (e.g., `# Review: python_file_best_practices — src/app.py`). 3. The file MUST contain a "Review Instructions" section with the rule's resolved instruction text. -4. The file MUST contain a "Files to Review" section listing the file paths to examine. +4. The file MUST contain a "Files to Review" section listing the file paths to examine when the task has at least one file to review. Inline-content tasks (see REVIEW-REQ-005.1.8) MUST NOT include a "Files to Review" section. 5. File paths in the "Files to Review" section MUST be relative to the repository root. 6. When the task has `additional_files` (unchanged matching files), the file MUST contain an "Unchanged Matching Files" section listing those file paths. 7. When the task has `all_changed_filenames`, the file MUST contain an "All Changed Files" section listing every changed filename for context. +8. When the task has `inline_content` set (used for `type: string` step outputs — see JOBS-REQ-004.8), the file MUST contain a "Content to Review" section whose body is the inline content verbatim. The review heading scope MUST read `inline content` when the task has `inline_content` and no `files_to_review`. ### REVIEW-REQ-005.2: File Path Formatting diff --git a/specs/deepwork/review/REVIEW-REQ-009-review-pass-caching.md b/specs/deepwork/review/REVIEW-REQ-009-review-pass-caching.md index c8caff8b..9a8c2d31 100644 --- a/specs/deepwork/review/REVIEW-REQ-009-review-pass-caching.md +++ b/specs/deepwork/review/REVIEW-REQ-009-review-pass-caching.md @@ -20,6 +20,7 @@ The mechanism relies on a deterministic `review_id` that encodes the rule name, 4. The content hash MUST be the first 12 hex characters of the SHA-256 digest of the concatenated contents of all files to review, with files sorted alphabetically before concatenation. 5. Files that cannot be read MUST contribute the placeholder string `MISSING` instead of their contents. 6. The same inputs (rule name, file paths, file contents) MUST always produce the same `review_id`. +7. For inline-content tasks (where `files_to_review` is empty and `inline_content` is set — used for `type: string` step outputs per JOBS-REQ-004.8), the file paths component MUST be the literal `inline` and the content hash MUST be derived from the inline string value so that distinct string values produce distinct `review_id`s. ### REVIEW-REQ-009.2: `mark_review_as_passed` MCP Tool diff --git a/src/deepwork/jobs/mcp/quality_gate.py b/src/deepwork/jobs/mcp/quality_gate.py index cb655578..26edcf6c 100644 --- a/src/deepwork/jobs/mcp/quality_gate.py +++ b/src/deepwork/jobs/mcp/quality_gate.py @@ -92,6 +92,27 @@ def _collect_output_file_paths( return paths +def _build_preamble( + step: WorkflowStep, + job: JobDefinition, + workflow: Workflow, + input_values: dict[str, ArgumentValue], +) -> str: + """Build the preamble prefixed to every dynamic review's instructions. + + Combines workflow ``common_job_info`` and the rendered step inputs. + Returns an empty string when neither is available. + """ + input_context = _build_input_context(step, job, input_values) + common_info = workflow.common_job_info or "" + preamble_parts: list[str] = [] + if common_info: + preamble_parts.append(f"## Job Context\n\n{common_info}") + if input_context: + preamble_parts.append(input_context) + return "\n\n".join(preamble_parts) + + def _build_input_context( step: WorkflowStep, job: JobDefinition, @@ -145,16 +166,7 @@ def build_dynamic_review_rules( targets. """ rules: list[ReviewRule] = [] - input_context = _build_input_context(step, job, input_values) - common_info = workflow.common_job_info or "" - - # Build preamble with common info and inputs - preamble_parts: list[str] = [] - if common_info: - preamble_parts.append(f"## Job Context\n\n{common_info}") - if input_context: - preamble_parts.append(input_context) - preamble = "\n\n".join(preamble_parts) + preamble = _build_preamble(step, job, workflow, input_values) # Process each output for output_name, output_ref in step.outputs.items(): @@ -289,6 +301,83 @@ def build_dynamic_review_rules( return rules +def build_string_output_review_tasks( + step: WorkflowStep, + job: JobDefinition, + workflow: Workflow, + outputs: dict[str, ArgumentValue], + input_values: dict[str, ArgumentValue], + project_root: Path, + platform: str = "claude", +) -> list[ReviewTask]: + """Build synthetic ReviewTasks for ``type: string`` outputs with review blocks. + + String outputs have no file to match against, so they cannot flow + through the normal ReviewRule/match_files_to_rules pipeline. Instead, + this function creates ``ReviewTask`` objects directly, with the string + value carried on ``inline_content`` so the reviewer agent sees it + inlined in its instruction file. + + Both output-ref-level and step_argument-level review blocks are + honored; when both are present, the argument-level task is suffixed + ``_arg`` to distinguish it (matching the file_path rule naming). + """ + tasks: list[ReviewTask] = [] + preamble = _build_preamble(step, job, workflow, input_values) + + try: + source_rel = (job.job_dir / "job.yml").relative_to(project_root) + except ValueError: + source_rel = job.job_dir / "job.yml" + source_location = f"{source_rel}:0" + + for output_name, output_ref in step.outputs.items(): + arg = job.get_argument(output_name) + if not arg or arg.type != "string": + continue + + review_blocks: list[ReviewBlock] = [] + if output_ref.review: + review_blocks.append(output_ref.review) + if arg.review: + review_blocks.append(arg.review) + + if not review_blocks: + continue + + value = outputs.get(output_name) + if value is None: + continue + # Defensive: string outputs should always be str, but the runtime + # value may technically be any ArgumentValue. + inline_value = value if isinstance(value, str) else str(value) + + for i, review_block in enumerate(review_blocks): + full_instructions = ( + f"{preamble}\n\n{review_block.instructions}" + if preamble + else review_block.instructions + ) + suffix = "_arg" if i > 0 else "" + rule_name = f"step_{step.name}_output_{output_name}{suffix}" + agent_name: str | None = None + if review_block.agent is not None: + agent_name = review_block.agent.get(platform) + + tasks.append( + ReviewTask( + rule_name=rule_name, + files_to_review=[], + instructions=full_instructions, + agent_name=agent_name, + source_location=source_location, + inline_content=inline_value, + ) + ) + + return tasks + + def run_quality_gate( step: WorkflowStep, job: JobDefinition, @@ -321,6 +410,19 @@ def run_quality_gate( project_root=project_root, ) + # 2b. Build synthetic ReviewTasks for type: string outputs with review blocks. + # These bypass file-pattern matching entirely — the string value is + # carried on the task via inline_content so the reviewer sees it inline. + string_output_tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs=outputs, + input_values=input_values, + project_root=project_root, + platform=platform, + ) + # 3. Load .deepreview rules deepreview_rules, _errors = load_all_rules(project_root) @@ -354,7 +456,7 @@ def run_quality_gate( dynamic_tasks = match_files_to_rules(output_files, dynamic_rules, project_root, platform) # 7. Combine all tasks - all_tasks = dynamic_tasks + deepreview_tasks + all_tasks = dynamic_tasks + string_output_tasks + deepreview_tasks if not all_tasks: return None diff --git a/src/deepwork/review/config.py b/src/deepwork/review/config.py index 7a09816f..2c59fcb9 100644 --- a/src/deepwork/review/config.py +++ b/src/deepwork/review/config.py @@ -51,6 +51,7 @@ class ReviewTask: additional_files: list[str] = field(default_factory=list) # Unchanged matching files all_changed_filenames: list[str] | None = None precomputed_info_bash_command: str | None = None # Resolved command to run + inline_content: str | None = None # Inline string value for type: string outputs def parse_deepreview_file(filepath: Path) -> list[ReviewRule]: diff --git a/src/deepwork/review/formatter.py b/src/deepwork/review/formatter.py index 0df4a640..ce38a774 100644 --- a/src/deepwork/review/formatter.py +++ b/src/deepwork/review/formatter.py @@ -137,6 +137,9 @@ def _task_name(task: ReviewTask) -> str: rule comes from a subdirectory .deepreview file. This disambiguates same-named rules from different directories (REVIEW-REQ-004.10). + For inline-content tasks (type: string step outputs per JOBS-REQ-004.8) + the scope reads ``inline content`` instead of a file count. + Args: task: The ReviewTask to name. @@ -144,6 +147,8 @@ def _task_name(task: ReviewTask) -> str: Task name string. """ prefix = _scope_prefix(task) + if not task.files_to_review and task.inline_content is not None: + return f"{prefix}{task.rule_name} review of inline content" if len(task.files_to_review) == 1: return f"{prefix}{task.rule_name} review of {task.files_to_review[0]}" return f"{prefix}{task.rule_name} review of {len(task.files_to_review)} files" diff --git a/src/deepwork/review/instructions.py b/src/deepwork/review/instructions.py index eea25655..76b7b389 100644 --- a/src/deepwork/review/instructions.py +++ b/src/deepwork/review/instructions.py @@ -24,6 +24,10 @@ def compute_review_id(task: ReviewTask, project_root: Path) -> str: Format: ``{sanitized_rule}--{sanitized_paths}--{content_hash_12}``. + For tasks with ``inline_content`` (type: string outputs), the paths + component is the literal ``"inline"`` and the content hash is derived + from the inline string value. + Args: task: The ReviewTask to compute an ID for. project_root: Absolute path to the project root. @@ -33,7 +37,7 @@ def compute_review_id(task: ReviewTask, project_root: Path) -> str: """ rule_part = _sanitize_for_id(task.rule_name) paths_part = _paths_component(task.files_to_review) - hash_part = _content_hash(task.files_to_review, project_root) + hash_part = _content_hash(task.files_to_review, project_root, task.inline_content) return f"{rule_part}--{paths_part}--{hash_part}" @@ -47,8 +51,11 @@ def _paths_component(files: list[str]) -> str: Each path has ``/`` replaced with ``-``. Multiple paths are sorted alphabetically, then joined with ``_AND_``. If the result exceeds - 100 characters, falls back to ``{N}_files``. + 100 characters, falls back to ``{N}_files``. When ``files`` is + empty (inline-content tasks), returns the literal ``"inline"``. """ + if not files: + return "inline" sanitized = sorted(f.replace("/", "-") for f in files) joined = "_AND_".join(sanitized) if len(joined) > 100: @@ -56,11 +63,14 @@ def _paths_component(files: list[str]) -> str: return joined -def _content_hash(files: list[str], project_root: Path) -> str: - """SHA-256 content hash (first 12 hex chars) of the given files. +def _content_hash(files: list[str], project_root: Path, inline_content: str | None = None) -> str: + """SHA-256 content hash (first 12 hex chars) of the task content. Files are sorted alphabetically before concatenation. Files that - cannot be read contribute the placeholder ``MISSING``. + cannot be read contribute the placeholder ``MISSING``. When + ``inline_content`` is provided, it is mixed into the hash (via a + sentinel marker) so that each distinct string value produces a + distinct review ID. """ h = hashlib.sha256() for filepath in sorted(files): @@ -69,6 +79,9 @@ def _content_hash(files: list[str], project_root: Path) -> str: except (OSError, UnicodeDecodeError): content = "MISSING" h.update(content.encode("utf-8")) + if inline_content is not None: + h.update(b"\x00INLINE\x00") + h.update(inline_content.encode("utf-8")) return h.hexdigest()[:12] @@ -219,11 +232,18 @@ def build_instruction_file( parts.append(task.instructions.strip()) parts.append("") - # Files to review - parts.append("## Files to Review\n") - for filepath in task.files_to_review: - parts.append(f"- @{filepath}") - parts.append("") + # Files to review (omitted for inline-content tasks with no files) + if task.files_to_review: + parts.append("## Files to Review\n") + for filepath in task.files_to_review: + parts.append(f"- @{filepath}") + parts.append("") + + # Inline content to review (for type: string outputs) + if task.inline_content is not None: + parts.append("## Content to Review\n") + parts.append(task.inline_content.rstrip()) + parts.append("") # Additional context: unchanged matching files if task.additional_files: @@ -280,6 +300,8 @@ def _describe_scope(task: ReviewTask) -> str: Returns: Scope description string. """ + if not task.files_to_review and task.inline_content is not None: + return "inline content" if len(task.files_to_review) == 1: return task.files_to_review[0] return f"{len(task.files_to_review)} files" diff --git a/tests/unit/jobs/mcp/test_quality_gate.py b/tests/unit/jobs/mcp/test_quality_gate.py index dacdf06c..32a38540 100644 --- a/tests/unit/jobs/mcp/test_quality_gate.py +++ b/tests/unit/jobs/mcp/test_quality_gate.py @@ -14,6 +14,7 @@ from deepwork.jobs.mcp.quality_gate import ( build_dynamic_review_rules, + build_string_output_review_tasks, run_quality_gate, validate_json_schemas, ) @@ -1076,8 +1077,14 @@ def test_skips_output_with_none_value(self, tmp_path: Path) -> None: ) assert rules == [] - def test_string_output_with_review_produces_no_file_rule(self, tmp_path: Path) -> None: - """String-type output with a review block has empty file_paths.""" + def test_string_output_with_review_produces_no_rule_in_build_dynamic( + self, tmp_path: Path + ) -> None: + """String-type outputs do not produce ReviewRule objects. + + ReviewRule is for file-pattern matching; string outputs flow through + build_string_output_review_tasks() instead (see JOBS-REQ-004.8). + """ review = ReviewBlock(strategy="individual", instructions="Check summary") arg = StepArgument(name="summary", description="Summary", type="string") output_ref = StepOutputRef(argument_name="summary", required=True, review=review) @@ -1093,7 +1100,7 @@ def test_string_output_with_review_produces_no_file_rule(self, tmp_path: Path) - work_summary=None, project_root=tmp_path, ) - # String type has empty file_paths, so no rule is created + # String type has empty file_paths, so no ReviewRule is created assert rules == [] def test_process_requirements_with_list_file_and_string_outputs(self, tmp_path: Path) -> None: @@ -1179,6 +1186,364 @@ def test_no_deepreview_rules_and_no_output_files_skips_matching(self, tmp_path: assert result is None +class TestBuildStringOutputReviewTasks: + """Tests for build_string_output_review_tasks — validates JOBS-REQ-004.8.""" + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.1, JOBS-REQ-004.8.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_output_level_review_produces_inline_task(self, tmp_path: Path) -> None: + """A review block on a string output-ref produces a synthetic ReviewTask.""" + review = ReviewBlock(strategy="individual", instructions="Check the summary") + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "I did the research and found X."}, + input_values={}, + project_root=tmp_path, + ) + + assert len(tasks) == 1 + task = tasks[0] + assert task.rule_name == "step_write_output_summary" + assert task.files_to_review == [] + assert task.inline_content == "I did the research and found X." + assert "Check the summary" in task.instructions + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_argument_level_review_produces_inline_task(self, tmp_path: Path) -> None: + """A review block on the step_argument (not the output-ref) produces a task.""" + arg_review = ReviewBlock(strategy="matches_together", instructions="Verify the note") + arg = StepArgument(name="note", description="Note", type="string", review=arg_review) + output_ref = StepOutputRef(argument_name="note", required=True) + step = WorkflowStep(name="write", outputs={"note": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"note": "all good"}, + input_values={}, + project_root=tmp_path, + ) + + assert len(tasks) == 1 + # When only arg-level review exists (index 0), no _arg suffix + assert tasks[0].rule_name == "step_write_output_note" + assert tasks[0].inline_content == "all good" + assert "Verify the note" in tasks[0].instructions + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.3). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_both_output_and_arg_level_reviews_produce_separate_tasks(self, tmp_path: Path) -> None: + """Output-level and arg-level reviews on the same string output both execute.""" + output_review = ReviewBlock(strategy="individual", instructions="Output check") + arg_review = ReviewBlock(strategy="individual", instructions="Arg check") + arg = StepArgument(name="summary", description="Summary", type="string", review=arg_review) + output_ref = StepOutputRef(argument_name="summary", required=True, review=output_review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "the value"}, + input_values={}, + project_root=tmp_path, + ) + + assert len(tasks) == 2 + assert tasks[0].rule_name == "step_write_output_summary" + assert tasks[1].rule_name == "step_write_output_summary_arg" + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_preamble_includes_common_job_info_and_inputs(self, tmp_path: Path) -> None: + """The synthetic task's instructions include common_job_info and step inputs.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + input_arg = StepArgument(name="topic", description="Topic", type="string") + output_arg = StepArgument(name="summary", description="Summary", type="string") + input_ref = StepInputRef(argument_name="topic", required=True) + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep( + name="write", + inputs={"topic": input_ref}, + outputs={"summary": output_ref}, + ) + workflow = Workflow( + name="main", + summary="Test", + steps=[step], + common_job_info="This job analyses narrative summaries.", + ) + job = JobDefinition( + name="test_job", + summary="Test", + step_arguments=[input_arg, output_arg], + workflows={"main": workflow}, + job_dir=tmp_path / ".deepwork" / "jobs" / "test_job", + ) + job.job_dir.mkdir(parents=True, exist_ok=True) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "done"}, + input_values={"topic": "AI safety"}, + project_root=tmp_path, + ) + + assert len(tasks) == 1 + assert "narrative summaries" in tasks[0].instructions + assert "topic" in tasks[0].instructions + assert "AI safety" in tasks[0].instructions + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.5). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_none_value_skipped(self, tmp_path: Path) -> None: + """String outputs without a value do not produce synthetic tasks.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={}, # no value for "summary" + input_values={}, + project_root=tmp_path, + ) + assert tasks == [] + + def test_file_path_outputs_are_ignored(self, tmp_path: Path) -> None: + """file_path outputs do not flow through this function.""" + 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) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"report": "report.md"}, + input_values={}, + project_root=tmp_path, + ) + assert tasks == [] + + def test_no_review_block_skipped(self, tmp_path: Path) -> None: + """String outputs without any review block produce no tasks.""" + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True) # no review + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "value"}, + input_values={}, + project_root=tmp_path, + ) + assert tasks == [] + + def test_agent_name_resolved_from_review_block(self, tmp_path: Path) -> None: + """When the review block specifies an agent for the target platform, it flows through.""" + review = ReviewBlock( + strategy="individual", + instructions="Check it", + agent={"claude": "string-reviewer"}, + ) + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "value"}, + input_values={}, + project_root=tmp_path, + platform="claude", + ) + assert len(tasks) == 1 + assert tasks[0].agent_name == "string-reviewer" + + def test_job_dir_outside_project_root_falls_back(self, tmp_path: Path) -> None: + """When job.job_dir is not under project_root, source_location falls back to the absolute path.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + # job.job_dir lives in a sibling directory, NOT under tmp_path + outside = tmp_path.parent / f"outside_{tmp_path.name}" + outside.mkdir() + workflow = Workflow(name="main", summary="Test", steps=[step]) + job = JobDefinition( + name="test_job", + summary="Test", + step_arguments=[arg], + workflows={"main": workflow}, + job_dir=outside / ".deepwork" / "jobs" / "test_job", + ) + + tasks = build_string_output_review_tasks( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "value"}, + input_values={}, + project_root=tmp_path, + ) + assert len(tasks) == 1 + # source_location falls back to the absolute job.yml path + assert str(outside) in tasks[0].source_location + assert tasks[0].source_location.endswith("job.yml:0") + + +class TestRunQualityGateStringOutputs: + """End-to-end tests: run_quality_gate emits reviews for string outputs. + + Validates JOBS-REQ-004.8.1 — string output reviews are not silently dropped. + """ + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_string_output_review_produces_instructions(self, tmp_path: Path) -> None: + """run_quality_gate returns guidance when a string output has a review block.""" + review = ReviewBlock(strategy="individual", instructions="Check the summary value") + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + with patch("deepwork.jobs.mcp.quality_gate.load_all_rules", return_value=([], [])): + result = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "processed 42 documents"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + + assert result is not None + assert "Quality reviews are required" in result + + # The generated instruction file must include the string value + instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" + md_files = list(instructions_dir.glob("*.md")) + assert len(md_files) == 1 + body = md_files[0].read_text() + assert "## Content to Review" in body + assert "processed 42 documents" in body + assert "Check the summary value" in body + # No Files to Review section for inline-content tasks + assert "## Files to Review" not in body + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_passed_marker_skips_string_review_on_same_value(self, tmp_path: Path) -> None: + """A .passed marker for an inline review is honored when the value is unchanged.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + # First run: produces a task and writes the instruction file. + with patch("deepwork.jobs.mcp.quality_gate.load_all_rules", return_value=([], [])): + first = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "cached value"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + assert first is not None + + # Drop a .passed marker next to the instruction file. + instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" + md_files = list(instructions_dir.glob("*.md")) + assert len(md_files) == 1 + review_id = md_files[0].stem + (instructions_dir / f"{review_id}.passed").write_bytes(b"") + + # Second run with the same value — should be skipped by the cache. + with patch("deepwork.jobs.mcp.quality_gate.load_all_rules", return_value=([], [])): + second = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "cached value"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + assert second is None + + # THIS TEST VALIDATES A HARD REQUIREMENT (JOBS-REQ-004.8.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_changed_string_value_invalidates_cache(self, tmp_path: Path) -> None: + """Changing the string value produces a new review_id so the cache is bypassed.""" + review = ReviewBlock(strategy="individual", instructions="Check it") + arg = StepArgument(name="summary", description="Summary", type="string") + output_ref = StepOutputRef(argument_name="summary", required=True, review=review) + step = WorkflowStep(name="write", outputs={"summary": output_ref}) + job, workflow = _make_job(tmp_path, [arg], step) + + # Run with value A and mark as passed. + with patch("deepwork.jobs.mcp.quality_gate.load_all_rules", return_value=([], [])): + run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "value A"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" + md_files = list(instructions_dir.glob("*.md")) + assert len(md_files) == 1 + review_id_a = md_files[0].stem + (instructions_dir / f"{review_id_a}.passed").write_bytes(b"") + + # Run with value B — different review_id, so review runs again. + with patch("deepwork.jobs.mcp.quality_gate.load_all_rules", return_value=([], [])): + result = run_quality_gate( + step=step, + job=job, + workflow=workflow, + outputs={"summary": "value B"}, + input_values={}, + work_summary=None, + project_root=tmp_path, + ) + assert result is not None + assert "Quality reviews are required" in result + + class TestQualityGatePassCaching: """Tests for JOBS-REQ-004.5.7: quality gate skips reviews with .passed markers.""" diff --git a/tests/unit/review/test_formatter.py b/tests/unit/review/test_formatter.py index 20478709..3d7232d9 100644 --- a/tests/unit/review/test_formatter.py +++ b/tests/unit/review/test_formatter.py @@ -56,6 +56,20 @@ def test_grouped_task_name_includes_file_count(self, tmp_path: Path) -> None: result = format_for_claude([(task, file_path)], tmp_path) assert 'name: "py_review review of 3 files"' in result + def test_inline_content_task_name_says_inline_content(self, tmp_path: Path) -> None: + """Inline-content tasks render as "review of inline content", not "0 files".""" + task = ReviewTask( + rule_name="string_rule", + files_to_review=[], + instructions="Review the value.", + agent_name=None, + inline_content="the value", + ) + file_path = tmp_path / "instructions.md" + result = format_for_claude([(task, file_path)], tmp_path) + assert 'name: "string_rule review of inline content"' in result + assert "0 files" not in result + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-006.3.3b). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_default_subagent_type_when_no_agent(self, tmp_path: Path) -> None: diff --git a/tests/unit/review/test_instructions.py b/tests/unit/review/test_instructions.py index 924beb79..0a3ecd3c 100644 --- a/tests/unit/review/test_instructions.py +++ b/tests/unit/review/test_instructions.py @@ -420,6 +420,82 @@ def test_special_chars_in_rule_name_sanitized(self, tmp_path: Path) -> None: assert _sanitize_for_id("rule_name-1.0") == "rule_name-1.0" +class TestInlineContent: + """Tests for inline-content review tasks — validates REVIEW-REQ-005.1.8 and REVIEW-REQ-009.1.7. + + Inline-content tasks are used for type: string step outputs (see JOBS-REQ-004.8). + They have no files_to_review and carry the value to review on inline_content. + """ + + def _make_inline_task(self, content: str, rule_name: str = "string_rule") -> ReviewTask: + return ReviewTask( + rule_name=rule_name, + files_to_review=[], + instructions="Review the inline content.", + agent_name=None, + source_location=".deepreview:1", + inline_content=content, + ) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.8). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_includes_content_to_review_section(self) -> None: + task = self._make_inline_task("hello world") + content = build_instruction_file(task) + assert "## Content to Review" in content + assert "hello world" in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.4). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_omits_files_to_review_section_when_no_files(self) -> None: + task = self._make_inline_task("a value") + content = build_instruction_file(task) + assert "## Files to Review" not in content + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.8). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_scope_heading_is_inline_content(self) -> None: + task = self._make_inline_task("anything") + content = build_instruction_file(task) + assert "# Review: string_rule — inline content" in content.split("\n")[0] + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_review_id_uses_inline_marker(self, tmp_path: Path) -> None: + task = self._make_inline_task("value a") + review_id = compute_review_id(task, tmp_path) + parts = review_id.split("--") + assert len(parts) == 3 + assert parts[0] == "string_rule" + assert parts[1] == "inline" + assert len(parts[2]) == 12 + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_distinct_inline_values_produce_distinct_ids(self, tmp_path: Path) -> None: + task_a = self._make_inline_task("value a") + task_b = self._make_inline_task("value b") + assert compute_review_id(task_a, tmp_path) != compute_review_id(task_b, tmp_path) + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_same_inline_value_produces_same_id(self, tmp_path: Path) -> None: + task_a = self._make_inline_task("same value") + task_b = self._make_inline_task("same value") + assert compute_review_id(task_a, tmp_path) == compute_review_id(task_b, tmp_path) + + def test_pass_caching_works_for_inline_content(self, tmp_path: Path) -> None: + """Writing a .passed marker for an inline task skips it next time.""" + task = self._make_inline_task("cache me") + review_id = compute_review_id(task, tmp_path) + instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" + instructions_dir.mkdir(parents=True) + (instructions_dir / f"{review_id}.passed").write_bytes(b"") + + results = write_instruction_files([task], tmp_path) + assert results == [] + + class TestPrecomputedContext: """Tests for precomputed info command execution — validates REVIEW-REQ-001.9, REVIEW-REQ-005.7."""