From 17ce7717c12fb42f4f282f3a5aecdf5d6bdaced8 Mon Sep 17 00:00:00 2001 From: Noah Horton Date: Sat, 11 Apr 2026 12:36:10 -0600 Subject: [PATCH 1/3] fix: tell review agents which project root to read files from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In git-worktree setups, reviewer subagents were reading files from the main checkout instead of the worktree where the commits actually live, producing spurious findings against stale (un-refactored) content. The "@filepath" entries in the instruction file's "Files to Review" section are not auto-expanded — the reviewer must Read each file itself — so they were being resolved against whatever cwd the subagent happened to inherit. Emit an explicit "## Project Root" directive at the top of each instruction file stating the absolute project root and instructing the reviewer to prepend it when calling Read. Adds REVIEW-REQ-005.1.9 and tests validating presence, ordering, and the write_instruction_files plumbing that threads project_root through. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../REVIEW-REQ-005-instruction-generation.md | 1 + src/deepwork/review/instructions.py | 24 ++++++++++- tests/unit/review/test_instructions.py | 42 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md b/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md index 96e65b56..72fff338 100644 --- a/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md +++ b/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md @@ -16,6 +16,7 @@ For each `ReviewTask`, the system generates a self-contained markdown instructio 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`. +9. The file MUST contain a "Project Root" section near the top (between the header and "Review Instructions") that states the absolute path of the project root against which all relative file paths in the document are resolved, and that instructs the reviewer to prepend this root when calling the Read tool. This is required so reviewer subagents read files from the correct working tree when their current working directory differs from the project root — e.g., when the review runs against a git worktree dispatched from the main checkout. ### REVIEW-REQ-005.2: File Path Formatting diff --git a/src/deepwork/review/instructions.py b/src/deepwork/review/instructions.py index 9dc1a5ab..c88aad41 100644 --- a/src/deepwork/review/instructions.py +++ b/src/deepwork/review/instructions.py @@ -210,7 +210,7 @@ def write_instruction_files( if task.precomputed_info_bash_command else None ) - content = build_instruction_file(task, review_id, precomputed_info) + content = build_instruction_file(task, review_id, precomputed_info, project_root) file_path = instructions_dir / f"{review_id}.md" safe_write(file_path, content) @@ -223,6 +223,7 @@ def build_instruction_file( task: ReviewTask, review_id: str = "", precomputed_info: str | None = None, + project_root: Path | None = None, ) -> str: """Build the markdown content for a single review instruction file. @@ -231,6 +232,12 @@ def build_instruction_file( review_id: The deterministic review ID for this task (used in the "After Review" section). precomputed_info: Pre-executed command output to include as context. + project_root: Absolute path to the project root that all relative + file paths in this instruction file resolve against. When + provided, the file includes an explicit "Project Root" + directive so the reviewer agent reads files from the correct + working tree even when its cwd differs (e.g., in a git + worktree dispatched from the main checkout). Returns: Markdown string containing the complete review instructions. @@ -241,6 +248,21 @@ def build_instruction_file( scope = _describe_scope(task) parts.append(f"# Review: {task.rule_name} — {scope}\n") + # Project root directive — tells the reviewer where to read files from. + # Critical in git-worktree setups where the agent's cwd may differ from + # the worktree the commits actually live in. + if project_root is not None: + abs_root = project_root.resolve() + parts.append("## Project Root\n") + parts.append( + f"**All file paths in this document are relative to `{abs_root}`.** " + "When reading any file below with the Read tool, you MUST construct " + "the absolute path by prepending this project root. Do NOT read files " + "relative to your current working directory — it may differ from the " + "project root (e.g., when this review runs against a git worktree)." + ) + parts.append("") + # Review instructions parts.append("## Review Instructions\n") parts.append(task.instructions.strip()) diff --git a/tests/unit/review/test_instructions.py b/tests/unit/review/test_instructions.py index 9a6a0c10..fed800a8 100644 --- a/tests/unit/review/test_instructions.py +++ b/tests/unit/review/test_instructions.py @@ -180,6 +180,48 @@ def test_after_review_section_before_traceability(self) -> None: trace_idx = content.index("This review was requested") assert after_idx < trace_idx + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.9). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_project_root_section_present_when_project_root_passed( + self, tmp_path: Path + ) -> None: + """When project_root is provided, a Project Root directive is emitted.""" + task = _make_task(files=["src/app.py"]) + content = build_instruction_file(task, project_root=tmp_path) + assert "## Project Root" in content + # The absolute path is rendered verbatim for the reviewer to prepend. + assert str(tmp_path.resolve()) in content + # The directive must tell the reviewer to prepend the root. + assert "prepend" in content.lower() + + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.9). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES + def test_project_root_section_before_review_instructions( + self, tmp_path: Path + ) -> None: + """The Project Root section is placed before Review Instructions.""" + task = _make_task() + content = build_instruction_file(task, project_root=tmp_path) + root_idx = content.index("## Project Root") + instr_idx = content.index("## Review Instructions") + assert root_idx < instr_idx + + def test_project_root_omitted_when_not_passed(self) -> None: + """Backward compat: omitting project_root omits the directive section.""" + task = _make_task() + content = build_instruction_file(task) + assert "## Project Root" not in content + + def test_write_instruction_files_passes_project_root(self, tmp_path: Path) -> None: + """write_instruction_files threads project_root through to build_instruction_file.""" + task = _make_task() + results = write_instruction_files([task], tmp_path) + assert len(results) == 1 + _t, file_path = results[0] + content = file_path.read_text() + assert "## Project Root" in content + assert str(tmp_path.resolve()) in content + class TestWriteInstructionFiles: """Tests for write_instruction_files.""" From ee6d8e3fa4a3047980e7f51208bfe40d4d541715 Mon Sep 17 00:00:00 2001 From: Noah Horton Date: Sat, 11 Apr 2026 15:19:13 -0600 Subject: [PATCH 2/3] chore: address review findings + changelog entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing code-quality issues surfaced by the reviewers on this PR (none were introduced by the original fix, but pulling them in here since the files are already under review): instructions.py: - _run_precompute_command docstring no longer claims "command" is an absolute path — it's a shell command string, invoked via shell=True - Noted the trust model for precompute shell commands (repo-sourced, never fed untrusted input) - ThreadPoolExecutor now caps at max_workers=8 so a repo with many distinct precompute commands can't briefly fork dozens of shells - _build_reference_files_section no longer re-encodes the (possibly truncated) content string to recompute its byte count; the consumed budget is now tracked explicitly as either original_byte_len or the exact remaining budget on truncation config.py: - ReviewRule.precomputed_info_bash_command field comment matches the docstring fix above test_instructions.py: - Moved five traceability comment blocks from inside the method body to the two lines directly preceding the def line, matching the convention enforced by test_file_quality - test_count_cap_triggers_omission no longer hardcodes "f20.txt" as "one of the omitted" — derives the filename from MAX_INLINE_FILES so the test stays correct if the constant changes - _sanitize_for_id import moved from a function-local line to the top-of-module import block REVIEW-REQ-005 spec: - Split five compound requirements so each numbered item carries exactly one RFC 2119 keyword (requirements_file DeepSchema rule). Preserved existing IDs (5.1.4, 5.1.8, 5.8.4–5.8.6) — each retains its primary MUST — and appended the secondary obligations as new sequential IDs (5.1.10, 5.1.11, 5.8.7–5.8.11) CHANGELOG.md: - Added entry for the worktree file-path fix under the Unreleased Fixed section Co-Authored-By: Claude Opus 4.6 (1M context) --- CHANGELOG.md | 2 + .../REVIEW-REQ-005-instruction-generation.md | 17 ++++++--- src/deepwork/review/config.py | 4 +- src/deepwork/review/instructions.py | 24 +++++++++--- tests/unit/review/test_instructions.py | 37 +++++++++---------- 5 files changed, 52 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1555dead..730fd1e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Review instruction files now include a `## Project Root` directive stating the absolute project root so reviewer subagents read files from the correct working tree — fixes spurious findings in git-worktree setups where the subagent's cwd differed from the worktree the commits actually lived in (REVIEW-REQ-005.1.9) + ### Removed ## [0.13.3] - 2026-04-10 diff --git a/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md b/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md index 72fff338..aa2efd2f 100644 --- a/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md +++ b/specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md @@ -11,12 +11,14 @@ 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 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. +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. 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`. +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. 9. The file MUST contain a "Project Root" section near the top (between the header and "Review Instructions") that states the absolute path of the project root against which all relative file paths in the document are resolved, and that instructs the reviewer to prepend this root when calling the Read tool. This is required so reviewer subagents read files from the correct working tree when their current working directory differs from the project root — e.g., when the review runs against a git worktree dispatched from the main checkout. +10. Inline-content tasks (see REVIEW-REQ-005.1.8) MUST NOT include a "Files to Review" section. +11. 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 @@ -62,6 +64,11 @@ For each `ReviewTask`, the system generates a self-contained markdown instructio 1. When a task's `reference_files` is empty, the instruction file MUST NOT contain a "Relevant File Contents" section. 2. When a task has `reference_files`, the instruction file MUST contain a "## Relevant File Contents" section placed between "Review Instructions" and "Files to Review". 3. Each inlined file MUST be rendered with a `### {relative_label}` subheading, the optional description, and the file contents inside a fenced code block whose language is inferred from the file extension. -4. The number of inlined reference files MUST NOT exceed `MAX_INLINE_FILES` (20). Entries beyond that cap MUST be listed in an "omitted due to size/count caps" summary line rather than inlined. -5. The total inlined byte size of reference file contents MUST NOT exceed `MAX_INLINE_TOTAL_BYTES` (256 * 1024). Files whose contents would exceed the remaining byte budget MUST be truncated with a visible truncation marker, and any subsequent entries MUST be reported in the omitted summary line. -6. When a referenced file cannot be read (missing, permission denied, or invalid UTF-8), the system MUST emit a graceful marker line referencing the file and the error, MUST NOT abort the section, and MUST NOT count the file's would-be bytes against the budget. +4. The number of inlined reference files MUST NOT exceed `MAX_INLINE_FILES` (20). +5. The total inlined byte size of reference file contents MUST NOT exceed `MAX_INLINE_TOTAL_BYTES` (256 * 1024). +6. When a referenced file cannot be read (missing, permission denied, or invalid UTF-8), the system MUST emit a graceful marker line referencing the file and the error. +7. Reference file entries beyond the `MAX_INLINE_FILES` cap MUST be listed in an "omitted due to size/count caps" summary line rather than inlined. +8. When a reference file's content would exceed the remaining byte budget, the file MUST be truncated with a visible truncation marker. +9. Reference file entries that cannot be inlined because the byte budget was exhausted by a preceding truncation MUST be reported in the omitted summary line. +10. When a referenced file cannot be read, the system MUST NOT abort the "Relevant File Contents" section. +11. When a referenced file cannot be read, the system MUST NOT count the file's would-be bytes against the total byte budget. diff --git a/src/deepwork/review/config.py b/src/deepwork/review/config.py index 136134a0..5909e9a8 100644 --- a/src/deepwork/review/config.py +++ b/src/deepwork/review/config.py @@ -42,7 +42,9 @@ class ReviewRule: agent: dict[str, str] | None all_changed_filenames: bool unchanged_matching_files: bool - precomputed_info_bash_command: str | None # Resolved absolute command path + precomputed_info_bash_command: ( + str | None + ) # Shell command string (first path component resolved to absolute) source_dir: Path # Directory containing the .deepreview file source_file: Path # Path to the .deepreview file source_line: int # Line number of the rule name in the .deepreview file diff --git a/src/deepwork/review/instructions.py b/src/deepwork/review/instructions.py index c88aad41..92944e1f 100644 --- a/src/deepwork/review/instructions.py +++ b/src/deepwork/review/instructions.py @@ -105,8 +105,15 @@ def _content_hash(files: list[str], project_root: Path, inline_content: str | No def _run_precompute_command(command: str, project_root: Path) -> str: """Run a single precompute bash command and return its stdout. + Precompute commands are fully trusted input sourced from the repo's own + ``.deepreview`` files; they run via ``shell=True`` and MUST NOT be fed + any untrusted external data (e.g., user-supplied strings interpolated + into the command). + Args: - command: Resolved absolute path to the command to execute. + command: Shell command string to execute. The first path component + has been resolved to an absolute path by the caller; the rest + of the command is passed through to the shell verbatim. project_root: Working directory for command execution. Returns: @@ -146,8 +153,10 @@ def _run_precompute_commands(commands: set[str], project_root: Path) -> dict[str if not commands: return {} + # Cap concurrent precompute shells so a repo with many rules does not + # briefly fork dozens of subprocesses at once on CI runners. results: dict[str, str] = {} - with ThreadPoolExecutor() as executor: + with ThreadPoolExecutor(max_workers=8) as executor: futures = { executor.submit(_run_precompute_command, cmd, project_root): cmd for cmd in commands } @@ -375,17 +384,20 @@ def _build_reference_files_section(reference_files: list[ReferenceFile]) -> str: remaining = MAX_INLINE_TOTAL_BYTES - total_bytes truncated_marker = "" content_bytes = content.encode("utf-8") - if len(content_bytes) > remaining: + original_byte_len = len(content_bytes) + if original_byte_len > remaining: # Truncate at a character boundary near the byte budget. content = content_bytes[:remaining].decode("utf-8", errors="ignore") truncated_marker = ( - f"\n... (truncated: file is {len(content_bytes)} bytes, " - f"budget left was {remaining})" + f"\n... (truncated: file is {original_byte_len} bytes, budget left was {remaining})" ) + consumed = remaining + else: + consumed = original_byte_len parts.append(header) parts.append(f"\n\n```{lang}\n{content}{truncated_marker}\n```\n") - total_bytes += len(content.encode("utf-8")) + total_bytes += consumed inlined_count += 1 if omitted: diff --git a/tests/unit/review/test_instructions.py b/tests/unit/review/test_instructions.py index fed800a8..d1123380 100644 --- a/tests/unit/review/test_instructions.py +++ b/tests/unit/review/test_instructions.py @@ -14,6 +14,7 @@ MAX_INLINE_TOTAL_BYTES, _run_precompute_command, _run_precompute_commands, + _sanitize_for_id, build_instruction_file, compute_review_id, write_instruction_files, @@ -99,30 +100,30 @@ def test_includes_all_changed_filenames_without_at_prefix(self) -> None: f"Context-only files should not have @ prefix: {line}" ) + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.6). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_omits_unchanged_section_when_empty(self) -> None: - # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.6). - # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES task = _make_task(additional_files=[]) content = build_instruction_file(task) assert "## Unchanged Matching Files" not in content + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.7). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_omits_all_changed_section_when_none(self) -> None: - # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.7). - # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES task = _make_task(all_changed_filenames=None) content = build_instruction_file(task) assert "## All Changed Files" not in content + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_single_file_scope_description(self) -> None: - # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.2). - # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES task = _make_task(files=["src/app.py"]) content = build_instruction_file(task) assert "src/app.py" in content.split("\n")[0] + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.2). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_multi_file_scope_description(self) -> None: - # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.2). - # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES task = _make_task(files=["a.py", "b.py", "c.py"]) content = build_instruction_file(task) assert "3 files" in content.split("\n")[0] @@ -152,9 +153,9 @@ def test_omits_traceability_when_source_location_empty(self) -> None: content = build_instruction_file(task) assert "This review was requested" not in content + # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.6.1). + # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_traceability_is_at_end(self) -> None: - # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.6.1). - # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES task = _make_task(source_location=".deepreview:1") content = build_instruction_file(task) last_nonblank = [line for line in content.strip().split("\n") if line.strip()][-1] @@ -182,9 +183,7 @@ def test_after_review_section_before_traceability(self) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.9). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_project_root_section_present_when_project_root_passed( - self, tmp_path: Path - ) -> None: + def test_project_root_section_present_when_project_root_passed(self, tmp_path: Path) -> None: """When project_root is provided, a Project Root directive is emitted.""" task = _make_task(files=["src/app.py"]) content = build_instruction_file(task, project_root=tmp_path) @@ -196,9 +195,7 @@ def test_project_root_section_present_when_project_root_passed( # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.1.9). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_project_root_section_before_review_instructions( - self, tmp_path: Path - ) -> None: + def test_project_root_section_before_review_instructions(self, tmp_path: Path) -> None: """The Project Root section is placed before Review Instructions.""" task = _make_task() content = build_instruction_file(task, project_root=tmp_path) @@ -457,8 +454,6 @@ def test_missing_file_uses_placeholder(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.1.2). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_special_chars_in_rule_name_sanitized(self, tmp_path: Path) -> None: - from deepwork.review.instructions import _sanitize_for_id - assert _sanitize_for_id("my rule@v2!") == "my-rule-v2-" # Also verify it preserves allowed chars: alphanumeric, dash, underscore, dot assert _sanitize_for_id("rule_name-1.0") == "rule_name-1.0" @@ -675,15 +670,17 @@ def test_inlines_content_with_description(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.8.4). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_count_cap_triggers_omission(self, tmp_path: Path) -> None: + total = MAX_INLINE_FILES + 3 refs: list[ReferenceFile] = [] - for i in range(MAX_INLINE_FILES + 3): + for i in range(total): f = tmp_path / f"f{i}.txt" f.write_text(f"content {i}") refs.append(ReferenceFile(path=f, relative_label=f"f{i}.txt")) task = self._task_with_refs(refs) content = build_instruction_file(task) assert "omitted due to size/count caps" in content - assert "f20.txt" in content # one of the omitted + # The last-created file is guaranteed to be in the omitted set regardless of MAX_INLINE_FILES. + assert f"f{total - 1}.txt" in content assert content.count("```") >= 2 * MAX_INLINE_FILES # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.8.5). From 72b7de6b3c32803a758964b434c64c3cedb1596b Mon Sep 17 00:00:00 2001 From: Noah Horton Date: Sat, 11 Apr 2026 15:35:36 -0600 Subject: [PATCH 3/3] chore: address remaining low-severity review findings instructions.py: - precomputed_info lookup now uses direct key access ([]) instead of .get() so a dict/filter drift between unique_commands and the per-task lookup would fail loudly instead of silently dropping precomputed context. Also aligned both predicates on `is not None` so an empty-string command would not be executed then silently discarded. - Added a docstring note to _content_hash pinning the trust contract: files entries must be repo-root-relative paths from trusted .deepreview config, with no absolute paths or .. traversal. test_instructions.py: - New module-level `instructions_dir` pytest fixture that creates `.deepwork/tmp/review_instructions/` under tmp_path. Seven tests now use it instead of repeating the two-line setup inline. - test_single_file_produces_expected_format uses direct `(tmp_path / "src").mkdir(...)` instead of the indirect `.parent.mkdir` form, matching adjacent tests in TestComputeReviewId. - Added an inline comment on the `2 * MAX_INLINE_FILES` fence-count assertion explaining the 2x relationship. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/deepwork/review/instructions.py | 12 +++++-- tests/unit/review/test_instructions.py | 50 ++++++++++++++------------ 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/src/deepwork/review/instructions.py b/src/deepwork/review/instructions.py index 92944e1f..76e3feca 100644 --- a/src/deepwork/review/instructions.py +++ b/src/deepwork/review/instructions.py @@ -85,6 +85,11 @@ def _content_hash(files: list[str], project_root: Path, inline_content: str | No ``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. + + ``files`` entries are expected to be repo-root-relative paths sourced + from trusted ``.deepreview`` config files. Absolute paths or ``..`` + segments are not validated here; callers MUST ensure paths stay + inside ``project_root``. """ h = hashlib.sha256() for filepath in sorted(files): @@ -214,9 +219,12 @@ def write_instruction_files( if passed_marker.exists(): continue + # Direct key access (not .get()): the invariant below enforces that + # every non-None command produced unique_commands above MUST have a + # result in precompute_results — a missing key indicates drift. precomputed_info = ( - precompute_results.get(task.precomputed_info_bash_command) - if task.precomputed_info_bash_command + precompute_results[task.precomputed_info_bash_command] + if task.precomputed_info_bash_command is not None else None ) content = build_instruction_file(task, review_id, precomputed_info, project_root) diff --git a/tests/unit/review/test_instructions.py b/tests/unit/review/test_instructions.py index d1123380..696d0203 100644 --- a/tests/unit/review/test_instructions.py +++ b/tests/unit/review/test_instructions.py @@ -8,6 +8,8 @@ from pathlib import Path from unittest.mock import patch +import pytest + from deepwork.review.config import ReferenceFile, ReviewTask from deepwork.review.instructions import ( MAX_INLINE_FILES, @@ -21,6 +23,14 @@ ) +@pytest.fixture +def instructions_dir(tmp_path: Path) -> Path: + """Create and return the `.deepwork/tmp/review_instructions/` directory under tmp_path.""" + d = tmp_path / ".deepwork" / "tmp" / "review_instructions" + d.mkdir(parents=True) + return d + + def _make_task( rule_name: str = "test_rule", files: list[str] | None = None, @@ -245,9 +255,7 @@ def test_unique_filenames(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.5.1, REVIEW-REQ-009.5.1). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_clears_previous_files(self, tmp_path: Path) -> None: - instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" - instructions_dir.mkdir(parents=True) + def test_clears_previous_files(self, tmp_path: Path, instructions_dir: Path) -> None: (instructions_dir / "old_file.md").write_text("stale") tasks = [_make_task()] @@ -282,13 +290,11 @@ def test_uses_review_id_based_filenames(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.3.1, REVIEW-REQ-009.3.2). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_skips_tasks_with_passed_marker(self, tmp_path: Path) -> None: + def test_skips_tasks_with_passed_marker(self, tmp_path: Path, instructions_dir: Path) -> None: task = _make_task(rule_name="my_rule") review_id = compute_review_id(task, tmp_path) # Create .passed marker - 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) @@ -296,9 +302,9 @@ def test_skips_tasks_with_passed_marker(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.5.1, REVIEW-REQ-009.5.2). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_preserves_passed_files_on_cleanup(self, tmp_path: Path) -> None: - instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" - instructions_dir.mkdir(parents=True) + def test_preserves_passed_files_on_cleanup( + self, tmp_path: Path, instructions_dir: Path + ) -> None: passed_file = instructions_dir / "some_review.passed" passed_file.write_bytes(b"") @@ -308,9 +314,9 @@ def test_preserves_passed_files_on_cleanup(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.5.1, REVIEW-REQ-009.5.3). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_preserves_md_files_with_passed_marker(self, tmp_path: Path) -> None: - instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" - instructions_dir.mkdir(parents=True) + def test_preserves_md_files_with_passed_marker( + self, tmp_path: Path, instructions_dir: Path + ) -> None: (instructions_dir / "old_review.md").write_text("passed content") (instructions_dir / "old_review.passed").write_bytes(b"") @@ -321,10 +327,8 @@ def test_preserves_md_files_with_passed_marker(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.3.3). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_all_tasks_passed_returns_empty(self, tmp_path: Path) -> None: + def test_all_tasks_passed_returns_empty(self, tmp_path: Path, instructions_dir: Path) -> None: tasks = [_make_task(rule_name="rule_a"), _make_task(rule_name="rule_b")] - instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" - instructions_dir.mkdir(parents=True) for task in tasks: review_id = compute_review_id(task, tmp_path) @@ -335,13 +339,12 @@ def test_all_tasks_passed_returns_empty(self, tmp_path: Path) -> None: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.3.1, REVIEW-REQ-009.3.2). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES - def test_some_tasks_passed_returns_remaining(self, tmp_path: Path) -> None: + def test_some_tasks_passed_returns_remaining( + self, tmp_path: Path, instructions_dir: Path + ) -> None: task_a = _make_task(rule_name="rule_a") task_b = _make_task(rule_name="rule_b") - instructions_dir = tmp_path / ".deepwork" / "tmp" / "review_instructions" - instructions_dir.mkdir(parents=True) - # Only mark task_a as passed review_id_a = compute_review_id(task_a, tmp_path) (instructions_dir / f"{review_id_a}.passed").write_bytes(b"") @@ -357,7 +360,7 @@ class TestComputeReviewId: # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.1.1). # YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES def test_single_file_produces_expected_format(self, tmp_path: Path) -> None: - (tmp_path / "src" / "app.py").parent.mkdir(parents=True, exist_ok=True) + (tmp_path / "src").mkdir(parents=True, exist_ok=True) (tmp_path / "src" / "app.py").write_text("print('hello')") task = _make_task(rule_name="python_review", files=["src/app.py"]) @@ -523,12 +526,12 @@ def test_same_inline_value_produces_same_id(self, tmp_path: Path) -> None: 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: + def test_pass_caching_works_for_inline_content( + self, tmp_path: Path, instructions_dir: 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) @@ -681,6 +684,7 @@ def test_count_cap_triggers_omission(self, tmp_path: Path) -> None: assert "omitted due to size/count caps" in content # The last-created file is guaranteed to be in the omitted set regardless of MAX_INLINE_FILES. assert f"f{total - 1}.txt" in content + # Each inlined file contributes one opening and one closing fence. assert content.count("```") >= 2 * MAX_INLINE_FILES # THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.8.5).