From 2858edaccf924ee4a3d3596728c4040ec45b4168 Mon Sep 17 00:00:00 2001 From: "Agusti F." <6601142+agustif@users.noreply.github.com> Date: Wed, 4 Mar 2026 23:39:46 +0100 Subject: [PATCH] refactor(review): unify packet construction across prepare, batch, and external --- .../app/commands/review/batch/orchestrator.py | 75 ++---- desloppify/app/commands/review/external.py | 73 ++--- .../app/commands/review/packet/build.py | 17 ++ desloppify/app/commands/review/prepare.py | 100 +++---- .../review/policy/test_packet_mode_parity.py | 254 ++++++++++++++++++ 5 files changed, 351 insertions(+), 168 deletions(-) create mode 100644 desloppify/tests/review/policy/test_packet_mode_parity.py diff --git a/desloppify/app/commands/review/batch/orchestrator.py b/desloppify/app/commands/review/batch/orchestrator.py index e6c1a37c..3e37e246 100644 --- a/desloppify/app/commands/review/batch/orchestrator.py +++ b/desloppify/app/commands/review/batch/orchestrator.py @@ -9,19 +9,20 @@ from typing import cast from desloppify.app.commands.helpers.query import write_query_best_effort -from desloppify.base.coercions import coerce_positive_int from desloppify.base.discovery.file_paths import safe_write_text from desloppify.base.exception_sets import CommandError, PacketValidationError from desloppify.base.output.terminal import colorize, log -import desloppify.intelligence.narrative.core as narrative_mod from desloppify.intelligence import review as review_mod from desloppify.intelligence.review.feedback_contract import ( max_batch_issues_for_dimension_count, ) -from ..helpers import parse_dimensions +from ..coordinator import build_review_packet_payload, write_review_packet_snapshot from ..importing.cmd import do_import as _do_import -from ..packet.policy import coerce_review_batch_file_limit, redacted_review_config +from ..packet.build import ( + build_run_batches_next_command, + resolve_review_packet_context, +) from ..runner_failures import print_failures, print_failures_and_raise from ..runner_packets import ( build_batch_import_provenance, @@ -29,7 +30,6 @@ prepare_run_artifacts, run_stamp, selected_batch_indexes, - write_packet_snapshot, ) from ..runner_parallel import collect_batch_results, execute_batches from ..runner_process import ( @@ -42,9 +42,6 @@ from ..runtime_paths import ( blind_packet_path as _blind_packet_path, ) -from ..runtime_paths import ( - review_packet_dir as _review_packet_dir, -) from ..runtime_paths import ( runtime_project_root as _runtime_project_root, ) @@ -117,60 +114,28 @@ def _load_or_prepare_packet( print(colorize(f" Blind packet: {blind_path}", "dim")) return packet, packet_path, blind_path - path = Path(args.path) - dims = parse_dimensions(args) - dimensions = list(dims) if dims else None - retrospective = bool(getattr(args, "retrospective", False)) - retrospective_max_issues = coerce_positive_int( - getattr(args, "retrospective_max_issues", None), - default=30, - minimum=1, - ) - retrospective_max_batch_items = coerce_positive_int( - getattr(args, "retrospective_max_batch_items", None), - default=20, - minimum=1, - ) - lang_run, found_files = _setup_lang(lang, path, config) - lang_name = lang_run.name - narrative = narrative_mod.compute_narrative( - state, - context=narrative_mod.NarrativeContext(lang=lang_name, command="review"), - ) - - blind_path = _blind_packet_path() - packet = review_mod.prepare_holistic_review( - path, - lang_run, - state, - options=review_mod.HolisticReviewPrepareOptions( - dimensions=dimensions, - files=found_files or None, - max_files_per_batch=coerce_review_batch_file_limit(config), - include_issue_history=retrospective, - issue_history_max_issues=retrospective_max_issues, - issue_history_max_batch_items=retrospective_max_batch_items, - ), - ) - packet["config"] = redacted_review_config(config) - packet["narrative"] = narrative - next_command = "desloppify review --prepare" - if retrospective: - next_command += ( - " --retrospective" - f" --retrospective-max-issues {retrospective_max_issues}" - f" --retrospective-max-batch-items {retrospective_max_batch_items}" + context = resolve_review_packet_context(args) + next_command = build_run_batches_next_command(context) + try: + packet = build_review_packet_payload( + state=state, + lang=lang, + config=config, + context=context, + next_command=next_command, + setup_lang_fn=_setup_lang, + prepare_holistic_review_fn=review_mod.prepare_holistic_review, ) - packet["next_command"] = next_command + except ValueError as exc: + raise PacketValidationError(str(exc), exit_code=1) from exc write_query_best_effort( packet, context="review packet query update", ) - packet_path, blind_saved = write_packet_snapshot( + packet_path, blind_saved = write_review_packet_snapshot( packet, stamp=stamp, - review_packet_dir=_review_packet_dir(), - blind_path=blind_path, + blind_path_override=_blind_packet_path(), safe_write_text_fn=safe_write_text, ) print(colorize(f" Immutable packet: {packet_path}", "dim")) diff --git a/desloppify/app/commands/review/external.py b/desloppify/app/commands/review/external.py index 0473351d..26a4f73e 100644 --- a/desloppify/app/commands/review/external.py +++ b/desloppify/app/commands/review/external.py @@ -12,19 +12,18 @@ from typing import Any from desloppify.app.commands.helpers.query import write_query -from desloppify.base.coercions import coerce_positive_int from desloppify.base.discovery.file_paths import safe_write_text from desloppify.base.exception_sets import CommandError from desloppify.base.output.terminal import colorize -import desloppify.intelligence.narrative.core as narrative_mod from desloppify.intelligence import review as review_mod from .batch.orchestrator import FOLLOWUP_SCAN_TIMEOUT_SECONDS -from .helpers import parse_dimensions +from .coordinator import build_review_packet_payload, write_review_packet_snapshot from .importing.cmd import do_import, do_validate_import -from .runner_packets import run_stamp, sha256_file, write_packet_snapshot -from .runner_process import FollowupScanDeps, run_followup_scan -from .runtime.setup import setup_lang_concrete +from .packet.build import ( + build_external_submit_next_command, + resolve_review_packet_context, +) from .prompt_sections import ( build_batch_context, explode_to_single_dimension, @@ -38,15 +37,15 @@ render_seed_files_block, render_task_requirements, ) +from .runner_packets import run_stamp, sha256_file +from .runner_process import FollowupScanDeps, run_followup_scan +from .runtime.setup import setup_lang_concrete from .runtime_paths import ( blind_packet_path as _blind_packet_path, ) from .runtime_paths import ( external_session_root as _external_session_root, ) -from .runtime_paths import ( - review_packet_dir as _review_packet_dir, -) from .runtime_paths import ( runtime_project_root as _runtime_project_root, ) @@ -130,53 +129,27 @@ def _prepare_packet_snapshot( config: dict[str, Any], ) -> tuple[dict[str, Any], Path, Path]: """Prepare holistic review packet and persist immutable+blind snapshots.""" - path = Path(getattr(args, "path", ".") or ".") - dims = parse_dimensions(args) - dimensions = list(dims) if dims else None - retrospective = bool(getattr(args, "retrospective", False)) - retrospective_max_issues = coerce_positive_int( - getattr(args, "retrospective_max_issues", None), - default=30, - ) - retrospective_max_batch_items = coerce_positive_int( - getattr(args, "retrospective_max_batch_items", None), - default=20, - ) - lang_run, found_files = setup_lang_concrete(lang, path, config) - narrative = narrative_mod.compute_narrative( - state, - context=narrative_mod.NarrativeContext(lang=lang_run.name, command="review"), - ) - packet = review_mod.prepare_holistic_review( - path, - lang_run, - state, - options=review_mod.HolisticReviewPrepareOptions( - dimensions=dimensions, - files=found_files or None, - include_issue_history=retrospective, - issue_history_max_issues=retrospective_max_issues, - issue_history_max_batch_items=retrospective_max_batch_items, - ), - ) - packet["narrative"] = narrative - next_command = "desloppify review --external-submit --session-id --import " - if retrospective: - next_command += ( - " --retrospective" - f" --retrospective-max-issues {retrospective_max_issues}" - f" --retrospective-max-batch-items {retrospective_max_batch_items}" + context = resolve_review_packet_context(args) + next_command = build_external_submit_next_command(context) + try: + packet = build_review_packet_payload( + state=state, + lang=lang, + config=config, + context=context, + next_command=next_command, + setup_lang_fn=setup_lang_concrete, + prepare_holistic_review_fn=review_mod.prepare_holistic_review, ) - packet["next_command"] = next_command + except ValueError as exc: + raise CommandError(str(exc), exit_code=1) from exc write_query(packet) stamp = run_stamp() - blind_packet_path = _blind_packet_path() - packet_path, blind_path = write_packet_snapshot( + packet_path, blind_path = write_review_packet_snapshot( packet, stamp=stamp, - review_packet_dir=_review_packet_dir(), - blind_path=blind_packet_path, + blind_path_override=_blind_packet_path(), safe_write_text_fn=safe_write_text, ) return packet, packet_path, blind_path diff --git a/desloppify/app/commands/review/packet/build.py b/desloppify/app/commands/review/packet/build.py index 7802084c..19f04185 100644 --- a/desloppify/app/commands/review/packet/build.py +++ b/desloppify/app/commands/review/packet/build.py @@ -110,6 +110,22 @@ def build_run_batches_next_command(context: ReviewPacketContext) -> str: return " ".join(parts) +def build_prepare_next_command(context: ReviewPacketContext) -> str: + """Return the canonical next command for prepare-mode packet output.""" + parts: list[str] = ["desloppify", "review", "--prepare"] + if context.retrospective: + parts.extend( + [ + "--retrospective", + "--retrospective-max-issues", + str(context.retrospective_max_issues), + "--retrospective-max-batch-items", + str(context.retrospective_max_batch_items), + ] + ) + return " ".join(parts) + + def build_external_submit_next_command(context: ReviewPacketContext) -> str: """Return the canonical next command for external-session submit.""" parts: list[str] = [ @@ -150,6 +166,7 @@ def require_non_empty_packet(packet: dict[str, Any], *, path: Path) -> int: "ReviewPacketContext", "build_external_submit_next_command", "build_holistic_packet", + "build_prepare_next_command", "build_run_batches_next_command", "require_non_empty_packet", "resolve_review_packet_context", diff --git a/desloppify/app/commands/review/prepare.py b/desloppify/app/commands/review/prepare.py index b1852479..6c49c2d9 100644 --- a/desloppify/app/commands/review/prepare.py +++ b/desloppify/app/commands/review/prepare.py @@ -2,19 +2,22 @@ from __future__ import annotations -from pathlib import Path - +import desloppify.intelligence.narrative.core as narrative_mod from desloppify.app.commands.helpers.query import write_query -from desloppify.base.coercions import coerce_positive_int from desloppify.base.exception_sets import CommandError from desloppify.base.output.terminal import colorize -import desloppify.intelligence.narrative.core as narrative_mod from desloppify.intelligence import review as review_mod -from .helpers import parse_dimensions -from .packet.policy import coerce_review_batch_file_limit, redacted_review_config +from .coordinator import build_review_packet_payload +from .packet.build import ( + build_prepare_next_command, + resolve_review_packet_context, +) from .runtime.setup import setup_lang_concrete +# Backward-compatible patch target used by direct tests. +_ = narrative_mod + def do_prepare( args, @@ -25,65 +28,36 @@ def do_prepare( config: dict, ) -> None: """Prepare mode: holistic-only review packet in query.json.""" - path = Path(args.path) - dims = parse_dimensions(args) - dimensions = list(dims) if dims else None - retrospective = bool(getattr(args, "retrospective", False)) - retrospective_max_issues = coerce_positive_int( - getattr(args, "retrospective_max_issues", None), - default=30, - ) - retrospective_max_batch_items = coerce_positive_int( - getattr(args, "retrospective_max_batch_items", None), - default=20, - ) - - lang_run, found_files = setup_lang_concrete(lang, path, config) - - lang_name = lang_run.name - narrative = narrative_mod.compute_narrative( - state, - context=narrative_mod.NarrativeContext(lang=lang_name, command="review"), - ) - data = review_mod.prepare_holistic_review( - path, - lang_run, - state, - options=review_mod.HolisticReviewPrepareOptions( - dimensions=dimensions, - files=found_files or None, - max_files_per_batch=coerce_review_batch_file_limit(config), - include_issue_history=retrospective, - issue_history_max_issues=retrospective_max_issues, - issue_history_max_batch_items=retrospective_max_batch_items, - ), - ) - next_command = ( - "desloppify review --prepare" - ) - if retrospective: - next_command += ( - " --retrospective" - f" --retrospective-max-issues {retrospective_max_issues}" - f" --retrospective-max-batch-items {retrospective_max_batch_items}" + context = resolve_review_packet_context(args) + next_command = build_prepare_next_command(context) + try: + data = build_review_packet_payload( + state=state, + lang=lang, + config=config, + context=context, + next_command=next_command, + setup_lang_fn=setup_lang_concrete, + prepare_holistic_review_fn=review_mod.prepare_holistic_review, ) - data["config"] = redacted_review_config(config) - data["narrative"] = narrative - data["next_command"] = next_command - total = data.get("total_files", 0) - if total == 0: - msg = f"no files found at path '{path}'. Nothing to review." - scan_path = state.get("scan_path") if isinstance(state, dict) else None - if scan_path: - msg += ( - f"\nHint: your last scan used --path {scan_path}. " - f"Try: desloppify review --prepare --path {scan_path}" - ) - else: - msg += "\nHint: pass --path matching the path used during scan." - raise CommandError(msg, exit_code=1) + except ValueError as exc: + msg = str(exc) + if "no files found at path" in msg: + scan_path = state.get("scan_path") if isinstance(state, dict) else None + if scan_path: + msg += ( + f"\nHint: your last scan used --path {scan_path}. " + f"Try: desloppify review --prepare --path {scan_path}" + ) + else: + msg += "\nHint: pass --path matching the path used during scan." + raise CommandError(msg, exit_code=1) from exc write_query(data) - _print_prepare_summary(data, next_command=next_command, retrospective=retrospective) + _print_prepare_summary( + data, + next_command=next_command, + retrospective=context.retrospective, + ) def _print_prepare_summary( diff --git a/desloppify/tests/review/policy/test_packet_mode_parity.py b/desloppify/tests/review/policy/test_packet_mode_parity.py new file mode 100644 index 00000000..1c8f3a2c --- /dev/null +++ b/desloppify/tests/review/policy/test_packet_mode_parity.py @@ -0,0 +1,254 @@ +"""Regression tests for review packet parity and shared builder delegation.""" + +from __future__ import annotations + +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +from desloppify.app.commands.review.batch.orchestrator import _load_or_prepare_packet +from desloppify.app.commands.review.external import _prepare_packet_snapshot +from desloppify.app.commands.review.prepare import do_prepare +from desloppify.state import empty_state as build_empty_state + + +def _args(path: Path) -> SimpleNamespace: + return SimpleNamespace( + path=str(path), + dimensions=None, + retrospective=True, + retrospective_max_issues=7, + retrospective_max_batch_items=5, + packet=None, + ) + + +def _fake_setup(*_args, **_kwargs): + return SimpleNamespace(name="python"), ["src/app.py", "src/utils.py"] + + +def _fake_holistic_prepare(_path, lang_run, _state, *, options): + dimensions = ( + list(options.dimensions) + if isinstance(options.dimensions, list) + else ["naming_quality", "logic_clarity"] + ) + return { + "command": "review", + "mode": "holistic", + "language": lang_run.name, + "dimensions": dimensions, + "system_prompt": "mock prompt", + "investigation_batches": [ + { + "name": "naming batch", + "dimensions": [dimensions[0]], + "files_to_read": ["src/app.py"], + "why": "test", + } + ], + "total_files": 2, + "workflow": ["inspect files"], + } + + +def test_review_packet_shared_fields_match_across_modes(tmp_path): + args = _args(tmp_path) + state = build_empty_state() + lang = SimpleNamespace(name="python") + config = { + "target_strict_score": 98, + "noise_budget": 10, + "review_batch_max_files": 17, + } + captured_prepare: dict = {} + + with ( + patch( + "desloppify.app.commands.review.packet.build.narrative_mod.compute_narrative", + return_value={"headline": "stable narrative"}, + ), + patch( + "desloppify.app.commands.review.prepare.setup_lang_concrete", + side_effect=_fake_setup, + ), + patch( + "desloppify.app.commands.review.batch.orchestrator._setup_lang", + side_effect=_fake_setup, + ), + patch( + "desloppify.app.commands.review.external.setup_lang_concrete", + side_effect=_fake_setup, + ), + patch( + "desloppify.app.commands.review.prepare.review_mod.prepare_holistic_review", + side_effect=_fake_holistic_prepare, + ), + patch( + "desloppify.app.commands.review.batch.orchestrator.review_mod.prepare_holistic_review", + side_effect=_fake_holistic_prepare, + ), + patch( + "desloppify.app.commands.review.external.review_mod.prepare_holistic_review", + side_effect=_fake_holistic_prepare, + ), + patch( + "desloppify.app.commands.review.prepare.write_query", + side_effect=lambda payload: captured_prepare.update(payload), + ), + patch( + "desloppify.app.commands.review.prepare._print_prepare_summary", + return_value=None, + ), + patch( + "desloppify.app.commands.review.batch.orchestrator.write_query_best_effort", + return_value=None, + ), + patch( + "desloppify.app.commands.review.external.write_query", + return_value=None, + ), + patch( + "desloppify.app.commands.review.batch.orchestrator.write_review_packet_snapshot", + return_value=(tmp_path / "batch.packet.json", tmp_path / "batch.blind.json"), + ), + patch( + "desloppify.app.commands.review.external.write_review_packet_snapshot", + return_value=(tmp_path / "ext.packet.json", tmp_path / "ext.blind.json"), + ), + ): + do_prepare(args, state, lang, None, config=config) + packet_batch, _batch_packet_path, _batch_blind_path = _load_or_prepare_packet( + args, + state=state, + lang=lang, + config=config, + stamp="20260304_000000", + ) + packet_external, _ext_packet_path, _ext_blind_path = _prepare_packet_snapshot( + args, + state, + lang, + config=config, + ) + + shared_fields = [ + "command", + "mode", + "language", + "dimensions", + "system_prompt", + "investigation_batches", + "total_files", + "workflow", + "config", + "narrative", + ] + for field in shared_fields: + assert captured_prepare[field] == packet_batch[field] == packet_external[field] + + assert "target_strict_score" not in captured_prepare["config"] + assert captured_prepare["next_command"] == ( + "desloppify review --prepare --retrospective" + " --retrospective-max-issues 7 --retrospective-max-batch-items 5" + ) + assert packet_batch["next_command"] == ( + "desloppify review --run-batches --runner codex --parallel --scan-after-import" + " --retrospective --retrospective-max-issues 7 --retrospective-max-batch-items 5" + ) + assert packet_external["next_command"] == ( + "desloppify review --external-submit --session-id --import " + " --retrospective --retrospective-max-issues 7 --retrospective-max-batch-items 5" + ) + + +def test_review_modes_delegate_to_shared_builder(tmp_path): + args = _args(tmp_path) + state = build_empty_state() + lang = SimpleNamespace(name="python") + payload = { + "command": "review", + "mode": "holistic", + "total_files": 1, + "investigation_batches": [], + "workflow": [], + "config": {}, + "narrative": {}, + } + + with ( + patch( + "desloppify.app.commands.review.prepare.build_review_packet_payload", + return_value=dict(payload), + ) as prepare_builder, + patch( + "desloppify.app.commands.review.prepare.write_query", + return_value=None, + ), + patch( + "desloppify.app.commands.review.prepare._print_prepare_summary", + return_value=None, + ), + ): + do_prepare(args, state, lang, None, config={}) + assert prepare_builder.call_count == 1 + assert ( + prepare_builder.call_args.kwargs["next_command"] + == "desloppify review --prepare --retrospective" + " --retrospective-max-issues 7 --retrospective-max-batch-items 5" + ) + + with ( + patch( + "desloppify.app.commands.review.batch.orchestrator.build_review_packet_payload", + return_value=dict(payload), + ) as batch_builder, + patch( + "desloppify.app.commands.review.batch.orchestrator.write_query_best_effort", + return_value=None, + ), + patch( + "desloppify.app.commands.review.batch.orchestrator.write_review_packet_snapshot", + return_value=(tmp_path / "packet.json", tmp_path / "blind.json"), + ), + ): + _load_or_prepare_packet( + args, + state=state, + lang=lang, + config={}, + stamp="20260304_000000", + ) + assert batch_builder.call_count == 1 + assert ( + batch_builder.call_args.kwargs["next_command"] + == "desloppify review --run-batches --runner codex --parallel --scan-after-import" + " --retrospective --retrospective-max-issues 7 --retrospective-max-batch-items 5" + ) + + with ( + patch( + "desloppify.app.commands.review.external.build_review_packet_payload", + return_value=dict(payload), + ) as external_builder, + patch( + "desloppify.app.commands.review.external.write_query", + return_value=None, + ), + patch( + "desloppify.app.commands.review.external.write_review_packet_snapshot", + return_value=(tmp_path / "ext.packet.json", tmp_path / "ext.blind.json"), + ), + ): + _prepare_packet_snapshot( + args, + state, + lang, + config={}, + ) + assert external_builder.call_count == 1 + assert ( + external_builder.call_args.kwargs["next_command"] + == "desloppify review --external-submit --session-id --import " + " --retrospective --retrospective-max-issues 7 --retrospective-max-batch-items 5" + )