diff --git a/project/TICKETS.md b/project/TICKETS.md index 83fa1589..52fd8bc9 100644 --- a/project/TICKETS.md +++ b/project/TICKETS.md @@ -189,4 +189,5 @@ project analysis or human-owned ticket inputs. | **ticket-188** | [`README.md`](./ticket-188/README.md) | - | - | - | - | - | | **ticket-189** | [`README.md`](./ticket-189/README.md) | - | - | - | - | - | | **ticket-190** | [`README.md`](./ticket-190/README.md) | - | - | - | - | - | +| **ticket-191** | [`README.md`](./ticket-191/README.md) | - | - | - | - | - | diff --git a/project/ticket-192/README.md b/project/ticket-192/README.md new file mode 100644 index 00000000..233b70f7 --- /dev/null +++ b/project/ticket-192/README.md @@ -0,0 +1,64 @@ +# Ticket 192: Reduce cyclomatic complexity: merge_missing_ticket_records (CC=25) + +- **ID**: ticket-192 +- **Owner**: unresolved:human +- **Status**: IN_PROGRESS +- **Workflow state**: EDIT +- **Created**: 2026-09-20 + +## Goal and scope + +SESSION_EXECUTION_AUTHORIZATION: the planfile queue owner handed STARTER-611 +to this session with explicit completion instructions (checks, commit, +`planfile ticket done STARTER-611`). + +`project/analysis.toon.yaml` reports `merge_missing_ticket_records` defined in +`src/koru/planfile_compat.py:106` with cyclomatic complexity 25 (limit 15). +Extract identifier/status normalization, reported-id collection, the coverage +test, diagnostic decoration, the recovery loop and report assembly into +focused module-level helpers with unchanged behaviour, bringing the function +under the limit 15. + +## Acceptance criteria + +- [x] AC-01: `merge_missing_ticket_records` cyclomatic complexity is below 15 + (code2llm re-run on this checkout, output outside the repo tree). +- [x] AC-02: `tests/test_planfile_compat.py` plus the existing consumer + suites (`tests/test_queue_clean.py`, `tests/test_context.py`, + `tests/test_planfile_queue.py`) pass. +- [x] AC-03: `ruff check` passes on `src/koru/planfile_compat.py` and + `tests/test_planfile_compat.py`. +- [x] AC-04: `bash project/governance-check.sh --base origin/main` passes. + +## Validation evidence + +Recorded 2026-09-20 in `.worktrees/ticket-192--planfile-compat-complexity` +(branch `ticket/192-planfile-compat-complexity`, base `origin/main` = `17270faf`): + +- AC-01: `code2llm -f all -o /tmp/opencode/code2llm-ticket192 + --no-chunk --exclude *.md --exclude plugins` — the CC report no longer lists + `merge_missing_ticket_records` (module row `planfile_compat CC=10`). + Independent AST recount: `merge_missing_ticket_records` CC 25 → 3; every + extracted helper ≤ 6 (`_recover_missing_records` 6, `_decorated_recovery` 5, + `_ticket_identifier` 3, `_reported_ticket_ids` 3, `_already_covered` 3, + `_compatibility_report` 3, `_normalized_status` 2). Behaviour differential + old-vs-new over 2420 generated scenarios (5 sprint layouts × status matrix × + reported-payload shapes, including corrupt YAML) is byte-identical for the + merged list and `report.to_dict()`. +- AC-02: `PYTHONPATH=src python -m pytest tests/test_planfile_compat.py + tests/test_queue_clean.py tests/test_context.py tests/test_planfile_queue.py + tests/test_cqrs_planfile_queue_context.py -q` — new suite (13 tests) passes; + consumer suites pass except three pre-existing `TestPlanfileQueueLlm` + transport failures that reproduce identically on the pristine accepted base + (verified via `git stash -u` comparison) and are unrelated to this module. +- AC-03: `ruff check src/koru/planfile_compat.py tests/test_planfile_compat.py` + — All checks passed. `ruff format --check` remaining finding is the + pre-existing `unknown_status_count` span, unchanged verbatim from the + accepted base. +- AC-04: `bash project/governance-check.sh --base origin/main` — + `GOV-PASS: passed (0 errors, 0 warnings)`. + +## Tracking boundary + +This directory contains the minimal reviewed intent. Optional participant prose +and raw command logs are not required delivery output. diff --git a/project/ticket-192/intent.json b/project/ticket-192/intent.json new file mode 100644 index 00000000..573f0061 --- /dev/null +++ b/project/ticket-192/intent.json @@ -0,0 +1,97 @@ +{ + "schema": "new-project.intent/v3", + "ticket": "ticket-192", + "summary": "Reduce cyclomatic complexity: merge_missing_ticket_records (CC=25)", + "workstream": "application", + "classification": { + "kind": "SERVICE", + "priority": "P2", + "origin": "health" + }, + "allowedPaths": [ + "project/ticket-192/**", + "TODO.md", + "project/TICKETS.md", + "src/koru/planfile_compat.py", + "tests/test_planfile_compat.py" + ], + "forbiddenPaths": [ + "project/ticket-*/user-*.md" + ], + "stacks": [], + "dependsOn": [], + "conflictsWith": [], + "integrationTicket": null, + "delivery": { + "acceptedBaseSha": "17270fafdea8003c07d0dfd2a44fcb4b1867e6a4", + "targetBranch": "main", + "outcome": "merge_missing_ticket_records drops from cyclomatic complexity 25 to below the code2llm limit 15 by extracting behavior-preserving module-level helpers (_ticket_identifier, _normalized_status, _reported_ticket_ids, _already_covered, _decorated_recovery, _recover_missing_records, _compatibility_report). Record pairing (id/ticket_id fallback, strip, dedupe, reported-payload authority), legacy/unknown status decoration, sprint marker removal, report counting and read-error propagation are byte-for-byte equivalent; a new tests/test_planfile_compat.py pins the behavior and the existing consumer suites pass unmodified.", + "nonGoals": [ + "Changing any observable merged record field, diagnostic payload or report count", + "Regenerating project/analysis.toon.yaml or project/planfile-tickets.yaml (integration-owned artifacts)", + "Renaming or removing any public name in koru.planfile_compat", + "Reformatting pre-existing spans of src/koru/planfile_compat.py outside the refactored function" + ], + "complexity": "S", + "estimatedMinutes": 30, + "budgets": { + "maxImplementationFiles": 2, + "maxAffectedComponents": 1, + "maxPublicInterfaceChanges": 0, + "maxRuntimeDependencies": 0 + }, + "architecture": { + "status": "accepted", + "decision": "Decompose merge_missing_ticket_records into single-responsibility private module functions mirroring the module's existing function-oriented style: identifier/status normalization, reported-id collection, coverage test, diagnostic decoration, the recovery loop and report assembly. The public function keeps only source loading and composition; each helper returns explicit values so behavior is equivalent and the public surface is unchanged.", + "components": [ + { + "name": "planfile-compat", + "paths": [ + "src/koru/planfile_compat.py", + "tests/test_planfile_compat.py" + ] + } + ], + "responsibilityChanges": false, + "interfaceChanges": [], + "dataChanges": [], + "ui": { + "impact": "none", + "states": [], + "evidence": [] + }, + "rollback": "Revert the commit; the original single merge_missing_ticket_records body is restored verbatim." + }, + "runtimeDependencies": [], + "validation": [ + { + "criterion": "AC-01", + "commands": [ + "code2llm -f all -o /tmp/opencode/code2llm-ticket192 --no-chunk --exclude *.md --exclude plugins (complexity re-measured on merge_missing_ticket_records)" + ], + "evidence": "Re-run code2llm reports CC < 15 for merge_missing_ticket_records." + }, + { + "criterion": "AC-02", + "commands": [ + "PYTHONPATH=src python -m pytest tests/test_planfile_compat.py tests/test_queue_clean.py tests/test_context.py tests/test_planfile_queue.py -q" + ], + "evidence": "New behavior-pinning suite and existing consumer suites pass unmodified." + }, + { + "criterion": "AC-03", + "commands": [ + "ruff check src/koru/planfile_compat.py tests/test_planfile_compat.py" + ], + "evidence": "Lint passes on the refactored module and the new test module." + }, + { + "criterion": "AC-04", + "commands": [ + "bash project/governance-check.sh --base origin/main" + ], + "evidence": "Governance gate passes with zero errors." + } + ] + } +} diff --git a/src/koru/planfile_compat.py b/src/koru/planfile_compat.py index e53eb860..8258245f 100644 --- a/src/koru/planfile_compat.py +++ b/src/koru/planfile_compat.py @@ -103,6 +103,91 @@ def _raw_ticket_records(project: Path) -> tuple[list[dict[str, Any]], tuple[str, return records, tuple(errors) +def _ticket_identifier(record: dict[str, Any]) -> str: + """Return the normalized id used to pair raw and reported records.""" + return str(record.get("id") or record.get("ticket_id") or "").strip() + + +def _normalized_status(record: dict[str, Any]) -> str: + return str(record.get("status") or "").strip().lower() + + +def _reported_ticket_ids(reported_records: list[dict[str, Any]]) -> set[str]: + ids: set[str] = set() + for item in reported_records: + ticket_id = _ticket_identifier(item) + if ticket_id: + ids.add(ticket_id) + return ids + + +def _already_covered(ticket_id: str, reported_ids: set[str], seen_raw: set[str]) -> bool: + return not ticket_id or ticket_id in reported_ids or ticket_id in seen_raw + + +def _decorated_recovery(record: dict[str, Any], status: str) -> dict[str, Any]: + """Copy a raw record and attach the operator-facing status diagnostic.""" + recovered_record = dict(record) + recovered_record.pop("_compat_sprint", None) + if status == LEGACY_STATUS: + recovered_record["legacy_status"] = LEGACY_STATUS + recovered_record["status_diagnostic"] = { + "kind": LEGACY_STATUS_DIAGNOSTIC, + "status": LEGACY_STATUS, + "action": "run koru queue migrate-legacy-skipped --apply", + } + elif status not in SUPPORTED_TICKET_STATUSES: + recovered_record["legacy_status"] = status or None + recovered_record["status_diagnostic"] = { + "kind": "unknown-status", + "status": status or None, + "action": "inspect and migrate explicitly; no automatic mutation", + } + return recovered_record + + +def _recover_missing_records( + raw_records: list[dict[str, Any]], + reported_ids: set[str], +) -> tuple[list[dict[str, Any]], Counter[str]]: + """Build decorated records for raw tickets the validated payload omitted.""" + recovered_records: list[dict[str, Any]] = [] + status_counts: Counter[str] = Counter() + seen_raw: set[str] = set() + + for raw in raw_records: + ticket_id = _ticket_identifier(raw) + if _already_covered(ticket_id, reported_ids, seen_raw): + continue + seen_raw.add(ticket_id) + status = _normalized_status(raw) + if status == LEGACY_STATUS or status not in SUPPORTED_TICKET_STATUSES: + status_counts[status or ""] += 1 + recovered_records.append(_decorated_recovery(raw, status)) + + return recovered_records, status_counts + + +def _compatibility_report( + *, + raw_count: int, + reported_count: int, + recovered_count: int, + status_counts: Counter[str], + read_errors: tuple[str, ...], +) -> PlanfileCompatibilityReport: + return PlanfileCompatibilityReport( + raw_ticket_count=raw_count, + reported_ticket_count=reported_count, + recovered_ticket_count=recovered_count, + legacy_status_counts=dict(sorted(status_counts.items())), + unknown_status_count=sum( + count for status, count in status_counts.items() if status != LEGACY_STATUS + ), + read_errors=read_errors, + ) + + def merge_missing_ticket_records( reported: list[dict[str, Any]], project: Path, @@ -117,54 +202,14 @@ def merge_missing_ticket_records( """ raw_records, read_errors = _raw_ticket_records(project.resolve()) reported_records = [item for item in reported if isinstance(item, dict)] - reported_ids = { - str(item.get("id") or item.get("ticket_id") or "").strip() - for item in reported_records - if str(item.get("id") or item.get("ticket_id") or "").strip() - } - merged = list(reported_records) - recovered = 0 - status_counts: Counter[str] = Counter() - seen_raw: set[str] = set() - - for raw in raw_records: - ticket_id = str(raw.get("id") or raw.get("ticket_id") or "").strip() - if not ticket_id or ticket_id in reported_ids or ticket_id in seen_raw: - continue - seen_raw.add(ticket_id) - status = str(raw.get("status") or "").strip().lower() - if status == LEGACY_STATUS or status not in SUPPORTED_TICKET_STATUSES: - status_counts[status or ""] += 1 - recovered_record = dict(raw) - recovered_record.pop("_compat_sprint", None) - if status == LEGACY_STATUS: - recovered_record["legacy_status"] = LEGACY_STATUS - recovered_record["status_diagnostic"] = { - "kind": LEGACY_STATUS_DIAGNOSTIC, - "status": LEGACY_STATUS, - "action": "run koru queue migrate-legacy-skipped --apply", - } - elif status not in SUPPORTED_TICKET_STATUSES: - recovered_record["legacy_status"] = status or None - recovered_record["status_diagnostic"] = { - "kind": "unknown-status", - "status": status or None, - "action": "inspect and migrate explicitly; no automatic mutation", - } - merged.append(recovered_record) - recovered += 1 - - report = PlanfileCompatibilityReport( - raw_ticket_count=len(raw_records), - reported_ticket_count=len(reported_records), - recovered_ticket_count=recovered, - legacy_status_counts=dict(sorted(status_counts.items())), - unknown_status_count=sum( - count for status, count in status_counts.items() if status != LEGACY_STATUS - ), + recovered_records, status_counts = _recover_missing_records(raw_records, _reported_ticket_ids(reported_records)) + return list(reported_records) + recovered_records, _compatibility_report( + raw_count=len(raw_records), + reported_count=len(reported_records), + recovered_count=len(recovered_records), + status_counts=status_counts, read_errors=read_errors, ) - return merged, report __all__ = [ diff --git a/tests/test_planfile_compat.py b/tests/test_planfile_compat.py new file mode 100644 index 00000000..49d1c29e --- /dev/null +++ b/tests/test_planfile_compat.py @@ -0,0 +1,303 @@ +"""Behavior pinning for `koru.planfile_compat.merge_missing_ticket_records`. + +The merge must stay a read-only compatibility layer: reported (validated) +records stay authoritative, raw sprint records the CLI could not parse are +recovered with an operator-facing diagnostic, and the report counts stay +derivable from the recovered payload alone. +""" + +from __future__ import annotations + +from pathlib import Path + +from koru.planfile_compat import ( + LEGACY_STATUS, + PlanfileCompatibilityReport, + merge_missing_ticket_records, +) + + +def write_sprint(project: Path, name: str, body: str) -> None: + sprint_dir = project / ".planfile" / "sprints" + sprint_dir.mkdir(parents=True, exist_ok=True) + (sprint_dir / f"{name}.yaml").write_text(body, encoding="utf-8") + + +def test_no_sprint_directory_returns_empty_report(tmp_path: Path) -> None: + merged, report = merge_missing_ticket_records([], tmp_path) + + assert merged == [] + assert report == PlanfileCompatibilityReport() + assert report.migration_candidate_count == 0 + + +def test_legacy_skipped_record_is_recovered_with_diagnostic(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """sprint: + tickets: + PLF-1: + id: PLF-1 + name: Historical work + status: skipped +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + assert [record["id"] for record in merged] == ["PLF-1"] + record = merged[0] + assert record["legacy_status"] == "skipped" + assert record["status_diagnostic"] == { + "kind": "legacy-status", + "status": "skipped", + "action": "run koru queue migrate-legacy-skipped --apply", + } + assert "_compat_sprint" not in record + assert report.raw_ticket_count == 1 + assert report.reported_ticket_count == 0 + assert report.recovered_ticket_count == 1 + assert report.migration_candidate_count == 1 + assert report.legacy_status_counts == {"skipped": 1} + assert report.unknown_status_count == 0 + assert report.read_errors == () + + +def test_unknown_status_is_recovered_without_automatic_mutation(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-2: + ticket_id: PLF-2 + name: Weird state + status: Frozen +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + record = merged[0] + assert record["legacy_status"] == "frozen" + assert record["status_diagnostic"] == { + "kind": "unknown-status", + "status": "frozen", + "action": "inspect and migrate explicitly; no automatic mutation", + } + assert report.legacy_status_counts == {"frozen": 1} + assert report.unknown_status_count == 1 + assert report.migration_candidate_count == 0 + + +def test_missing_status_counts_as_missing_and_recovers_undecorated_field(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-3: + id: PLF-3 + name: No status at all +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + record = merged[0] + assert record["legacy_status"] is None + assert record["status_diagnostic"] == { + "kind": "unknown-status", + "status": None, + "action": "inspect and migrate explicitly; no automatic mutation", + } + assert report.legacy_status_counts == {"": 1} + assert report.unknown_status_count == 1 + + +def test_supported_status_is_recovered_without_diagnostic(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-4: + id: PLF-4 + status: in_progress + PLF-5: + id: PLF-5 + status: Ready +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + by_id = {record["id"]: record for record in merged} + assert set(by_id) == {"PLF-4", "PLF-5"} + for record in merged: + assert "legacy_status" not in record + assert "status_diagnostic" not in record + assert report.recovered_ticket_count == 2 + assert report.legacy_status_counts == {} + assert report.unknown_status_count == 0 + + +def test_reported_payload_stays_authoritative_for_shared_ids(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-6: + id: PLF-6 + status: skipped + PLF-7: + ticket_id: PLF-7 + status: open +""", + ) + reported = [ + {"id": "PLF-6", "status": "done", "source": "cli"}, + {"ticket_id": " PLF-7 ", "status": "review"}, + ] + + merged, report = merge_missing_ticket_records(reported, tmp_path) + + assert merged[:2] == reported + assert report.recovered_ticket_count == 0 + assert report.reported_ticket_count == 2 + assert report.raw_ticket_count == 2 + assert report.legacy_status_counts == {} + + +def test_non_dict_reported_entries_are_ignored(tmp_path: Path) -> None: + merged, report = merge_missing_ticket_records(["nonsense", None, 3], tmp_path) + + assert merged == [] + assert report.reported_ticket_count == 0 + + +def test_duplicate_raw_ids_are_recovered_once(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "alpha", + """tickets: + PLF-8: + id: PLF-8 + status: skipped +""", + ) + write_sprint( + tmp_path, + "beta", + """tickets: + PLF-8: + id: PLF-8 + status: skipped +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + assert [record["id"] for record in merged] == ["PLF-8"] + assert report.raw_ticket_count == 2 + assert report.recovered_ticket_count == 1 + assert report.legacy_status_counts == {"skipped": 1} + + +def test_mapping_key_is_the_last_id_fallback_and_empty_ids_are_dropped(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-9: + name: id only in mapping key + status: open + PLF-10: + id: "" + status: open +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + assert [record["id"] for record in merged] == ["PLF-9"] + assert report.raw_ticket_count == 2 + assert report.recovered_ticket_count == 1 + + +def test_corrupt_sprint_file_reports_read_error_and_keeps_going(tmp_path: Path) -> None: + write_sprint(tmp_path, "broken", "tickets: [unclosed") + write_sprint( + tmp_path, + "healthy", + """tickets: + PLF-11: + id: PLF-11 + status: skipped +""", + ) + + merged, report = merge_missing_ticket_records([], tmp_path) + + assert [record["id"] for record in merged] == ["PLF-11"] + assert len(report.read_errors) == 1 + assert report.read_errors[0].startswith("broken.yaml: ") + + +def test_non_dict_documents_and_values_are_skipped(tmp_path: Path) -> None: + write_sprint(tmp_path, "list-root", "- a\n- b\n") + write_sprint(tmp_path, "tickets-list", "tickets:\n - id: PLF-12\n") + write_sprint(tmp_path, "scalar-ticket", "tickets:\n PLF-13: skipped\n") + + merged, report = merge_missing_ticket_records([], tmp_path) + + assert merged == [] + assert report.raw_ticket_count == 0 + + +def test_input_list_is_not_mutated_and_result_is_new(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-14: + id: PLF-14 + status: skipped +""", + ) + reported = [{"id": "PLF-KEEP", "status": "open"}] + snapshot = [dict(item) for item in reported] + + merged, report = merge_missing_ticket_records(reported, tmp_path) + + assert reported == snapshot + assert merged is not reported + assert merged[0] == snapshot[0] + assert report.recovered_ticket_count == 1 + + +def test_report_to_dict_round_trip(tmp_path: Path) -> None: + write_sprint( + tmp_path, + "current", + """tickets: + PLF-15: + id: PLF-15 + status: skipped + PLF-16: + id: PLF-16 + status: archived +""", + ) + + _merged, report = merge_missing_ticket_records([], tmp_path) + + payload = report.to_dict() + assert payload == { + "raw_ticket_count": 2, + "reported_ticket_count": 0, + "recovered_ticket_count": 2, + "migration_candidate_count": 1, + "legacy_status_counts": {"archived": 1, "skipped": 1}, + "unknown_status_count": 1, + "read_errors": [], + } + assert report.migration_candidate_count == report.legacy_status_counts.get(LEGACY_STATUS)