drew
0bc734c020
Applies `ruff format` to the accumulated formatting debt on this branch. Formatting-only — no behavioral changes. Required for CI/lint's format gate (`nox -s format -- --check`), which the branch was failing on 288 tracked files that drifted from ruff's canonical style. In-progress WIP files are intentionally excluded so this commit stays a clean formatting-only diff. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
384 lines
16 KiB
Python
384 lines
16 KiB
Python
"""Typed prompt-time context handed off from
|
|
``_review_prompt.build_review_prompt`` to ``_review_finalize.finalize_review``
|
|
(via the ``item['_dispatcher_review_context']`` carrier on the dispatch
|
|
``item`` dict).
|
|
|
|
Why a dedicated module
|
|
----------------------
|
|
|
|
Both writers (``_review_prompt``) and readers (``_review_finalize``,
|
|
``dispatch_review``) need the type. Putting the definition in a
|
|
neutral sibling module avoids a producer-imports-consumer cycle and
|
|
gives the dataclass its own focused unit-of-change boundary: a future
|
|
field addition (e.g. another telemetry counter) lands in this file,
|
|
plus one line in each producer and consumer.
|
|
|
|
Why a dataclass instead of a plain dict
|
|
---------------------------------------
|
|
|
|
The original implementation passed a dict and let each caller use
|
|
``.get()`` defensively. That made:
|
|
|
|
- Adding a new field a churny operation (touch every reader to
|
|
``.get`` the new key with a default).
|
|
- Refactor-safety weak — a typo in the key on either side would
|
|
silently fall back to the default, masking the bug.
|
|
- The contract implicit (consumers had to read producer code to
|
|
discover what keys were available).
|
|
|
|
The dataclass surfaces all three at the type system: adding a field
|
|
is a single edit, typos at the read site become attribute errors, and
|
|
the schema is discoverable in one place.
|
|
|
|
Backward compatibility note
|
|
---------------------------
|
|
|
|
``finalize_review`` and ``dispatch_review._action`` accept either a
|
|
``ReviewContext`` instance OR a plain dict on
|
|
``item['_dispatcher_review_context']``. Direct unit-test invocations
|
|
that build dict literals continue to work; tests that build a
|
|
dataclass also work. The :func:`coerce_review_context` helper does
|
|
the dispatch.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import logging
|
|
from dataclasses import dataclass, field, fields
|
|
from typing import Any, Callable
|
|
|
|
|
|
_logger = logging.getLogger("review_pipeline")
|
|
|
|
|
|
class ReviewContextStrictError(TypeError):
|
|
"""Raised by :func:`coerce_review_context` in ``strict=True`` mode
|
|
when the caller stamps a dict with key(s) that are not declared on
|
|
:class:`ReviewContext`.
|
|
|
|
Why strict mode exists
|
|
----------------------
|
|
|
|
The production producer (``_review_prompt``) constructs
|
|
:class:`ReviewContext` directly. Python's dataclass already raises
|
|
``TypeError`` for unknown kwargs at construction time, so a
|
|
producer-side typo cannot reach the production read site —
|
|
that path is type-safe by language guarantee. Strict mode is
|
|
therefore a **test-fixture safety net**, not a production-typo
|
|
trap. The only carriers that arrive at the read site as plain
|
|
dicts are unit-test fixtures and direct-invocation callers; a
|
|
stale test fixture with a typo'd key (``{"head_shaa": "abc"}``)
|
|
would otherwise silently default ``head_sha=""`` and mask the
|
|
fixture drift behind a passing test. Strict mode is enabled at
|
|
the production read sites (``_review_finalize.finalize_review``
|
|
and ``dispatch_review._action``) so a stale fixture exercising
|
|
those code paths fails loudly instead of corrupting
|
|
cycle-archive telemetry / the freshness check.
|
|
|
|
Lenient mode (the default) preserves the back-compat property
|
|
that legacy fixtures pre-dating a future field rename keep
|
|
compiling — required so a ``ReviewContext`` rename does not
|
|
cascade-break dozens of unrelated tests on the same diff.
|
|
"""
|
|
|
|
|
|
# Sentinel for "the field expects an empty string when the dict
|
|
# carrier supplies a falsy value." We can't share a function literal
|
|
# across the module because dataclass field defaults must be
|
|
# call-time-distinct, so we lift the str-or-empty coercion into
|
|
# named callables that are easy to reference in the per-field
|
|
# coercion table below.
|
|
def _coerce_str_or_empty(value: Any) -> str:
|
|
"""Return ``str(value)`` for any truthy ``value``, ``""`` otherwise.
|
|
|
|
Pinned shape for ``head_sha``: an empty string is the documented
|
|
"no context" sentinel that the freshness check treats as "skip".
|
|
A None or any other falsy carrier value collapses to that
|
|
sentinel.
|
|
"""
|
|
return str(value or "")
|
|
|
|
|
|
def _coerce_int_or_zero(value: Any) -> int:
|
|
"""Return ``int(value)`` if ``value`` is already a builtin ``int``,
|
|
``0`` otherwise.
|
|
|
|
The narrow ``type(value) is int`` check intentionally rejects
|
|
``int`` subclasses for two reasons:
|
|
|
|
- **bool**: ``isinstance(True, int)`` is True. An ``isinstance``
|
|
check would silently map a stamped ``True`` to 1 and ``False``
|
|
to 0 — semantically wrong for a count field. Flooring to 0 on
|
|
a bool carrier surfaces the producer-side type confusion as a
|
|
zero-count rather than a fictional non-zero count.
|
|
- **numpy / pandas integer types** (``numpy.int64``,
|
|
``pd.Int64Dtype``, etc.) also subclass ``int``. Producers in
|
|
this pipeline only stamp Python builtin ints, but the strict
|
|
check rejects a future caller who pipes a pandas dataframe
|
|
column through verbatim — that caller would silently get 0
|
|
and should know about it. If a future producer legitimately
|
|
needs to stamp ``numpy.int64`` counts, widen this coercer
|
|
with an explicit ``isinstance`` branch (and update the
|
|
``request_changes_count`` test surface) rather than weakening
|
|
the bool guard.
|
|
"""
|
|
return int(value) if type(value) is int else 0
|
|
|
|
|
|
def _coerce_list_of_dict(value: Any) -> list[dict[str, Any]]:
|
|
"""Pass-through for a list; ``[]`` for anything else. Used by
|
|
``pr_comments`` so a non-list carrier value doesn't crash
|
|
downstream consumers that iterate the list."""
|
|
return value if isinstance(value, list) else []
|
|
|
|
|
|
def _coerce_bool(value: Any) -> bool:
|
|
"""Standard truthiness coercion. Lifted into a named callable
|
|
only for symmetry with the other coercers below."""
|
|
return bool(value)
|
|
|
|
|
|
def _coerce_dict_of_int(value: Any) -> dict[str, int]:
|
|
"""Coerce a dict carrier of ``{token: count}`` to ``dict[str,
|
|
int]``. Non-dict / None values flatten to an empty dict. Per-key
|
|
int-coercion preserves zero values so the cycle archive's
|
|
"stamp zeros" contract for context-present-but-empty-counts is
|
|
upheld."""
|
|
if not isinstance(value, dict):
|
|
return {}
|
|
out: dict[str, int] = {}
|
|
for k, v in value.items():
|
|
if v is None:
|
|
continue
|
|
try:
|
|
out[str(k)] = int(v)
|
|
except (TypeError, ValueError):
|
|
continue
|
|
return out
|
|
|
|
|
|
def _coerce_passthrough(value: Any) -> Any:
|
|
"""Forward ``value`` unchanged. Used for fields whose carrier
|
|
payload is in-memory only (e.g. :attr:`ReviewContext.clone_handle`,
|
|
a worktree handle the dispatcher's ``finally`` block calls
|
|
``cleanup`` on) and therefore must NOT be coerced — touching the
|
|
object would either lose identity (the caller may have captured
|
|
a reference for cleanup) or break a non-JSON-serialisable
|
|
payload. Listed in :data:`_FIELD_COERCERS` explicitly rather
|
|
than via a fall-through default so the table's invariant —
|
|
``set(_FIELD_COERCERS) == {f.name for f in fields(ReviewContext)}``
|
|
— stays a tight equality (caught by the module-load assertion
|
|
below)."""
|
|
return value
|
|
|
|
|
|
# Per-field coercion table: maps each declared field on
|
|
# :class:`ReviewContext` to the callable that converts a dict
|
|
# carrier's raw value into the typed field value. Keeping the table
|
|
# next to the dataclass makes a field rename a one-line edit
|
|
# (the reader loop in :func:`coerce_review_context` is
|
|
# fields()-driven and therefore picks up the new field
|
|
# automatically), and lets each coercer state its own narrow
|
|
# contract. Fields whose carrier value is in-memory only (and must
|
|
# not be coerced) get :func:`_coerce_passthrough` rather than a
|
|
# silent fall-through, so the table contract stays a strict
|
|
# equality with the dataclass field set.
|
|
_FIELD_COERCERS: dict[str, Callable[[Any], Any]] = {
|
|
"head_sha": _coerce_str_or_empty,
|
|
"request_changes_count": _coerce_int_or_zero,
|
|
"pr_comments": _coerce_list_of_dict,
|
|
"data_complete": _coerce_bool,
|
|
"unresolved_link_counts": _coerce_dict_of_int,
|
|
"clone_handle": _coerce_passthrough,
|
|
}
|
|
|
|
|
|
@dataclass
|
|
class ReviewContext:
|
|
"""Prompt-time review context the dispatcher stamps on the dispatch
|
|
``item`` so the post-session action sees the same values without
|
|
re-querying Forgejo.
|
|
|
|
Fields
|
|
------
|
|
|
|
head_sha:
|
|
The PR head SHA captured at prompt-build time. Used for the
|
|
post-session freshness check (worker verdict vs. current
|
|
upstream SHA).
|
|
request_changes_count:
|
|
Active (non-dismissed) REQUEST_CHANGES count from the
|
|
existing-reviews fetch. Threaded so the post-session
|
|
Tier-1F threshold check does not re-fetch the review list.
|
|
pr_comments:
|
|
The pre-fetched issue-style PR comments. Used by the
|
|
operator-status poster's duplicate-suppression fingerprint
|
|
lookup.
|
|
data_complete:
|
|
Aggregate ``data_complete`` flag across every pre-fetched
|
|
section. False when any section was truncated, paginated
|
|
partially, or had its upstream fetch fail. The dispatcher
|
|
defensively downgrades APPROVED → COMMENT when this is
|
|
False.
|
|
unresolved_link_counts:
|
|
Telemetry dict mapping each
|
|
:class:`_review_fetch.UnresolvedReason` token (e.g.
|
|
``"not-found"``, ``"fetch-error"``) to the count of
|
|
placeholder entries the fetcher synthesised for that reason.
|
|
Replaces three flat ``unresolved_link_count_*`` int fields
|
|
from a previous round; a future reason now lands by
|
|
appending an :class:`UnresolvedReason` to the registry tuple
|
|
in ``_review_fetch.py`` (no corresponding
|
|
:class:`ReviewContext` field edit). The post-session action
|
|
emits flattened ``unresolved_link_count_<token>`` keys onto
|
|
every cycle archive so the operator-grep contract for the
|
|
existing tokens is preserved.
|
|
clone_handle:
|
|
The pre-clone worktree handle (or None when pre-clone was
|
|
disabled). The dispatcher's ``finally`` block calls
|
|
``cleanup`` on it. NOT JSON-serialisable; lives on
|
|
``ReviewContext`` only as an in-memory object and is never
|
|
embedded in the prompt or in cycle archives.
|
|
"""
|
|
|
|
head_sha: str = ""
|
|
request_changes_count: int = 0
|
|
pr_comments: list[dict[str, Any]] = field(default_factory=list)
|
|
data_complete: bool = True
|
|
unresolved_link_counts: dict[str, int] = field(default_factory=dict)
|
|
clone_handle: Any = None
|
|
|
|
@property
|
|
def unresolved_link_count_total(self) -> int:
|
|
"""Sum of every entry in :attr:`unresolved_link_counts`. The
|
|
``finalize_review`` telemetry path stamps this as
|
|
``unresolved_link_count`` on the cycle archive."""
|
|
return sum(self.unresolved_link_counts.values())
|
|
|
|
|
|
# Snapshot of the dataclass field set, used by strict-mode below.
|
|
# The frozenset gives O(1) per-key membership when subtracting
|
|
# ``set(carrier.keys())`` from it; the difference itself is O(n_dict
|
|
# + n_fields) per call, but each membership check inside the
|
|
# difference is constant-time (vs. O(n_fields) per check if the
|
|
# left-hand side were a list). One snapshot at module-load is fine
|
|
# because the dataclass surface is stable for the process lifetime.
|
|
_REVIEW_CONTEXT_FIELDS: frozenset[str] = frozenset(
|
|
f.name for f in fields(ReviewContext)
|
|
)
|
|
|
|
|
|
def _coerce_dict_carrier(value: dict[str, Any], *, strict: bool) -> ReviewContext:
|
|
"""Inner helper: build a :class:`ReviewContext` from a dict
|
|
carrier. Pulled out so :func:`coerce_review_context` reads as a
|
|
short dispatch over carrier types and the field-iteration logic
|
|
has its own explanation block.
|
|
|
|
The loop is :data:`_FIELD_COERCERS`-driven so a new field on
|
|
:class:`ReviewContext` participates automatically: declare it on
|
|
the dataclass + add a coercer entry. There is no per-field
|
|
if-block in this function. Older revisions had a parallel
|
|
``if "<name>" in value: known["<name>"] = ...`` block per
|
|
field, which was a drift magnet — every new field had to be
|
|
touched in two places that could silently disagree.
|
|
"""
|
|
if strict:
|
|
unknown = set(value.keys()) - _REVIEW_CONTEXT_FIELDS
|
|
if unknown:
|
|
raise ReviewContextStrictError(
|
|
"coerce_review_context(strict=True): dict carrier carries "
|
|
f"unknown key(s) {sorted(unknown)!r} — likely a typo at "
|
|
"the producer site. Known fields: "
|
|
f"{sorted(_REVIEW_CONTEXT_FIELDS)!r}."
|
|
)
|
|
known: dict[str, Any] = {}
|
|
for f in fields(ReviewContext):
|
|
if f.name not in value:
|
|
continue
|
|
coercer = _FIELD_COERCERS.get(f.name)
|
|
if coercer is None:
|
|
# New field on ReviewContext without a matching coercer
|
|
# entry — fail at module-load via the assertion below.
|
|
# This branch is reachable only if the assertion is
|
|
# weakened, in which case the field is forwarded as-is
|
|
# so production does not regress.
|
|
known[f.name] = value[f.name]
|
|
continue
|
|
known[f.name] = coercer(value[f.name])
|
|
return ReviewContext(**known)
|
|
|
|
|
|
# Module-load invariant: every field declared on ReviewContext has a
|
|
# matching coercer. A new field added to the dataclass without a
|
|
# coercer entry would silently fall back to the as-is forwarding
|
|
# branch above, defeating the point of the typed-coercion table.
|
|
# Catching the omission at import time is cheap insurance.
|
|
assert _REVIEW_CONTEXT_FIELDS == set(_FIELD_COERCERS.keys()), (
|
|
"_FIELD_COERCERS drifted from ReviewContext fields: "
|
|
f"missing {_REVIEW_CONTEXT_FIELDS - set(_FIELD_COERCERS.keys())!r}, "
|
|
f"extra {set(_FIELD_COERCERS.keys()) - _REVIEW_CONTEXT_FIELDS!r}."
|
|
)
|
|
|
|
|
|
def coerce_review_context(value: Any, *, strict: bool = False) -> ReviewContext | None:
|
|
"""Return a :class:`ReviewContext` for either a dataclass instance
|
|
or a dict (back-compat for tests that still build dict literals),
|
|
or ``None`` for absent / wrong-shape values.
|
|
|
|
Parameters
|
|
----------
|
|
value:
|
|
Whatever was stashed on ``item['_dispatcher_review_context']``.
|
|
The dispatcher / finalize_review code paths pass either a
|
|
:class:`ReviewContext` (production) or a dict (test fixtures
|
|
and direct unit-test invocations).
|
|
strict:
|
|
When True, unknown dict keys raise
|
|
:class:`ReviewContextStrictError` so a typo at the write
|
|
site (``head_shaa`` instead of ``head_sha``) surfaces
|
|
immediately instead of silently defaulting one cycle
|
|
downstream. The production read sites
|
|
(``_review_finalize.finalize_review`` and
|
|
``dispatch_review._action``) opt in to strict mode; legacy
|
|
unit-test fixtures that pass dict literals can opt out by
|
|
passing ``strict=False`` so a future field rename on the
|
|
dataclass side doesn't break them in lockstep. Strict mode
|
|
only affects the dict path — :class:`ReviewContext` inputs
|
|
are returned unchanged regardless.
|
|
|
|
Notes
|
|
-----
|
|
A non-dict, non-:class:`ReviewContext` value (e.g. ``42``,
|
|
``"abc"``, ``[]``) returns ``None`` and emits a single
|
|
``WARNING`` to the ``review_pipeline`` logger. ``None`` itself
|
|
is treated as "absent" and returns ``None`` silently — that is
|
|
the documented "no context" path used by dry-run / direct-test
|
|
callers.
|
|
"""
|
|
if isinstance(value, ReviewContext):
|
|
return value
|
|
if value is None:
|
|
return None
|
|
if not isinstance(value, dict):
|
|
# A non-dict, non-dataclass value almost always indicates a
|
|
# programming error at the write site. Log it once at WARNING
|
|
# so the cycle archive carries the breadcrumb (the function
|
|
# still returns None and the caller falls back to defaults,
|
|
# so the cycle does not crash — but operators can grep for
|
|
# "review_context" warnings to find the offending caller).
|
|
_logger.warning(
|
|
"coerce_review_context: ignoring unexpected carrier type %r "
|
|
"(expected ReviewContext, dict, or None); review-cycle "
|
|
"telemetry will fall back to defaults",
|
|
type(value).__name__,
|
|
)
|
|
return None
|
|
return _coerce_dict_carrier(value, strict=strict)
|
|
|
|
|
|
__all__ = (
|
|
"ReviewContext",
|
|
"ReviewContextStrictError",
|
|
"coerce_review_context",
|
|
)
|