3f12e4140c
Two related fixes surfaced by the 2026-05-17 run-5 live observation: 1. **Cycle-cap signature now includes total_reviews count** so the reviewer's COMMENT-only path actually moves the signature. Before: the cap signature was ``sha + (approvals + has_active_RC + has_unaddressed_RC)`` — all three boolean axes ignore COMMENT reviews entirely. After: ``+ total_reviews`` term bumps on EVERY review submission. The ``data_complete=False → COMMENT downgrade`` path the reviewer takes in low-context cycles now reflects in the signature, so the cap stops firing falsely. Without this, run-5 observed 4 fresh PRs hitting count=5 in ~10 minutes — the reviewer was successfully posting reviews every cycle but the cap saw "no change" because COMMENT-only reviews don't bump approvals_count, has_active_RC, or has_unaddressed_RC. 2. **Implementer now sees ALL reviewer feedback**, not just active REQUEST_CHANGES. Before: ``fetch_pr_fix_context`` passed ``include_active_reviews=False`` so failing-CI PRs reached the implementer with zero reviewer data. ``fetch_request_changes_pr_context`` only included active RC reviews — COMMENT-only feedback was invisible in both code paths. After: ``fetch_pr_fix_context`` also includes reviews, AND the fetcher now partitions reviews into TWO buckets — the existing ``request_changes_reviews`` (active blocking RC, unchanged semantic) and a new ``comment_reviews`` field carrying every non-dismissed non-RC review (COMMENT / APPROVE). Both are persisted in the PR-context sentinel and exposed via ``implementer_pr_context.py``'s ``comment_reviews`` field. New prompt section ``## Pre-fetched reviewer comments and approvals`` renders the comment_reviews bucket with author / event / commit / body / inline comments + a postscript marking them as ADVISORY (not blocking, unlike the existing RC section). This closes the architectural gap where the reviewer and implementer pools could work in silos on the same PR — the reviewer's substantive prose feedback now reaches the implementer regardless of which work-group routed it. Files touched: - tools/_pr_classification_cache.py — total_reviews in classify_pr + reactivity composite; docstring updated. - tools/_implementer_prefetch.py — new comment_reviews + comment_reviews_completed fields; fetcher partitions reviews once; fetch_pr_fix_context now includes reviews. - tools/_implementer_prompt.py — _build_comment_reviews_section; wired into prompt assembly between RC and PR-comments sections. - tools/_pr_context_sentinel.py — comment_reviews in _to_dict. - tools/implementer_pr_context.py — comment_reviews accessor for the worker's handoff read path. - tests/auto_agents/test_pr_context_sentinel.py — expected_value_keys + fixture + round-trip test updated. - .opencode/agents/estimator-implementation.md — restored canonical section header levels (####) for downstream test compatibility after R3.1 rewrite. Full auto_agents suite: 2303 passing (+12 from this and adjacent work, none broken). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
421 lines
18 KiB
Python
421 lines
18 KiB
Python
"""Filesystem-mediated handoff for dispatcher-prefetched PR context.
|
|
|
|
Why this exists
|
|
---------------
|
|
|
|
Originally built to defeat OpenCode's per-``task``-hop prompt
|
|
summarisation in the pre-R3 wrapper chain
|
|
(``implementation-worker`` → ``tier-dispatcher`` → ``tier-0`` →
|
|
``task-implementor``), where by depth 3 only the diff section
|
|
reliably survived and every other prefetched section (PR description,
|
|
CI status, comments, reviews, linked issues, Epic body) was routinely
|
|
summarised away.
|
|
|
|
After the R3 wrapper-chain retirement (2026-05-17) the implementer
|
|
pool dispatches directly to ``task-implementor-tier-<slot>``
|
|
variants — no intervening ``task`` hops, so the prefetched body
|
|
generally survives intact in the worker's user prompt. This module
|
|
remains in place as the durable fallback: ``task-implementor``'s
|
|
contract still ALWAYS reads from the sentinel rather than the
|
|
prompt body (defensive against a future regression that re-introduces
|
|
summarisation), and the sentinel is the single source of truth that
|
|
:mod:`tools.implementer_pr_context` reads from.
|
|
|
|
Lifecycle:
|
|
|
|
- Dispatcher calls :func:`write` BEFORE creating the OpenCode session
|
|
(i.e. right after the prefetch completes, in
|
|
:func:`dispatch_implementer._prefetch_prompt`).
|
|
- Worker (specifically ``task-implementor`` via the
|
|
``implementer-pr-context`` skill) reads the sentinel via
|
|
``tools/implementer_pr_context.py read --pr <N> --field <f>``.
|
|
- Dispatcher calls :func:`delete` from its
|
|
``post_session_action`` cleanup so a subsequent cycle for the same
|
|
PR sees a fresh sentinel (or none) rather than a stale one.
|
|
|
|
The on-disk schema is versioned (:data:`SCHEMA_VERSION`) so the
|
|
script can refuse a sentinel it doesn't understand rather than
|
|
handing the worker a half-parsed JSON blob. Schema is documented in
|
|
the worker-side script's docstring and the ``implementer-pr-context``
|
|
skill's SKILL.md.
|
|
|
|
This module is import-only; the script entry point lives at
|
|
``tools/implementer_pr_context.py``. Keeping the writer and reader
|
|
in separate files lets the reader run as a thin standalone Python
|
|
script (low startup overhead, no dispatcher imports) and lets the
|
|
writer share the dispatcher's heavyweight dependencies.
|
|
"""
|
|
from __future__ import annotations
|
|
|
|
import datetime as _dt
|
|
import json
|
|
import logging
|
|
import os
|
|
import sys
|
|
from pathlib import Path
|
|
from typing import Any
|
|
|
|
_TOOLS_DIR = str(Path(__file__).resolve().parent)
|
|
if _TOOLS_DIR not in sys.path:
|
|
sys.path.insert(0, _TOOLS_DIR)
|
|
from _loader import ( # noqa: E402 type: ignore[import-not-found]
|
|
load_sibling as _load_sibling,
|
|
)
|
|
|
|
# Single source of truth for the per-section completion flag names.
|
|
# Defined in :mod:`_implementer_prefetch`; imported here so the
|
|
# sentinel writer's projection logic loops over the same tuple
|
|
# the fetcher's preamble loops over. A future contributor adding a
|
|
# new ``*_completed`` flag to the dataclass updates the tuple
|
|
# once — the preamble (``_init_completion_flags``), the writer
|
|
# (this module's ``_to_dict``), and the binding regression tests
|
|
# all pick it up automatically.
|
|
_prefetch = _load_sibling("_implementer_prefetch", "_implementer_prefetch.py")
|
|
COMPLETION_FLAG_NAMES: tuple[str, ...] = _prefetch.COMPLETION_FLAG_NAMES
|
|
|
|
_logger = logging.getLogger("pr_context_sentinel")
|
|
|
|
|
|
# Increment when the on-disk JSON shape changes. The worker-side
|
|
# script reads this and refuses any sentinel whose schema_version
|
|
# does not match — better to fall through to ``curl`` than to hand
|
|
# the worker a partially-readable blob.
|
|
SCHEMA_VERSION = 1
|
|
|
|
|
|
# Override via env var for the test suite and for an operator who
|
|
# wants to inspect / share / archive handoffs from a long-running
|
|
# dispatcher. Defaults to a kind-namespaced subdirectory of /tmp so
|
|
# the implementer's handoffs don't collide with reviewer's (if
|
|
# reviewer ever gains a parallel feature).
|
|
_DEFAULT_HANDOFF_DIR = Path("/tmp/cleveragents-implementer-handoff")
|
|
_HANDOFF_DIR_ENV = "IMPLEMENTER_DISPATCHER_PR_CONTEXT_DIR"
|
|
|
|
|
|
def handoff_dir() -> Path:
|
|
"""Resolve the on-disk directory the sentinels live in.
|
|
|
|
Override via ``IMPLEMENTER_DISPATCHER_PR_CONTEXT_DIR`` for tests
|
|
+ manual inspection. The directory is created on demand by
|
|
:func:`write`; callers don't need to mkdir themselves.
|
|
"""
|
|
return Path(os.environ.get(_HANDOFF_DIR_ENV) or str(_DEFAULT_HANDOFF_DIR))
|
|
|
|
|
|
def handoff_path(pr_number: int) -> Path:
|
|
"""Return the per-PR sentinel file path (one file per PR per
|
|
cycle). The worker-side script accepts ``--pr`` and uses the
|
|
same convention — see :mod:`tools.implementer_pr_context`.
|
|
"""
|
|
return handoff_dir() / f"pr-{int(pr_number)}.json"
|
|
|
|
|
|
def _truncate_for_sentinel(value: str, max_chars: int = 200_000) -> str:
|
|
"""Hard cap on individual string fields so a pathological PR body
|
|
or diff doesn't bloat the sentinel to gigabytes. 200 KB is well
|
|
above any real-world PR description / diff while staying small
|
|
enough to fit comfortably in tmpfs.
|
|
|
|
Truncation appends a ``[…truncated N bytes]`` marker so the
|
|
worker-side reader can flag the situation rather than silently
|
|
serving a clipped value.
|
|
"""
|
|
if not isinstance(value, str):
|
|
return ""
|
|
if len(value) <= max_chars:
|
|
return value
|
|
dropped = len(value) - max_chars
|
|
return value[:max_chars] + f"\n[…truncated {dropped} bytes from sentinel]"
|
|
|
|
|
|
_COMPLETION_DEFAULT_TRUE = True
|
|
|
|
|
|
def _read_flag(result: Any, name: str) -> bool:
|
|
"""Read a "good-when-True" status flag off ``result``.
|
|
|
|
Applies to ``*_completed`` flags AND ``data_complete`` —
|
|
every boolean on :class:`ImplementerPrefetchResult` whose
|
|
semantics are "True means the fetch succeeded / aggregate
|
|
is healthy". NOT for inverted flags like ``diff_truncated``
|
|
or ``diff_unavailable`` (True = bad state), which still use
|
|
raw ``bool(getattr(...))`` because the strict-True default
|
|
would mask their failure semantics.
|
|
|
|
Strict-True semantics: only returns False when the attribute
|
|
is explicitly ``False``. A missing attribute or ``None``
|
|
defaults to True. This is more conservative than the previous
|
|
``bool(getattr(..., True))`` which would silently flip None →
|
|
False — making a typo'd attribute name (e.g. ``result.epic_completed
|
|
= None``) silently flip the flag and serve "fetched failed"
|
|
semantics to the worker.
|
|
|
|
Tests that need to forcibly flip a flag False MUST set it to
|
|
the literal ``False``, not to a falsy proxy.
|
|
"""
|
|
val = getattr(result, name, _COMPLETION_DEFAULT_TRUE)
|
|
return val is not False
|
|
|
|
|
|
def _to_dict(result: Any) -> dict[str, Any]:
|
|
"""Project an ``ImplementerPrefetchResult`` into a plain dict
|
|
suitable for JSON serialisation.
|
|
|
|
Pure projection — does NOT mutate the result and does NOT
|
|
re-issue any Forgejo calls. If a field is missing from the
|
|
result (e.g. ``epic_issue`` is ``None`` for a PR with no Epic
|
|
reference), the projection records ``null`` so the worker-side
|
|
reader can distinguish "section was attempted, came back empty"
|
|
from "section was never attempted".
|
|
|
|
Completion flags are NOT spelled out per key in the projected
|
|
dict — the function builds the value-projection base then
|
|
overlays a ``*_completed`` entry for every name in
|
|
:data:`COMPLETION_FLAG_NAMES`. That keeps the dataclass, the
|
|
preamble (``_init_completion_flags``), the writer (this
|
|
function), and the regression tests on a single source of
|
|
truth: adding a new flag to the tuple automatically
|
|
propagates here.
|
|
"""
|
|
pr_details = getattr(result, "pr_details", None) or {}
|
|
head = pr_details.get("head") or {}
|
|
base = pr_details.get("base") or {}
|
|
# ─── pr_details-derivation invariant ────────────────────────
|
|
# ``description`` and ``title`` are extracted from the single
|
|
# ``pr_details`` payload, so their completion semantics MUST
|
|
# collapse into the underlying ``pr_details_completed`` flag.
|
|
# Three sentinel keys (``pr_details_completed``,
|
|
# ``title_completed``, ``description_completed``) project
|
|
# the same source flag — a deliberate redundancy that makes
|
|
# ``--field metadata`` self-documenting (operators don't have
|
|
# to know about the derivation) at the cost of one extra
|
|
# boolean each in the serialised payload.
|
|
#
|
|
# The base ``pr_details_completed`` is overlaid below by the
|
|
# ``COMPLETION_FLAG_NAMES`` loop; ``title_completed`` and
|
|
# ``description_completed`` are explicit derived keys here
|
|
# because they are NOT in the dataclass (and therefore not
|
|
# in the canonical tuple).
|
|
#
|
|
# Invariant: a future change that fetches title or description
|
|
# from a SEPARATE endpoint (so they can succeed / fail
|
|
# independently of pr_details) MUST break this collapse —
|
|
# introduce dedicated ``title_completed`` /
|
|
# ``description_completed`` fields on
|
|
# :class:`_implementer_prefetch.ImplementerPrefetchResult`,
|
|
# add them to ``COMPLETION_FLAG_NAMES``, and drop the
|
|
# derived keys below.
|
|
pr_details_completed = _read_flag(result, "pr_details_completed")
|
|
projected: dict[str, Any] = {
|
|
"title": pr_details.get("title") or "",
|
|
"title_completed": pr_details_completed,
|
|
"description": _truncate_for_sentinel(pr_details.get("body") or ""),
|
|
"description_completed": pr_details_completed,
|
|
"head_sha": getattr(result, "head_sha", "") or "",
|
|
"head_ref": head.get("ref") or "",
|
|
"base_ref": base.get("ref") or "",
|
|
"ci_status": getattr(result, "ci_status", None),
|
|
"ci_detail": list(getattr(result, "ci_detail", []) or []),
|
|
# ``pr_comments`` carries the BOUNDED view (most-recent N plus
|
|
# every human/reviewer comment — see
|
|
# ``_implementer_prefetch._bounded_comment_view``), not the raw
|
|
# list: a heavy PR like #30 accumulates 1000+ bot attempt
|
|
# comments and embedding them all bloats both the sentinel and
|
|
# the worker's ``--field comments`` read. The deterministic
|
|
# rollup of the comments dropped from the verbatim window lives
|
|
# in ``pr_comments_digest``.
|
|
"pr_comments": list(getattr(result, "pr_comments_view", []) or []),
|
|
"pr_comments_digest": dict(
|
|
getattr(result, "pr_comments_digest", {}) or {}
|
|
),
|
|
"request_changes_reviews": list(
|
|
getattr(result, "request_changes_reviews", []) or []
|
|
),
|
|
# R3.4 (2026-05-17): COMMENT/APPROVE reviews — the reviewer's
|
|
# advisory feedback. Persisted alongside RC reviews so the
|
|
# implementer's sentinel-fetch sees the full reviewer record.
|
|
"comment_reviews": list(
|
|
getattr(result, "comment_reviews", []) or []
|
|
),
|
|
"issue_body": _truncate_for_sentinel(
|
|
getattr(result, "issue_body", "") or ""
|
|
),
|
|
"issue_comments": list(getattr(result, "issue_comments", []) or []),
|
|
"linked_issues": list(getattr(result, "linked_issues", []) or []),
|
|
"epic": getattr(result, "epic_issue", None),
|
|
"diff": _truncate_for_sentinel(
|
|
getattr(result, "diff_text", "") or ""
|
|
),
|
|
"diff_truncated": bool(getattr(result, "diff_truncated", False)),
|
|
"diff_unavailable": bool(getattr(result, "diff_unavailable", False)),
|
|
"diff_info": dict(getattr(result, "diff_info", {}) or {}),
|
|
"data_complete": _read_flag(result, "data_complete"),
|
|
"error_kinds": list(getattr(result, "error_kinds", []) or []),
|
|
}
|
|
# Overlay every dataclass-side ``*_completed`` flag via the
|
|
# canonical tuple. A new flag added to ``COMPLETION_FLAG_NAMES``
|
|
# automatically appears in the sentinel without further
|
|
# changes here. Uses the strict-True ``_read_flag`` defensive
|
|
# so a typo'd attribute (or accidental ``None`` assignment)
|
|
# defaults to True (the safer fallback for the worker) rather
|
|
# than silently flipping to "fetch failed".
|
|
#
|
|
# The ``not in projected`` guard is a belt-and-braces defence
|
|
# against accidental schema-base collisions: if a future
|
|
# contributor adds (say) ``"epic_completed": ...`` to the
|
|
# value-projection base above and forgets to remove the
|
|
# corresponding entry from ``COMPLETION_FLAG_NAMES`` (or
|
|
# vice-versa), the overlay would silently clobber the base
|
|
# value. The assert fails loudly with the offending name so
|
|
# the contributor sees the conflict immediately. The two
|
|
# legitimately derived keys (``title_completed`` /
|
|
# ``description_completed``) are NOT in the canonical tuple
|
|
# so they bypass this loop entirely.
|
|
for name in COMPLETION_FLAG_NAMES:
|
|
assert name not in projected, (
|
|
f"_to_dict schema-base already contains '{name}' — "
|
|
f"either remove the explicit key from the value-projection "
|
|
f"base or remove the name from COMPLETION_FLAG_NAMES. The "
|
|
f"overlay loop would silently clobber the base value."
|
|
)
|
|
projected[name] = _read_flag(result, name)
|
|
return projected
|
|
|
|
|
|
def write(
|
|
*,
|
|
pr_number: int,
|
|
work_type: str,
|
|
work_group: str,
|
|
result: Any,
|
|
item: dict[str, Any] | None = None,
|
|
compliance_gaps: dict[str, Any] | None = None,
|
|
gate_preflight: dict[str, Any] | None = None,
|
|
recent_implementer_push: dict[str, Any] | None = None,
|
|
) -> Path | None:
|
|
"""Serialise the prefetch result for ``pr_number`` and write it
|
|
atomically to disk.
|
|
|
|
``result`` is the
|
|
:class:`_implementer_prefetch.ImplementerPrefetchResult` returned
|
|
by one of the per-work-group fetchers. ``work_type`` /
|
|
``work_group`` come from the dispatcher's caller-side context.
|
|
``item`` is forwarded only for the optional metadata block
|
|
(PR title, listing-time labels) — never reach back to Forgejo.
|
|
|
|
``compliance_gaps`` and ``gate_preflight`` are the deterministic
|
|
pre-check artefacts the dispatcher computes after prefetch (see
|
|
:mod:`_implementer_compliance` and :mod:`_implementer_gate_preflight`).
|
|
Passing them here serialises them into the sentinel under the
|
|
same JSON schema the worker reads via
|
|
``implementer_pr_context.py --field {compliance_gaps,gate_preflight}``.
|
|
Markdown stanzas in the prompt body get summarised away by the
|
|
intermediate tier agents (see :file:`task-implementor.md`); the
|
|
sentinel survives because the worker reads it directly off disk.
|
|
|
|
Returns the resolved path on success, or ``None`` on any I/O
|
|
or serialisation failure (and logs WARNING). The dispatcher's
|
|
caller treats a ``None`` return as best-effort and proceeds
|
|
with the cycle — the worker can still fall back to the prompt
|
|
content (or to ``curl``).
|
|
|
|
Atomic via ``write_text(tmp) + replace(target)`` so a concurrent
|
|
reader (the worker's script) never sees a half-written file.
|
|
"""
|
|
pr_number = int(pr_number)
|
|
target = handoff_path(pr_number)
|
|
item = item or {}
|
|
payload: dict[str, Any] = {
|
|
"schema_version": SCHEMA_VERSION,
|
|
"pr_number": pr_number,
|
|
"work_type": work_type,
|
|
"work_group": work_group,
|
|
"dispatcher_pid": os.getpid(),
|
|
"created_at": _dt.datetime.now(_dt.timezone.utc).isoformat(),
|
|
"listing_title": str(item.get("title") or ""),
|
|
**_to_dict(result),
|
|
}
|
|
# Deterministic-check sections (plan 2026-05-13). Binary
|
|
# contract: either the dispatcher computed the section this
|
|
# cycle (both the value AND the ``*_completed`` flag are
|
|
# written) or it did not (both keys are absent — worker's
|
|
# reader treats absence as _MISSING and falls through to its
|
|
# in-session discovery flow).
|
|
#
|
|
# No tri-state. An earlier iteration documented a third
|
|
# "tried-but-got-nothing" state (``completed=True`` with a
|
|
# ``null`` payload) but the compliance + preflight detectors
|
|
# always produce a structured dict when they run, so that
|
|
# state is unreachable. Removing the language keeps the
|
|
# reader's three-case generic contract (used by the prefetch
|
|
# fields) from leaking into these binary fields.
|
|
if compliance_gaps is not None:
|
|
payload["compliance_gaps"] = compliance_gaps
|
|
payload["compliance_gaps_completed"] = True
|
|
if gate_preflight is not None:
|
|
payload["gate_preflight"] = gate_preflight
|
|
payload["gate_preflight_completed"] = True
|
|
# C4 (2026-05-13): when the prior cycle pushed the current
|
|
# head_sha within the staleness window, surface the record so
|
|
# the worker knows it just contributed and CI-still-red means
|
|
# the fix didn't address the actual failing test.
|
|
if recent_implementer_push is not None:
|
|
payload["recent_implementer_push"] = recent_implementer_push
|
|
payload["recent_implementer_push_completed"] = True
|
|
tmp = target.with_suffix(target.suffix + ".tmp")
|
|
try:
|
|
target.parent.mkdir(parents=True, exist_ok=True)
|
|
tmp.write_text(
|
|
json.dumps(payload, indent=2, default=str), encoding="utf-8"
|
|
)
|
|
tmp.replace(target)
|
|
except (OSError, TypeError, ValueError) as e:
|
|
_logger.warning(
|
|
"PR context sentinel write failed for PR #%s at %s: %s",
|
|
pr_number, target, e,
|
|
)
|
|
# Clean up the .tmp file if it was partially written before
|
|
# the failure. Without this, a disk-full / partial-write
|
|
# leaves a ``pr-30.json.tmp`` orphan that a concurrent
|
|
# reader could in theory open (the orphan won't replace
|
|
# ``target`` so it's never the authoritative sentinel, but
|
|
# it accumulates over time on a busy dispatcher).
|
|
try:
|
|
tmp.unlink(missing_ok=True)
|
|
except OSError:
|
|
pass
|
|
return None
|
|
_logger.info(
|
|
"PR context sentinel written for PR #%s -> %s (data_complete=%s)",
|
|
pr_number, target, payload.get("data_complete"),
|
|
)
|
|
return target
|
|
|
|
|
|
def delete(pr_number: int) -> None:
|
|
"""Remove the per-PR sentinel.
|
|
|
|
Called by the dispatcher's ``post_session_action`` cleanup so a
|
|
subsequent cycle for the same PR doesn't see a stale handoff. A
|
|
missing file is a no-op (idempotent) — the cleanup contract
|
|
treats every reachable path as "I tried", and the next ``write``
|
|
overwrites whatever's there anyway.
|
|
"""
|
|
target = handoff_path(pr_number)
|
|
try:
|
|
target.unlink(missing_ok=True)
|
|
except OSError as e:
|
|
_logger.warning(
|
|
"PR context sentinel cleanup failed for PR #%s at %s: %s",
|
|
pr_number, target, e,
|
|
)
|
|
|
|
|
|
__all__ = (
|
|
"SCHEMA_VERSION",
|
|
"handoff_dir",
|
|
"handoff_path",
|
|
"write",
|
|
"delete",
|
|
)
|