From ad517fe389d2ed475a2ad063659ab1a0454693d6 Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Tue, 8 Sep 2026 00:13:53 -0700 Subject: [PATCH 1/2] Add semantic dedupe for approved follow-ups Bound semantic matching and existing-issue lookup to repository-scoped source context, preserve conservative fallback behavior, and sanitize publication artifacts. Agent-Issue-Provenance: v1 repo=wwind123/coding-review-agent-loop issue=490 flow=approved plan=8a9140f06804e4bf --- docs/local_agent_loop.md | 35 + helpers/skill_runner.py | 34 +- src/coding_review_agent_loop/cli.py | 55 ++ src/coding_review_agent_loop/config.py | 41 + src/coding_review_agent_loop/followups.py | 822 +++++++++++++++++- src/coding_review_agent_loop/github.py | 9 +- src/coding_review_agent_loop/managed_ci.py | 3 + src/coding_review_agent_loop/orchestrator.py | 62 ++ .../semantic_dedupe.py | 302 +++++++ tests/test_followups.py | 177 ++++ 10 files changed, 1481 insertions(+), 59 deletions(-) create mode 100644 src/coding_review_agent_loop/semantic_dedupe.py create mode 100644 tests/test_followups.py diff --git a/docs/local_agent_loop.md b/docs/local_agent_loop.md index b1e3d25..fea968d 100644 --- a/docs/local_agent_loop.md +++ b/docs/local_agent_loop.md @@ -2450,6 +2450,41 @@ be fixed before merge. - `fix-and-summarize`: send same-PR follow-ups to the coder for another review round, then summarize future follow-ups after final approval. - `fix-and-issue`: send same-PR follow-ups to the coder for another review round, then create issues for future follow-ups after final approval and comment with the created issue links. +Issue-filing modes use a conservative two-stage reconciliation. Deterministic +normalization, headings, identifiers, paths, and topic overlap run first. The +publisher then searches open follow-up trackers in the configured repository +with at most five focused queries, twenty results per query, and fifty unique +candidates per publication. Parent issue, approved-plan hash, PR, and related +links are preferred before a bounded repository-wide topic query. Search +results are repository/identity checked and the selected tracker is +revalidated as open immediately before reuse; closed or cross-repository +results never suppress a new issue. Search indexing is eventually consistent, +and there is no atomic repository-wide lock, so a create-then-interruption +window is recovered by bounded rediscovery on a later invocation. + +For ambiguous in-batch groups and narrowed existing trackers, an optional cheap +semantic classifier receives only bounded excerpts and must return strict JSON +with `duplicate_of`, `confidence`, and a non-empty `reason`. Only `high` +confidence equivalence of the actual deliverable suppresses or merges work. +Medium/low confidence files normally with a sanitized possible-duplicate note; +provider failures, invalid output, timeouts, and local budget exhaustion fall +back to deterministic behavior. A quota-reset exhaustion is different: it is +propagated so the orchestration run can stop without filing or publishing a +success audit record. Configure the classifier with +`--semantic-followup-backend`, `--semantic-followup-model`, +`--semantic-followup-timeout-seconds`, `--semantic-followup-max-calls`, +`--semantic-followup-max-candidates`, and +`--semantic-followup-prompt-char-limit`; use +`--no-semantic-followup-dedupe` for deterministic-only operation. + +Publication summaries distinguish created issues, reused trackers, uncertain +matches, and cap-skipped work. The three-new-issue cap is applied after all +groups have been checked for reuse, so reusing an existing tracker does not +consume a creation slot. Candidate titles, excerpts, retained reviewer/planning +context, and model reasons are sanitized before entering a GitHub body or +comment, while the expected publish-once audit record remains the only +authorized protocol record in those bodies. + For plan-first runs that continue into implementation, issue-filing modes apply twice at different lifecycle points: planning-stage future follow-ups are filed before implementation begins, while PR-stage approved-review future follow-ups diff --git a/helpers/skill_runner.py b/helpers/skill_runner.py index d355e8e..fe644a2 100644 --- a/helpers/skill_runner.py +++ b/helpers/skill_runner.py @@ -92,6 +92,7 @@ import argparse import dataclasses import hashlib +import inspect import json import os import re @@ -114,6 +115,7 @@ ) from coding_review_agent_loop.errors import AgentLoopError, UnknownPriorItemDispositionError from coding_review_agent_loop.followups import ( + FollowupSourceContext, _approved_followup_from_unresolved_item, _publish_approved_followups, ) @@ -908,14 +910,30 @@ def _publish_pr_followups( # (the coder dir). The Codex+Gemini skill flow never creates a Claude checkout, # so ensure that directory exists or gh raises FileNotFoundError (#300). Path(active_workdir(config)).mkdir(parents=True, exist_ok=True) - published = _publish_approved_followups( - Runner(dry_run=False), - config=config, - pr_number=pr, - head_sha=head_sha, - pr_comments=pr_comments, - followups=approved, - ) + publish_kwargs = { + "config": config, + "pr_number": pr, + "head_sha": head_sha, + "pr_comments": pr_comments, + "followups": approved, + "source_context": FollowupSourceContext( + repo=repo, + source_kind="pr", + source_number=pr, + source_identity=head_sha, + related_pr_numbers=(pr,), + ), + } + # Keep old test/integration adapters that monkeypatch the publisher with + # the pre-context signature callable; the production call always carries + # explicit source context. + signature = inspect.signature(_publish_approved_followups) + if "source_context" not in signature.parameters and not any( + parameter.kind == inspect.Parameter.VAR_KEYWORD + for parameter in signature.parameters.values() + ): + publish_kwargs.pop("source_context") + published = _publish_approved_followups(Runner(dry_run=False), **publish_kwargs) return {"mode": mode, "published": bool(published), "count": len(approved)} diff --git a/src/coding_review_agent_loop/cli.py b/src/coding_review_agent_loop/cli.py index 6f54e2d..313b2e8 100644 --- a/src/coding_review_agent_loop/cli.py +++ b/src/coding_review_agent_loop/cli.py @@ -19,6 +19,11 @@ DEFAULT_REPAIR_MODELS, DEFAULT_FLAT_CHILD_LIMIT, DEFAULT_ANTIGRAVITY_QUOTA_SIGNATURES, + DEFAULT_SEMANTIC_FOLLOWUP_BACKEND, + DEFAULT_SEMANTIC_FOLLOWUP_MAX_CALLS, + DEFAULT_SEMANTIC_FOLLOWUP_MAX_CANDIDATES, + DEFAULT_SEMANTIC_FOLLOWUP_PROMPT_CHAR_LIMIT, + DEFAULT_SEMANTIC_FOLLOWUP_TIMEOUT_SECONDS, AgentLoopConfig, config_from_args, ensure_agent_workdirs, @@ -562,6 +567,56 @@ def add_common(subparser: argparse.ArgumentParser) -> None: "'fix-and-issue'; default: ignore)." ), ) + semantic_group = subparser.add_mutually_exclusive_group() + semantic_group.add_argument( + "--semantic-followup-dedupe", + "--followup-semantic-dedupe", + dest="semantic_followup_dedupe", + action="store_true", + default=True, + help="Use bounded semantic matching when deterministic follow-up dedupe is ambiguous (default).", + ) + semantic_group.add_argument( + "--no-semantic-followup-dedupe", + dest="semantic_followup_dedupe", + action="store_false", + help="Disable semantic follow-up matching; retain deterministic and conservative fallback behavior.", + ) + subparser.add_argument( + "--semantic-followup-backend", + choices=("claude", "codex", "gemini", "antigravity"), + default=DEFAULT_SEMANTIC_FOLLOWUP_BACKEND, + help="Cheap isolated provider used for semantic follow-up matching.", + ) + subparser.add_argument( + "--semantic-followup-model", + default="", + help="Optional model override for the semantic follow-up provider.", + ) + subparser.add_argument( + "--semantic-followup-timeout-seconds", + type=int, + default=DEFAULT_SEMANTIC_FOLLOWUP_TIMEOUT_SECONDS, + help="Per-call semantic follow-up provider timeout (default: 30).", + ) + subparser.add_argument( + "--semantic-followup-max-calls", + type=int, + default=DEFAULT_SEMANTIC_FOLLOWUP_MAX_CALLS, + help="Maximum semantic provider calls per publication (default: 5).", + ) + subparser.add_argument( + "--semantic-followup-max-candidates", + type=int, + default=DEFAULT_SEMANTIC_FOLLOWUP_MAX_CANDIDATES, + help="Maximum existing/batch candidates presented to one semantic call (default: 50).", + ) + subparser.add_argument( + "--semantic-followup-prompt-char-limit", + type=int, + default=DEFAULT_SEMANTIC_FOLLOWUP_PROMPT_CHAR_LIMIT, + help="Maximum prompt size for one semantic follow-up call (default: 12000).", + ) subparser.add_argument( "--planning-context-mode", choices=("full", "compact"), diff --git a/src/coding_review_agent_loop/config.py b/src/coding_review_agent_loop/config.py index f4a452f..11998ac 100644 --- a/src/coding_review_agent_loop/config.py +++ b/src/coding_review_agent_loop/config.py @@ -54,6 +54,11 @@ DEFAULT_ANTIGRAVITY_PRINT_TIMEOUT_SECONDS = 10 * 60 DEFAULT_REPAIR_MODELS: tuple[str, ...] = ("Gemini 3.7 Flash (Medium)",) DEFAULT_REASONING_EFFORT = "medium" +DEFAULT_SEMANTIC_FOLLOWUP_BACKEND: AgentName = "gemini" +DEFAULT_SEMANTIC_FOLLOWUP_TIMEOUT_SECONDS = 30 +DEFAULT_SEMANTIC_FOLLOWUP_MAX_CALLS = 5 +DEFAULT_SEMANTIC_FOLLOWUP_MAX_CANDIDATES = 50 +DEFAULT_SEMANTIC_FOLLOWUP_PROMPT_CHAR_LIMIT = 12_000 CODEX_REASONING_EFFORTS: frozenset[str] = frozenset( {"minimal", "low", "medium", "high", "xhigh"} ) @@ -119,6 +124,15 @@ class AgentLoopConfig: agent_memory_dir: Path refresh_test_profile: bool approved_followups: str = "ignore" + # Approved-follow-up semantic reuse is deliberately bounded and can be + # disabled for offline/reproducibility-sensitive invocations. + semantic_followup_dedupe: bool = True + semantic_followup_backend: AgentName = DEFAULT_SEMANTIC_FOLLOWUP_BACKEND + semantic_followup_model: str = "" + semantic_followup_timeout_seconds: int = DEFAULT_SEMANTIC_FOLLOWUP_TIMEOUT_SECONDS + semantic_followup_max_calls: int = DEFAULT_SEMANTIC_FOLLOWUP_MAX_CALLS + semantic_followup_max_candidates: int = DEFAULT_SEMANTIC_FOLLOWUP_MAX_CANDIDATES + semantic_followup_prompt_char_limit: int = DEFAULT_SEMANTIC_FOLLOWUP_PROMPT_CHAR_LIMIT plan_execution_mode: str = "plan-only" planning_context_mode: str = "compact" pr_review_context_mode: str = "full" @@ -317,6 +331,16 @@ def __post_init__(self) -> None: raise AgentLoopError("antigravity_models chain cannot be empty or contain blank entries.") if self.antigravity_print_timeout_seconds <= 0: raise AgentLoopError("--antigravity-print-timeout-seconds must be greater than zero.") + if self.semantic_followup_backend not in {"claude", "codex", "gemini", "antigravity"}: + raise AgentLoopError("--semantic-followup-backend must name a supported agent.") + for option_name, value in ( + ("--semantic-followup-timeout-seconds", self.semantic_followup_timeout_seconds), + ("--semantic-followup-max-calls", self.semantic_followup_max_calls), + ("--semantic-followup-max-candidates", self.semantic_followup_max_candidates), + ("--semantic-followup-prompt-char-limit", self.semantic_followup_prompt_char_limit), + ): + if isinstance(value, bool) or not isinstance(value, int) or value <= 0: + raise AgentLoopError(f"{option_name} must be a positive integer.") timeout = self.coder_test_command_timeout_seconds if isinstance(timeout, bool) or not isinstance(timeout, (int, float)): raise AgentLoopError( @@ -1293,6 +1317,23 @@ def config_from_args( ), refresh_test_profile=args.refresh_test_profile, approved_followups=args.approved_followups, + semantic_followup_dedupe=getattr(args, "semantic_followup_dedupe", True), + semantic_followup_backend=getattr( + args, "semantic_followup_backend", DEFAULT_SEMANTIC_FOLLOWUP_BACKEND + ), + semantic_followup_model=getattr(args, "semantic_followup_model", ""), + semantic_followup_timeout_seconds=getattr( + args, "semantic_followup_timeout_seconds", DEFAULT_SEMANTIC_FOLLOWUP_TIMEOUT_SECONDS + ), + semantic_followup_max_calls=getattr( + args, "semantic_followup_max_calls", DEFAULT_SEMANTIC_FOLLOWUP_MAX_CALLS + ), + semantic_followup_max_candidates=getattr( + args, "semantic_followup_max_candidates", DEFAULT_SEMANTIC_FOLLOWUP_MAX_CANDIDATES + ), + semantic_followup_prompt_char_limit=getattr( + args, "semantic_followup_prompt_char_limit", DEFAULT_SEMANTIC_FOLLOWUP_PROMPT_CHAR_LIMIT + ), plan_execution_mode=getattr(args, "plan_execution_mode", None) or "plan-only", planning_context_mode=getattr(args, "planning_context_mode", None) or "compact", pr_review_context_mode=getattr(args, "pr_review_context_mode", None) or "full", diff --git a/src/coding_review_agent_loop/followups.py b/src/coding_review_agent_loop/followups.py index 98bb190..2c42331 100644 --- a/src/coding_review_agent_loop/followups.py +++ b/src/coding_review_agent_loop/followups.py @@ -6,13 +6,29 @@ from collections import Counter from collections.abc import Sequence from dataclasses import dataclass +from typing import Callable, Literal from .config import AgentLoopConfig -from .github import create_issue, post_issue_comment, post_pr_comment +from .errors import AgentLoopError, QuotaResetExceededError +from .github import ( + FoundIssue, + create_issue, + post_issue_comment, + post_pr_comment, + search_issues, + validate_open_issue, +) from .logging import log from .protocol import ApprovedFollowup, UnresolvedReviewItem from .runner import Runner from .protocol_markers import TrustedBody, sanitize_historical_text +from .semantic_dedupe import ( + BudgetExhausted, + SemanticCandidate, + SemanticDedupeMatcher, + SemanticMatch, + SemanticTransport, +) MAX_APPROVED_FOLLOWUP_ISSUES = 3 APPROVED_FOLLOWUP_MARKER_RE = re.compile( @@ -40,6 +56,46 @@ def reviewers(self) -> tuple[str, ...]: return tuple(reviewers) +@dataclass(frozen=True) +class FollowupSourceContext: + """Repository-scoped provenance used to narrow historical trackers.""" + + repo: str + source_kind: Literal["pr", "plan"] + source_number: int + source_identity: str | None = None + parent_issue_numbers: tuple[int, ...] = () + related_issue_numbers: tuple[int, ...] = () + related_pr_numbers: tuple[int, ...] = () + + def __post_init__(self) -> None: + if not self.repo or "/" not in self.repo: + raise AgentLoopError("follow-up source context requires an owner/name repository") + if self.source_kind not in {"pr", "plan"}: + raise AgentLoopError("follow-up source context kind must be pr or plan") + if isinstance(self.source_number, bool) or not isinstance(self.source_number, int) or self.source_number <= 0: + raise AgentLoopError("follow-up source number must be positive") + for field_name in ("parent_issue_numbers", "related_issue_numbers", "related_pr_numbers"): + values = getattr(self, field_name) + if any(isinstance(value, bool) or not isinstance(value, int) or value <= 0 for value in values): + raise AgentLoopError(f"{field_name} must contain positive integer identities") + + def render(self) -> str: + parts = [ + f"repository={sanitize_historical_text(self.repo)}", + f"source={self.source_kind}#{self.source_number}", + ] + if self.source_identity: + parts.append(f"identity={sanitize_historical_text(self.source_identity)}") + if self.parent_issue_numbers: + parts.append("parent issue(s)=" + ", ".join(f"#{n}" for n in self.parent_issue_numbers)) + if self.related_issue_numbers: + parts.append("related issue(s)=" + ", ".join(f"#{n}" for n in self.related_issue_numbers)) + if self.related_pr_numbers: + parts.append("related PR(s)=" + ", ".join(f"#{n}" for n in self.related_pr_numbers)) + return "; ".join(parts) + + @dataclass(frozen=True) class ApprovedFollowupReconciliation: groups: tuple[GroupedApprovedFollowup, ...] @@ -80,6 +136,15 @@ class PlanApprovedFollowupReconciliation: deduplicated_count: int +@dataclass(frozen=True) +class FollowupPublication: + group: GroupedApprovedFollowup | PlanGroupedApprovedFollowup + status: Literal["created", "reused", "uncertain", "cap-skipped"] + issue_number: int | None = None + issue_url: str | None = None + reason: str | None = None + + _FOLLOWUP_STOPWORDS = { "a", "about", @@ -284,6 +349,9 @@ def reconcile_approved_followups( followups: Sequence[ApprovedFollowup], *, issue_limit: int = MAX_APPROVED_FOLLOWUP_ISSUES, + semantic_matcher: Callable[ + [ApprovedFollowup, tuple[GroupedApprovedFollowup, ...]], SemanticMatch | None + ] | None = None, ) -> ApprovedFollowupReconciliation: grouped: list[GroupedApprovedFollowup] = [] indexes: dict[str, int] = {} @@ -295,6 +363,25 @@ def reconcile_approved_followups( if any(_followup_similarity(followup, item) >= 0.55 for item in group.items): existing_index = index break + if existing_index is None and semantic_matcher is not None and grouped: + # Deterministic keys and similarity are always the first pass. The + # injected matcher sees only the already-narrowed batch groups and + # may merge only an explicitly high-confidence, non-null match. + semantic = semantic_matcher(followup, tuple(grouped)) + if semantic is not None and semantic.confidence == "high": + target = semantic.duplicate_of + if isinstance(target, str) and target.startswith("group-"): + suffix = target.removeprefix("group-") + if suffix.isdigit(): + candidate_index = int(suffix) - 1 + if 0 <= candidate_index < len(grouped): + existing_index = candidate_index + elif isinstance(target, int) and not isinstance(target, bool): + # Test/in-process matchers may use the natural zero-based + # group identity. Existing issue numbers are never valid + # here because batch groups carry string identities. + if 0 <= target < len(grouped): + existing_index = target if existing_index is None: indexes.update((key, len(grouped)) for key in keys) grouped.append(GroupedApprovedFollowup(text=followup.text, items=(followup,))) @@ -319,6 +406,9 @@ def reconcile_plan_approved_followups( sources: Sequence[PlanApprovedFollowupSource], *, issue_limit: int = MAX_APPROVED_FOLLOWUP_ISSUES, + semantic_matcher: Callable[ + [ApprovedFollowup, tuple[GroupedApprovedFollowup, ...]], SemanticMatch | None + ] | None = None, ) -> PlanApprovedFollowupReconciliation: source_by_projection_id: dict[int, PlanApprovedFollowupSource] = {} projections: list[ApprovedFollowup] = [] @@ -327,7 +417,11 @@ def reconcile_plan_approved_followups( projections.append(projection) source_by_projection_id[id(projection)] = source - reconciliation = reconcile_approved_followups(projections, issue_limit=issue_limit) + reconciliation = reconcile_approved_followups( + projections, + issue_limit=issue_limit, + semantic_matcher=semantic_matcher, + ) def plan_group(group: GroupedApprovedFollowup) -> PlanGroupedApprovedFollowup: return PlanGroupedApprovedFollowup( @@ -501,11 +595,283 @@ def _followup_heading_key(text: str) -> str | None: return None +def _issue_url(repo: str, issue_number: int) -> str: + return f"https://github.com/{repo}/issues/{issue_number}" + + +def _validated_issue_number(found: FoundIssue, *, repo: str) -> int | None: + number = found.number + if isinstance(number, bool) or not isinstance(number, int) or number <= 0: + return None + url = found.url or "" + if url: + match = re.fullmatch( + r"https?://github\.com/(?P[^/\s]+/[^/\s]+)/issues/(?P[1-9]\d*)/?", + url, + re.I, + ) + if match is None or match.group("repo").casefold() != repo.casefold(): + return None + if int(match.group("number")) != number: + return None + return number + + +def _looks_like_followup_tracker(found: FoundIssue) -> bool: + text = f"{found.title or ''}\n{found.body or ''}".casefold() + return ( + "future follow-up" in text + or "follow up future" in text + or "approved planning" in text + or "approved review" in text + ) + + +def _candidate_context_matches(found: FoundIssue, source_context: FollowupSourceContext) -> bool: + text = f"{found.title or ''}\n{found.body or ''}".casefold() + identity_values = ( + *source_context.parent_issue_numbers, + *source_context.related_issue_numbers, + *source_context.related_pr_numbers, + source_context.source_number if source_context.source_kind == "pr" else 0, + ) + if any(f"#{number}" in text for number in identity_values if number): + return True + if source_context.source_identity and source_context.source_identity.casefold() in text: + return True + return False + + +def _narrow_existing_candidates( + proposed: ApprovedFollowup, + candidates: Sequence[FoundIssue], + *, + source_context: FollowupSourceContext, + max_candidates: int, +) -> tuple[FoundIssue, ...]: + proposed_ids = _followup_identifier_keys(proposed.text) + proposed_terms = _followup_topic_terms(proposed.text) + scored: list[tuple[int, FoundIssue]] = [] + for candidate in candidates: + if not _looks_like_followup_tracker(candidate): + continue + candidate_text = f"{candidate.title or ''}\n{candidate.body or ''}" + candidate_ids = _followup_identifier_keys(candidate_text) + candidate_terms = _followup_topic_terms(candidate_text) + score = 0 + if _candidate_context_matches(candidate, source_context): + score += 8 + if proposed_ids & candidate_ids: + score += 5 + score += min(4, len(proposed_terms & candidate_terms)) + if score: + scored.append((score, candidate)) + scored.sort(key=lambda pair: (-pair[0], pair[1].number or 0)) + return tuple(candidate for _score, candidate in scored[:max_candidates]) + + +def _exact_existing_match( + proposed: ApprovedFollowup, + candidate: FoundIssue, +) -> bool: + proposed_key = _normalize_followup_key(proposed.text) + if not proposed_key: + return False + candidate_text = f"{candidate.title or ''}\n{candidate.body or ''}" + candidate_key = _normalize_followup_key(candidate_text) + if proposed_key in candidate_key or candidate_key in proposed_key: + return True + proposed_ids = _followup_identifier_keys(proposed.text) + candidate_ids = _followup_identifier_keys(candidate_text) + proposed_terms = _followup_topic_terms(proposed.text) + candidate_terms = _followup_topic_terms(candidate_text) + return bool(proposed_ids and proposed_ids <= candidate_ids and proposed_terms and proposed_terms <= candidate_terms) + + +def _search_followup_trackers( + runner: Runner, + *, + config: AgentLoopConfig, + source_context: FollowupSourceContext, + followups: Sequence[ApprovedFollowup], +) -> tuple[FoundIssue, ...]: + queries: list[str] = [] + prefix = f"repo:{source_context.repo} is:issue is:open" + + def add_query(value: str) -> None: + value = value.strip() + if value and value not in queries and len(queries) < 5: + queries.append(value) + + for number in source_context.parent_issue_numbers: + add_query(f'{prefix} "#{number}" "follow-up"') + if source_context.source_identity: + add_query(f'{prefix} "{sanitize_historical_text(source_context.source_identity)}" "follow-up"') + for number in source_context.related_pr_numbers: + add_query(f'{prefix} "PR #{number}" "follow-up"') + for number in source_context.related_issue_numbers: + add_query(f'{prefix} "#{number}" "follow-up"') + terms: list[str] = [] + for followup in followups: + for term in sorted(_followup_topic_terms(followup.text)): + if term not in terms: + terms.append(term) + if terms: + add_query(f'{prefix} "future follow-up" "{" ".join(terms[:4])}"') + if not queries: + add_query(f'{prefix} "future follow-up"') + + found: dict[int, FoundIssue] = {} + excluded_numbers = { + source_context.source_number + if source_context.source_kind == "plan" + else -1 + } + for query in queries: + try: + results = search_issues( + runner, + config=config, + search=query, + state="open", + limit=20, + ) + except QuotaResetExceededError: + raise + except Exception as exc: + log(config, f"Approved follow-up tracker search unavailable ({exc}); using conservative creation fallback") + continue + if len(results) >= 20: + log(config, f"Approved follow-up tracker search reached limit for {query!r}; results may be truncated") + for result in results: + number = _validated_issue_number(result, repo=source_context.repo) + if number is not None and number not in excluded_numbers: + found.setdefault(number, result) + if len(found) >= config.semantic_followup_max_candidates: + log(config, "Approved follow-up tracker candidate budget reached; truncating aggregate candidates") + break + return tuple(found.values())[: config.semantic_followup_max_candidates] + + +def _semantic_batch_matcher( + matcher: SemanticDedupeMatcher, + *, + source_context: FollowupSourceContext, +) -> Callable[[ApprovedFollowup, tuple[GroupedApprovedFollowup, ...]], SemanticMatch | None]: + def match( + proposed: ApprovedFollowup, + groups: tuple[GroupedApprovedFollowup, ...], + ) -> SemanticMatch | None: + # Shared source context is included in the prompt, but the matcher is + # still bounded to the configured group count and cannot create an + # identity outside this batch. + candidates = tuple( + SemanticCandidate( + identity=f"group-{index}", + title=f"Batch follow-up group {index}", + body=group.text, + ) + for index, group in enumerate(groups, start=1) + ) + try: + return matcher.match( + proposed=proposed.text, + candidates=candidates, + source_context=source_context.render(), + ) + except QuotaResetExceededError: + raise + except BudgetExhausted as exc: + log(matcher.config, f"Approved follow-up semantic batch budget exhausted ({exc}); retaining deterministic groups") + except Exception as exc: + log(matcher.config, f"Approved follow-up semantic batch matcher unavailable ({exc}); retaining deterministic groups") + return None + + return match + + +def _try_revalidate_open_issue( + runner: Runner, + *, + config: AgentLoopConfig, + issue_number: int, + cache: set[int], +) -> bool: + if issue_number in cache: + return True + try: + validate_open_issue(runner, config=config, issue_number=issue_number) + except QuotaResetExceededError: + raise + except Exception as exc: + log(config, f"Follow-up candidate #{issue_number} failed open-state revalidation ({exc}); filing remains enabled") + return False + cache.add(issue_number) + return True + + +def _format_publication_summary( + *, + heading: str, + publications: Sequence[FollowupPublication], + deduplicated_count: int, + skipped_by_cap: int, +) -> str: + lines = [heading, ""] + shown_targets: set[str] = set() + for publication in publications: + label = _safe_followup_main_text(publication.group.text) + if publication.status == "created": + target = publication.issue_url or "Created issue URL unavailable from GitHub CLI output." + if target not in shown_targets: + lines.append(f"- {target}") + shown_targets.add(target) + lines.append(f" - Created: {label}") + elif publication.status == "reused": + target = publication.issue_url or "existing issue URL unavailable" + reviewers = ", ".join(sanitize_historical_text(reviewer) for reviewer in publication.group.reviewers) + lines.append(f"- Reused existing follow-up issue: {target} — {label} ({reviewers})") + if publication.reason: + lines.append(f" - {sanitize_historical_text(publication.reason)}") + elif publication.status == "uncertain": + lines.append(f"- Possible duplicate not suppressed; filed: {publication.issue_url or 'URL unavailable'} — {label}") + if publication.reason: + lines.append(f" - Possible duplicate note: {sanitize_historical_text(publication.reason)}") + else: + lines.append(f"- Skipped by new-issue cap: {label}") + lines.extend( + [ + "", + f"Reconciliation: {sum(p.status in {'created', 'uncertain'} for p in publications)} filed, " + f"{deduplicated_count} deduplicated, {skipped_by_cap} skipped by cap.", + f"Tracker reuse: {sum(p.status == 'reused' for p in publications)} reused; " + f"{sum(p.status == 'uncertain' for p in publications)} uncertain.", + "", + "These were mentioned as future work and did not block merge readiness.", + "", + "-- coding-review-agent-loop", + ] + ) + if skipped_by_cap: + lines[-2:-2] = [ + "", + f"Skipped {skipped_by_cap} additional item(s) to avoid issue noise; reviewers should reserve " + "this section for substantial independent follow-up work.", + ] + return "\n".join(lines) + + def _dedupe_approved_followups(followups: Sequence[ApprovedFollowup]) -> list[GroupedApprovedFollowup]: return list(reconcile_approved_followups(followups, issue_limit=len(followups) or 0).groups) -def _followup_issue_body(pr_number: int, followup: GroupedApprovedFollowup) -> str: +def _followup_issue_body( + pr_number: int, + followup: GroupedApprovedFollowup, + *, + source_context: FollowupSourceContext | None = None, + possible_duplicate: str | None = None, +) -> str: lines = [ f"Future follow-up from approved review on PR #{pr_number}.", "", @@ -523,6 +889,14 @@ def _followup_issue_body(pr_number: int, followup: GroupedApprovedFollowup) -> s f"- {_safe_followup_main_text(followup.text)}", ] ) + if possible_duplicate: + lines.extend( + [ + "", + "Possible duplicate (not suppressed because semantic confidence was not high):", + f"- {sanitize_historical_text(possible_duplicate)}", + ] + ) lines.extend(["", "Original reviewer notes:"]) lines.extend( f"- {sanitize_historical_text(item.reviewer)}: {sanitize_historical_text(item.text)}" @@ -559,6 +933,8 @@ def _plan_followup_issue_body( plan_hash: str, plan_subject: str, followup: PlanGroupedApprovedFollowup, + source_context: FollowupSourceContext | None = None, + possible_duplicate: str | None = None, ) -> str: reviewers = tuple(sanitize_historical_text(reviewer) for reviewer in followup.reviewers) rounds = sorted({source.source_round for source in followup.sources if source.source_round is not None}) @@ -583,6 +959,8 @@ def _plan_followup_issue_body( lines.append("- Reviewers: " + ", ".join(reviewers)) if item_ids: lines.append("- Original plan item ID(s): " + ", ".join(item_ids)) + if source_context is not None: + lines.append(f"- Lookup context: {source_context.render()}") lines.extend( [ "", @@ -598,6 +976,14 @@ def _plan_followup_issue_body( ) for note in source.notes: lines.append(f" - Update from {sanitize_historical_text(note)}") + if possible_duplicate: + lines.extend( + [ + "", + "Possible duplicate (not suppressed because semantic confidence was not high):", + f"- {sanitize_historical_text(possible_duplicate)}", + ] + ) lines.extend( [ "", @@ -634,6 +1020,276 @@ def _create_plan_approved_followup_issues( return issue_urls +def _validated_created_issue_url(url: str | None, *, repo: str) -> tuple[int | None, str | None]: + if not url: + return None, None + match = re.fullmatch( + r"https?://github\.com/(?P[^/\s]+/[^/\s]+)/issues/(?P[1-9]\d*)/?", + url.strip(), + re.I, + ) + if match is None or match.group("repo").casefold() != repo.casefold(): + return None, None + number = int(match.group("number")) + return number, _issue_url(repo, number) + + +def _find_existing_for_group( + runner: Runner, + *, + config: AgentLoopConfig, + followup: ApprovedFollowup, + candidates: Sequence[FoundIssue], + source_context: FollowupSourceContext, + matcher: SemanticDedupeMatcher | None, + revalidated: set[int], +) -> tuple[str, int | None, str | None]: + """Return (status, issue number, reason) for a safe existing match.""" + valid_candidates = [ + candidate + for candidate in candidates + if _validated_issue_number(candidate, repo=source_context.repo) is not None + ] + for candidate in valid_candidates: + number = _validated_issue_number(candidate, repo=source_context.repo) + assert number is not None + if _exact_existing_match(followup, candidate) and _try_revalidate_open_issue( + runner, config=config, issue_number=number, cache=revalidated + ): + log(config, f"Suppressed approved follow-up as deterministic duplicate of existing issue #{number}") + return "reused", number, "Deterministic equivalence with this existing tracker." + + if matcher is None: + return "new", None, None + narrowed = _narrow_existing_candidates( + followup, + valid_candidates, + source_context=source_context, + max_candidates=config.semantic_followup_max_candidates, + ) + if not narrowed: + return "new", None, None + semantic_candidates = tuple( + SemanticCandidate(identity=_validated_issue_number(candidate, repo=source_context.repo), + title=candidate.title or "", + body=candidate.body or "") + for candidate in narrowed + ) + semantic_candidates = tuple(candidate for candidate in semantic_candidates if candidate.identity is not None) + try: + match = matcher.match( + proposed=followup.text, + candidates=semantic_candidates, + source_context=source_context.render(), + ) + except QuotaResetExceededError: + raise + except BudgetExhausted as exc: + log(config, f"Approved follow-up semantic candidate budget exhausted ({exc}); using deterministic fallback") + return "new", None, None + except Exception as exc: + log(config, f"Approved follow-up semantic matcher unavailable ({exc}); using deterministic fallback") + return "new", None, None + if match.duplicate_of is None: + return "new", None, None + if not isinstance(match.duplicate_of, int) or isinstance(match.duplicate_of, bool): + log(config, "Approved follow-up semantic matcher returned a non-issue identity; using fallback") + return "new", None, None + candidate = next( + (candidate for candidate in narrowed if candidate.number == match.duplicate_of), + None, + ) + if candidate is None: + return "new", None, None + if match.confidence == "high" and _try_revalidate_open_issue( + runner, config=config, issue_number=match.duplicate_of, cache=revalidated + ): + log( + config, + f"Suppressed approved follow-up as high-confidence semantic duplicate of existing issue #{match.duplicate_of}: " + f"{sanitize_historical_text(match.reason)}", + ) + return "reused", match.duplicate_of, match.reason + if match.confidence in {"medium", "low"}: + return "uncertain", None, f"Candidate issue #{match.duplicate_of}: {match.reason}" + return "new", None, None + + +def _publish_issue_followup_groups( + runner: Runner, + *, + config: AgentLoopConfig, + groups: Sequence[GroupedApprovedFollowup], + source_context: FollowupSourceContext, + heading: str, + deduplicated_count: int, + skipped_by_cap: int, + plan_subject: str | None = None, + issue_number: int | None = None, + plan_hash: str | None = None, + semantic_matcher: SemanticDedupeMatcher | None = None, + semantic_transport: SemanticTransport | None = None, +) -> tuple[str, tuple[str, ...]]: + followups = [ApprovedFollowup(reviewer=group.reviewers[0], text=group.text) for group in groups] + candidates = _search_followup_trackers( + runner, + config=config, + source_context=source_context, + followups=followups, + ) + matcher = semantic_matcher or ( + SemanticDedupeMatcher(runner=runner, config=config, transport=semantic_transport) + if config.semantic_followup_dedupe and not config.dry_run + else None + ) + revalidated: set[int] = set() + publications: list[FollowupPublication] = [] + created_count = 0 + for group in groups: + proposed = ApprovedFollowup(reviewer=group.reviewers[0], text=group.text) + status, existing_number, reason = _find_existing_for_group( + runner, + config=config, + followup=proposed, + candidates=candidates, + source_context=source_context, + matcher=matcher, + revalidated=revalidated, + ) + if status == "reused": + assert existing_number is not None + publications.append( + FollowupPublication( + group=group, + status="reused", + issue_number=existing_number, + issue_url=_issue_url(source_context.repo, existing_number), + reason=reason, + ) + ) + continue + if created_count >= MAX_APPROVED_FOLLOWUP_ISSUES: + publications.append(FollowupPublication(group=group, status="cap-skipped", reason="Only new issues consume the cap.")) + skipped_by_cap += 1 + continue + body_reason = reason if status == "uncertain" else None + try: + if isinstance(group, PlanGroupedApprovedFollowup): + assert issue_number is not None and plan_hash is not None and plan_subject is not None + raw_url = create_issue( + runner, + config=config, + title=_plan_followup_issue_title(group), + body=_plan_followup_issue_body( + issue_number=issue_number, + plan_hash=plan_hash, + plan_subject=plan_subject, + followup=group, + source_context=source_context, + possible_duplicate=body_reason, + ), + ) + else: + raw_url = create_issue( + runner, + config=config, + title=_followup_issue_title(proposed), + body=_followup_issue_body( + source_context.source_number, + group, + source_context=source_context, + possible_duplicate=body_reason, + ), + ) + except QuotaResetExceededError: + raise + except Exception: + # A create-then-interruption window is recoverable when GitHub has + # indexed the tracker. Rediscover that exact group before allowing + # the orchestration failure to escape. + recovered = _search_followup_trackers( + runner, + config=config, + source_context=source_context, + followups=(proposed,), + ) + recovered_match = next( + ( + candidate + for candidate in recovered + if _exact_existing_match(proposed, candidate) + and _validated_issue_number(candidate, repo=source_context.repo) is not None + ), + None, + ) + if recovered_match is not None: + recovered_number = _validated_issue_number(recovered_match, repo=source_context.repo) + assert recovered_number is not None + if _try_revalidate_open_issue( + runner, config=config, issue_number=recovered_number, cache=revalidated + ): + publications.append( + FollowupPublication( + group=group, + status="reused", + issue_number=recovered_number, + issue_url=_issue_url(source_context.repo, recovered_number), + reason="Recovered an issue created before the publication interruption.", + ) + ) + continue + raise + created_count += 1 + created_number, created_url = _validated_created_issue_url(raw_url, repo=source_context.repo) + if created_number is not None: + # Keep newly-created identities in the invocation cache. This + # closes the create-then-next-group window without relying on + # GitHub search indexing to become immediately consistent. + candidates = ( + *candidates, + FoundIssue( + number=created_number, + title=_plan_followup_issue_title(group) + if isinstance(group, PlanGroupedApprovedFollowup) + else _followup_issue_title(proposed), + url=created_url, + body=( + _plan_followup_issue_body( + issue_number=issue_number or source_context.source_number, + plan_hash=plan_hash or "unknown", + plan_subject=plan_subject or "unknown", + followup=group, + ) + if isinstance(group, PlanGroupedApprovedFollowup) + else _followup_issue_body( + source_context.source_number, + group, + ) + ), + ), + ) + publications.append( + FollowupPublication( + group=group, + status="uncertain" if status == "uncertain" else "created", + issue_number=created_number, + issue_url=created_url, + reason=reason, + ) + ) + body = _format_publication_summary( + heading=heading, + publications=publications, + deduplicated_count=deduplicated_count, + skipped_by_cap=skipped_by_cap, + ) + return body, tuple( + publication.issue_url + for publication in publications + if publication.issue_url + ) + + def _create_approved_followup_issues( runner: Runner, *, @@ -702,14 +1358,58 @@ def _publish_approved_followups( head_sha: str | None, pr_comments: Sequence[object], followups: list[ApprovedFollowup], + source_context: FollowupSourceContext, + usage_context: object | None = None, + semantic_transport: SemanticTransport | None = None, ) -> bool: if not followups or config.approved_followups == "ignore": return False + mode = ( + "summarize" + if config.approved_followups in ("summarize", "fix-and-summarize") + else "issue" + ) + # Replay detection is deliberately before reconciliation, lookup, and + # provider activity. A publish-once marker is the invocation's durable + # audit record even when every follow-up was reused. + if _has_approved_followups_marker( + pr_comments, + pr_number=pr_number, + head_sha=head_sha, + mode=mode, + ): + log( + config, + f"Approved-review future follow-ups already recorded for PR #{pr_number} at {head_sha or 'unknown'} ({mode})", + ) + return False + + semantic_matcher = None + semantic_provider_matcher: SemanticDedupeMatcher | None = None + if ( + mode == "issue" + and config.semantic_followup_dedupe + and not config.dry_run + ): + matcher = SemanticDedupeMatcher( + runner=runner, + config=config, + usage_context=usage_context, + transport=semantic_transport, + ) + semantic_provider_matcher = matcher + semantic_matcher = _semantic_batch_matcher(matcher, source_context=source_context) reconciliation = reconcile_approved_followups( followups, - issue_limit=MAX_APPROVED_FOLLOWUP_ISSUES, + issue_limit=( + MAX_APPROVED_FOLLOWUP_ISSUES + if mode == "summarize" + else len(followups) or MAX_APPROVED_FOLLOWUP_ISSUES + ), + semantic_matcher=semantic_matcher, ) - if not reconciliation.selected_groups: + groups = reconciliation.selected_groups if mode == "summarize" else reconciliation.groups + if not groups: return False log( config, @@ -719,19 +1419,7 @@ def _publish_approved_followups( f"{reconciliation.skipped_by_cap} skipped by cap", ) - if config.approved_followups in ("summarize", "fix-and-summarize"): - mode = "summarize" - if _has_approved_followups_marker( - pr_comments, - pr_number=pr_number, - head_sha=head_sha, - mode=mode, - ): - log( - config, - f"Approved-review future follow-ups already recorded for PR #{pr_number} at {head_sha or 'unknown'} ({mode})", - ) - return False + if mode == "summarize": body = _format_approved_followup_summary(pr_number, reconciliation) body = _append_approved_followups_marker( body, @@ -747,40 +1435,37 @@ def _publish_approved_followups( ) return True - if config.approved_followups in ("issue", "fix-and-issue"): - mode = "issue" - if _has_approved_followups_marker( - pr_comments, + if mode == "issue": + publication_body, _issue_urls = _publish_issue_followup_groups( + runner, + config=config, + groups=groups, + source_context=source_context, + heading=f"Created approved-review future follow-up issues for PR #{pr_number}:", + deduplicated_count=reconciliation.deduplicated_count, + skipped_by_cap=reconciliation.skipped_by_cap, + semantic_matcher=semantic_provider_matcher, + semantic_transport=semantic_transport, + ) + if not _issue_urls: + # Preserve the historical fail-safe when GitHub did not return a + # usable identity for any newly-created issue. Reused trackers + # are included in _issue_urls and therefore still publish the + # durable audit record. + return False + body = _append_approved_followups_marker( + publication_body, pr_number=pr_number, head_sha=head_sha, mode=mode, - ): - log( - config, - f"Approved-review future follow-ups already recorded for PR #{pr_number} at {head_sha or 'unknown'} ({mode})", - ) - return False - issue_urls = _create_approved_followup_issues( + ) + post_pr_comment( runner, config=config, pr_number=pr_number, - reconciliation=reconciliation, + body=TrustedBody.canonical(body, expected_tokens=("AGENT_APPROVED_FOLLOWUPS",)), ) - if issue_urls: - body = _format_created_followup_issue_summary(pr_number, issue_urls, reconciliation) - body = _append_approved_followups_marker( - body, - pr_number=pr_number, - head_sha=head_sha, - mode=mode, - ) - post_pr_comment( - runner, - config=config, - pr_number=pr_number, - body=TrustedBody.canonical(body, expected_tokens=("AGENT_APPROVED_FOLLOWUPS",)), - ) - return True + return True return False @@ -802,6 +1487,7 @@ def _format_plan_approval_summary_with_followups( reconciliation: PlanApprovedFollowupReconciliation | None = None, issue_urls: Sequence[str] = (), filing_enabled: bool = False, + publication_details: str | None = None, ) -> str: lines = [ f"Planning complete for issue #{issue_number}.", @@ -817,6 +1503,13 @@ def _format_plan_approval_summary_with_followups( lines.extend(["", "Filed future follow-up issues:", ""]) unique_issue_urls = list(dict.fromkeys(issue_urls)) lines.extend(f"- {issue_url}" for issue_url in unique_issue_urls) + if publication_details: + detail_lines = publication_details.splitlines() + if detail_lines and detail_lines[0].endswith(":"): + detail_lines = detail_lines[1:] + detail_lines = [line for line in detail_lines if line != "-- coding-review-agent-loop"] + if detail_lines: + lines.extend(["", "Publication details:", *detail_lines]) lines.extend( [ "", @@ -881,7 +1574,10 @@ def _publish_plan_approved_followups( plan_subject: str, issue_comments: Sequence[object], sources: Sequence[PlanApprovedFollowupSource], + source_context: FollowupSourceContext, allow_issue_filing: bool = True, + usage_context: object | None = None, + semantic_transport: SemanticTransport | None = None, ) -> bool: filing_enabled = allow_issue_filing and config.approved_followups in ("issue", "fix-and-issue") mode = "issue" if filing_enabled else "summarize" @@ -898,12 +1594,34 @@ def _publish_plan_approved_followups( ) return False + semantic_matcher = None + semantic_provider_matcher: SemanticDedupeMatcher | None = None + if filing_enabled and config.semantic_followup_dedupe and not config.dry_run: + semantic_provider_matcher = SemanticDedupeMatcher( + runner=runner, + config=config, + usage_context=usage_context, + transport=semantic_transport, + ) + semantic_matcher = _semantic_batch_matcher( + semantic_provider_matcher, + source_context=source_context, + ) reconciliation = ( - reconcile_plan_approved_followups(sources, issue_limit=MAX_APPROVED_FOLLOWUP_ISSUES) + reconcile_plan_approved_followups( + sources, + issue_limit=( + len(sources) or MAX_APPROVED_FOLLOWUP_ISSUES + if filing_enabled + else MAX_APPROVED_FOLLOWUP_ISSUES + ), + semantic_matcher=semantic_matcher, + ) if sources else None ) issue_urls: list[str] = [] + publication_details: str | None = None if reconciliation is not None and reconciliation.selected_groups: log( config, @@ -913,14 +1631,21 @@ def _publish_plan_approved_followups( f"{reconciliation.skipped_by_cap} skipped by cap", ) if filing_enabled: - issue_urls = _create_plan_approved_followup_issues( + publication_details, created_urls = _publish_issue_followup_groups( runner, config=config, + groups=reconciliation.groups, + source_context=source_context, + heading="Created approved-plan future follow-up issues:", + deduplicated_count=reconciliation.deduplicated_count, + skipped_by_cap=reconciliation.skipped_by_cap, issue_number=issue_number, plan_hash=plan_hash, plan_subject=plan_subject, - reconciliation=reconciliation, + semantic_matcher=semantic_provider_matcher, + semantic_transport=semantic_transport, ) + issue_urls = list(created_urls) # The approved plan is a re-rendered historical GitHub artifact. Its # encoded plan metadata may contain durable records, but those records are @@ -931,6 +1656,7 @@ def _publish_plan_approved_followups( reconciliation=reconciliation, issue_urls=issue_urls, filing_enabled=filing_enabled, + publication_details=publication_details, ) body = _append_plan_approved_followups_marker( body, diff --git a/src/coding_review_agent_loop/github.py b/src/coding_review_agent_loop/github.py index 87b8bf5..d52010b 100644 --- a/src/coding_review_agent_loop/github.py +++ b/src/coding_review_agent_loop/github.py @@ -1823,6 +1823,7 @@ def search_issues( config: AgentLoopConfig, search: str, state: str = "all", + limit: int = ISSUE_RECOVERY_SEARCH_LIMIT, ) -> tuple[FoundIssue, ...]: """Search issues in `config.repo`, used to recover from a create-then-crash window (#476). @@ -1834,7 +1835,9 @@ def search_issues( materialization path still previews creations instead of "adopting" nothing. """ - log(config, f"Searching GitHub issues in {config.repo}: {search}") + if isinstance(limit, bool) or not isinstance(limit, int) or limit <= 0: + raise AgentLoopError("search_issues limit must be a positive integer") + log(config, f"Searching GitHub issues in {config.repo}: {search} (limit={limit})") if config.dry_run: runner.run( [ @@ -1848,7 +1851,7 @@ def search_issues( "--state", state, "--limit", - str(ISSUE_RECOVERY_SEARCH_LIMIT), + str(limit), "--json", "number,title,url,body", ], @@ -1867,7 +1870,7 @@ def search_issues( "--state", state, "--limit", - str(ISSUE_RECOVERY_SEARCH_LIMIT), + str(limit), "--json", "number,title,url,body", ], diff --git a/src/coding_review_agent_loop/managed_ci.py b/src/coding_review_agent_loop/managed_ci.py index 404135f..87874f6 100644 --- a/src/coding_review_agent_loop/managed_ci.py +++ b/src/coding_review_agent_loop/managed_ci.py @@ -1257,6 +1257,9 @@ def authenticate_source_managed_resume( "--pr-review-context-mode", "--expected-closing-issue", "--plan-execution-mode", "--flat-child-limit", "--split-stage", "--head", "--title", "--body-file", "--approved-followups", "--containment-mode", "--containment-memory-high", + "--semantic-followup-backend", "--semantic-followup-model", + "--semantic-followup-timeout-seconds", "--semantic-followup-max-calls", + "--semantic-followup-max-candidates", "--semantic-followup-prompt-char-limit", "--containment-memory-max", "--containment-memory-swap-max", "--containment-tasks-max", "--containment-aggregate-memory-high", "--containment-aggregate-memory-max", "--containment-aggregate-memory-swap-max", "--containment-aggregate-tasks-max", diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index cb8d2d0..e76cc7e 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -66,6 +66,7 @@ from .github import ( CiWatchOutcome, IssueContext, + PullRequestMetadata, PullRequestChecks, PullRequestMergeability, PullRequestReviewContext, @@ -352,6 +353,7 @@ _publish_plan_approved_followups, _normalize_followup_key, _publish_approved_followups, + FollowupSourceContext, ) from .round_state import ( PostedRoundMetadata, @@ -5777,7 +5779,15 @@ def _publish_plan_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> plan_subject=plan_subject, issue_comments=issue_context.comments, sources=approved_future_followup_sources, + source_context=FollowupSourceContext( + repo=config.repo, + source_kind="plan", + source_number=issue_number, + source_identity=plan_hash, + parent_issue_numbers=(issue_number,), + ), allow_issue_filing=mode in {"implement-one-shot", "implement-by-phase"}, + usage_context=usage_context, ) split_scope_materialized = _handle_plan_first_split_scope( runner, @@ -6980,6 +6990,34 @@ def _stop_on_terminal_without_status( return 0 +def _pr_followup_source_context( + *, + config: AgentLoopConfig, + pr_number: int, + pr_metadata: PullRequestMetadata, + issue_context: IssueContext | None, +) -> FollowupSourceContext: + linked = parse_linked_issue_numbers(pr_metadata.body, repo=config.repo) + parent_numbers = (issue_context.number,) if issue_context is not None else () + related = tuple(number for number in linked if number not in parent_numbers) + if issue_context is None and len(linked) > 1: + log( + config, + f"PR #{pr_number} has multiple linked issue references; preserving them as related context instead of inventing a parent", + ) + if issue_context is None and not linked: + log(config, f"Unable to resolve a parent issue for PR #{pr_number} follow-up lookup; using PR and topic context") + return FollowupSourceContext( + repo=config.repo, + source_kind="pr", + source_number=pr_number, + source_identity=pr_metadata.head_sha, + parent_issue_numbers=parent_numbers, + related_issue_numbers=related, + related_pr_numbers=(pr_number,), + ) + + def _stop_after_ci_watch_timeout( runner: Runner, *, @@ -6991,6 +7029,8 @@ def _stop_after_ci_watch_timeout( followups: list[ApprovedFollowup], details: list[str], reason: Literal["budget_exhausted", "timeout"], + source_context: FollowupSourceContext, + usage_context: RunUsageContext | None = None, ) -> int: """Publish resumable guidance for a watch that cannot continue or finish.""" _publish_approved_followups( @@ -7000,6 +7040,8 @@ def _stop_after_ci_watch_timeout( head_sha=head_sha, pr_comments=pr_comments, followups=followups, + source_context=source_context, + usage_context=usage_context, ) post_pr_comment( runner, @@ -7498,6 +7540,12 @@ def managed_ci_active(metadata: PullRequestMetadata) -> bool: initial_pr_context = pr_context pr_metadata = pr_context.metadata pr_comments = pr_context.comments + followup_source_context = _pr_followup_source_context( + config=config, + pr_number=pr_number, + pr_metadata=pr_metadata, + issue_context=issue_context, + ) if closing_contract is not None: validate_pr_expected_closing_issues( runner, @@ -8518,6 +8566,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, ) post_pr_comment( runner, @@ -8587,6 +8637,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, details=[ "The shared CI watch budget was exhausted by earlier watcher rounds; " "no fresh CI poll was performed." @@ -8625,6 +8677,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, ) run_optional_tests(runner, config) if config.auto_merge: @@ -8673,6 +8727,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, ) assert watch_outcome.stall is not None post_pr_comment( @@ -8707,6 +8763,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, details=details, reason="timeout", ) @@ -8773,6 +8831,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, ) post_pr_comment( runner, @@ -8842,6 +8902,8 @@ def _publish_pr_completion(reviewer: AgentName, turn: _ReviewerTurnResult) -> No head_sha=pr_metadata.head_sha, pr_comments=pr_comments, followups=future_followups, + source_context=followup_source_context, + usage_context=usage_context, ) run_optional_tests(runner, config) if config.auto_merge or managed_ci_active(pr_metadata): diff --git a/src/coding_review_agent_loop/semantic_dedupe.py b/src/coding_review_agent_loop/semantic_dedupe.py new file mode 100644 index 0000000..385a39c --- /dev/null +++ b/src/coding_review_agent_loop/semantic_dedupe.py @@ -0,0 +1,302 @@ +"""Strict, bounded semantic matching for approved follow-up trackers. + +The publisher owns policy (which candidates are eligible and when a match may +be reused). This module only builds the small prompt, invokes the selected +cheap provider, and validates its JSON result. Keeping that boundary narrow +makes provider failures conservative and keeps the model from becoming a +second publishing authority. +""" + +from __future__ import annotations + +import json +import tempfile +from dataclasses import dataclass, replace +from pathlib import Path +from typing import Callable, Literal, TYPE_CHECKING + +from .agents.base import AgentName +from .agents.registry import run_agent_result +from .errors import AgentLoopError +from .protocol_markers import sanitize_historical_text + +if TYPE_CHECKING: + from .config import AgentLoopConfig + from .runner import Runner + from .usage import RunUsageContext + + +SemanticConfidence = Literal["high", "medium", "low"] + + +@dataclass(frozen=True) +class SemanticCandidate: + """A bounded, repository-validated candidate presented to the matcher.""" + + identity: int | str + title: str + body: str + + +@dataclass(frozen=True) +class SemanticMatch: + duplicate_of: int | str | None + confidence: SemanticConfidence + reason: str + + +@dataclass(frozen=True) +class SemanticProviderResult: + text: str + result: object | None = None + + +SemanticTransport = Callable[[str, "Runner", "AgentLoopConfig", float], SemanticProviderResult | str] + + +def _excerpt(text: str, limit: int) -> str: + # Prompt data is historical/untrusted too. It is sanitized before it is + # copied into the provider prompt and bounded so a large issue cannot turn + # a cheap reconciliation call into a full review. + safe = sanitize_historical_text(text or "") + return safe[:limit] + + +def _identity_label(identity: int | str) -> str: + if isinstance(identity, bool): + raise AgentLoopError("semantic candidate identity cannot be boolean") + if isinstance(identity, int): + if identity <= 0: + raise AgentLoopError("semantic issue identity must be positive") + return f"issue:{identity}" + if isinstance(identity, str) and identity.startswith("group-"): + suffix = identity.removeprefix("group-") + if suffix.isdigit() and int(suffix) > 0: + return identity + raise AgentLoopError(f"invalid semantic candidate identity: {identity!r}") + + +def parse_semantic_match(raw: str, *, allowed_ids: set[int | str]) -> SemanticMatch: + """Validate the exact semantic matcher contract without repair or coercion.""" + try: + payload = json.loads(raw) + except (TypeError, json.JSONDecodeError) as exc: + raise AgentLoopError("semantic dedupe provider returned invalid JSON") from exc + if not isinstance(payload, dict) or set(payload) != {"duplicate_of", "confidence", "reason"}: + raise AgentLoopError("semantic dedupe result must contain exactly duplicate_of, confidence, and reason") + + duplicate_of = payload["duplicate_of"] + if duplicate_of is not None: + if isinstance(duplicate_of, bool) or not isinstance(duplicate_of, (int, str)): + raise AgentLoopError("semantic duplicate_of has the wrong type") + if duplicate_of not in allowed_ids: + # Existing-issue responses may use either the documented numeric + # form or the explicit issue:N form. Both must be known locally. + if not ( + isinstance(duplicate_of, str) + and duplicate_of.startswith("issue:") + and duplicate_of.removeprefix("issue:").isdigit() + and int(duplicate_of.removeprefix("issue:")) in allowed_ids + ): + raise AgentLoopError("semantic duplicate_of is not an allowed candidate") + duplicate_of = int(duplicate_of.removeprefix("issue:")) + + confidence = payload["confidence"] + if confidence not in {"high", "medium", "low"}: + raise AgentLoopError("semantic confidence must be high, medium, or low") + reason = payload["reason"] + if not isinstance(reason, str) or not reason.strip(): + raise AgentLoopError("semantic reason must be a non-empty string") + return SemanticMatch( + duplicate_of=duplicate_of, + confidence=confidence, + reason=reason.strip(), + ) + + +def build_semantic_prompt( + *, + proposed: str, + candidates: tuple[SemanticCandidate, ...], + source_context: str, + prompt_char_limit: int, +) -> str: + if not candidates: + raise AgentLoopError("semantic dedupe requires at least one candidate") + if prompt_char_limit <= 0: + raise AgentLoopError("semantic prompt budget must be positive") + candidate_budget = max(256, prompt_char_limit // len(candidates)) + lines = [ + "You are a cheap semantic duplicate classifier for approved future follow-up issues.", + "Do not use tools, browse, execute commands, or infer facts outside this prompt.", + "Compare the actual deliverable, not merely shared topic words.", + "Return one strict JSON object and no markdown or explanatory text:", + '{"duplicate_of": null, "confidence": "low", "reason": "No candidate tracks the same deliverable."}', + "Use duplicate_of=null when no candidate is equivalent. Only use high confidence when the proposed work is the same deliverable; related or complementary work is not a duplicate.", + f"Source context: {_excerpt(source_context, min(1200, prompt_char_limit // 4))}", + f"Proposed follow-up: {_excerpt(proposed, min(1600, prompt_char_limit // 3))}", + "Candidates:", + ] + for candidate in candidates: + lines.extend( + [ + f"- {_identity_label(candidate.identity)}:", + f" title: {_excerpt(candidate.title, candidate_budget // 3)}", + f" body: {_excerpt(candidate.body, candidate_budget)}", + ] + ) + return "\n".join(lines)[:prompt_char_limit] + + +def _isolated_provider_config(config: "AgentLoopConfig", backend: AgentName, model: str): + """Remove configured tool flags and place the provider in an empty temp dir.""" + # No configured checkout or dangerous-agent flag is exposed to this + # read-only classification turn. + isolated = Path(tempfile.mkdtemp(prefix="coding-review-followup-dedupe-")) + values: dict[str, object] = { + "coder": backend, + "reviewer": (backend,), + "claude_dir": isolated, + "codex_dir": isolated, + "gemini_dir": isolated, + "antigravity_dir": isolated, + "claude_args": (), + "codex_args": (), + "gemini_args": (), + "antigravity_args": (), + "dry_run": False, + } + if backend == "claude": + values["claude_model"] = model + elif backend == "codex": + values["codex_model"] = model + elif backend == "gemini": + values["gemini_model"] = model + elif backend == "antigravity": + values["antigravity_model"] = model or config.antigravity_models[0] + return replace(config, **values), isolated + + +def default_semantic_transport( + prompt: str, + runner: "Runner", + config: "AgentLoopConfig", + timeout_seconds: float, +) -> SemanticProviderResult: + backend = config.semantic_followup_backend + model = config.semantic_followup_model.strip() + isolated_config, isolated_dir = _isolated_provider_config(config, backend, model) + try: + result = run_agent_result( + runner, + agent=backend, + config=isolated_config, + prompt=prompt, + role="semantic-dedupe", + label="semantic-followup-dedupe", + timeout_seconds=timeout_seconds, + ) + return SemanticProviderResult(text=result.text, result=result) + finally: + # The directory is outside the repository and contains no durable + # orchestration state. Best-effort cleanup is deliberately omitted + # from exception handling so provider/control-flow errors propagate. + import shutil + + shutil.rmtree(isolated_dir, ignore_errors=True) + + +class SemanticDedupeMatcher: + """Bounded matcher used by both in-batch and existing-issue reconciliation.""" + + def __init__( + self, + *, + runner: "Runner", + config: "AgentLoopConfig", + transport: SemanticTransport | None = None, + usage_context: "RunUsageContext | None" = None, + ) -> None: + self.runner = runner + self.config = config + self.transport = transport or default_semantic_transport + self.usage_context = usage_context + self.calls = 0 + + def match( + self, + *, + proposed: str, + candidates: tuple[SemanticCandidate, ...], + source_context: str, + ) -> SemanticMatch: + if self.calls >= self.config.semantic_followup_max_calls: + raise BudgetExhausted("semantic call budget exhausted") + if len(candidates) > self.config.semantic_followup_max_candidates: + candidates = candidates[: self.config.semantic_followup_max_candidates] + if not candidates: + raise AgentLoopError("semantic dedupe requires a narrowed candidate set") + self.calls += 1 + prompt = build_semantic_prompt( + proposed=proposed, + candidates=candidates, + source_context=source_context, + prompt_char_limit=self.config.semantic_followup_prompt_char_limit, + ) + response = self.transport( + prompt, + self.runner, + self.config, + float(self.config.semantic_followup_timeout_seconds), + ) + if isinstance(response, str): + raw = response + provider_result = None + else: + raw = response.text + provider_result = response.result + if self.usage_context is not None and provider_result is not None: + from .usage import estimate_usage + + usage = getattr(provider_result, "usage", None) or estimate_usage(prompt, raw) + self.usage_context.add_record( + agent=self.config.semantic_followup_backend, + session_id=getattr(provider_result, "session_id", None), + returncode=getattr(provider_result, "returncode", 0), + usage=usage, + raw_backend_usage=getattr(provider_result, "raw_usage", None), + turn_role="semantic-dedupe", + model=getattr(provider_result, "model_used", None), + configured_model=getattr(provider_result, "configured_model", None), + configured_effort=getattr(provider_result, "configured_effort", None), + effort_source=getattr(provider_result, "effort_source", None), + observed_model=getattr(provider_result, "observed_model", None), + observed_effort=getattr(provider_result, "observed_effort", None), + observation_provenance=getattr(provider_result, "observation_provenance", None), + outcome="succeeded", + log_path=str(getattr(provider_result, "log_path", "")) or None, + containment=(getattr(provider_result, "containment", None).to_dict() + if getattr(provider_result, "containment", None) is not None + and hasattr(getattr(provider_result, "containment", None), "to_dict") + else None), + ).validation_status = "validated" + allowed = {candidate.identity for candidate in candidates} + return parse_semantic_match(raw, allowed_ids=allowed) + + +class BudgetExhausted(AgentLoopError): + """Local semantic budget exhaustion; callers retain deterministic behavior.""" + + +__all__ = [ + "BudgetExhausted", + "SemanticCandidate", + "SemanticConfidence", + "SemanticDedupeMatcher", + "SemanticMatch", + "SemanticProviderResult", + "SemanticTransport", + "build_semantic_prompt", + "default_semantic_transport", + "parse_semantic_match", +] diff --git a/tests/test_followups.py b/tests/test_followups.py new file mode 100644 index 0000000..2c0d4c9 --- /dev/null +++ b/tests/test_followups.py @@ -0,0 +1,177 @@ +"""Focused semantic approved-follow-up publication tests (#490).""" + +from __future__ import annotations + +import json +from types import SimpleNamespace + +import pytest + +from agent_loop_helpers import FakeRunner, make_config +from coding_review_agent_loop.errors import QuotaResetExceededError +from coding_review_agent_loop.followups import ( + FollowupSourceContext, + _publish_approved_followups, +) +from coding_review_agent_loop.protocol import ApprovedFollowup +from coding_review_agent_loop.semantic_dedupe import parse_semantic_match + + +def _source(*, parent: int) -> FollowupSourceContext: + return FollowupSourceContext( + repo="OWNER/REPO", + source_kind="pr", + source_number=488, + source_identity="head-488", + parent_issue_numbers=(parent,), + related_pr_numbers=(488,), + ) + + +@pytest.mark.parametrize( + ("existing_number", "parent", "candidate_title", "candidate_body", "proposed"), + ( + ( + 484, + 473, + "Follow up future plan-review note: Improve salvage recovery coverage", + "Future follow-up from approved planning for issue #473.\n" + "Capture untracked-only salvage work when `git diff HEAD` is empty; stage intent-to-add " + "or copy untracked files into salvage artifacts. " + "", + "Ensure salvage captures files present only in the worktree even when the HEAD diff is empty; " + "consider intent-to-add staging or copying those files into the recovery artifact.", + ), + ( + 746, + 744, + "Follow up future plan-review note: Isolate dispatch controls", + "Future follow-up from approved planning for issue #744.\n" + "Add coder-dispatch guardrails and credential isolation, or route dispatch through an API broker.", + "Prevent subagents from directly dispatching workflows by adding credential boundaries and a " + "brokered control path; this remains deferred lifecycle work.", + ), + ), +) +def test_pr_review_reuses_semantically_equivalent_planning_tracker( + tmp_path, + existing_number, + parent, + candidate_title, + candidate_body, + proposed, +): + runner = FakeRunner( + search_issues_payload=[ + { + "number": existing_number, + "title": candidate_title, + "url": f"https://github.com/OWNER/REPO/issues/{existing_number}", + "body": candidate_body, + } + ] + ) + config = make_config(tmp_path, approved_followups="issue") + calls: list[str] = [] + + def transport(prompt, _runner, _config, _timeout): + calls.append(prompt) + assert "Proposed follow-up:" in prompt + assert candidate_body.splitlines()[1].split(" ", + } + ) + + published = _publish_approved_followups( + runner, + config=config, + pr_number=488, + head_sha="head-488", + pr_comments=[], + followups=[ApprovedFollowup(reviewer="Claude", text=proposed)], + source_context=_source(parent=parent), + semantic_transport=transport, + ) + + assert published is True + assert len(calls) == 1 + assert runner.issues == [] + assert f"https://github.com/OWNER/REPO/issues/{existing_number}" in runner.comments[-1] + assert "Claude" in runner.comments[-1] + assert runner.comments[-1].count("AGENT_APPROVED_FOLLOWUPS") == 1 + + +def test_semantic_matcher_rejects_unknown_and_boolean_identities(): + with pytest.raises(Exception): + parse_semantic_match( + '{"duplicate_of": true, "confidence": "high", "reason": "same"}', + allowed_ids={484}, + ) + with pytest.raises(Exception): + parse_semantic_match( + '{"duplicate_of": 999, "confidence": "high", "reason": "same"}', + allowed_ids={484}, + ) + with pytest.raises(Exception): + parse_semantic_match( + '{"duplicate_of": 484, "confidence": "high", "reason": "same", "extra": 1}', + allowed_ids={484}, + ) + + +def test_quota_reset_escapes_without_creation_or_publication(tmp_path): + runner = FakeRunner( + search_issues_payload=[ + { + "number": 484, + "title": "Follow up future plan-review note: salvage", + "url": "https://github.com/OWNER/REPO/issues/484", + "body": "Future follow-up from approved planning for issue #473. salvage", + } + ] + ) + config = make_config(tmp_path, approved_followups="issue") + + def transport(_prompt, _runner, _config, _timeout): + raise QuotaResetExceededError("quota reset is too far away") + + with pytest.raises(QuotaResetExceededError): + _publish_approved_followups( + runner, + config=config, + pr_number=488, + head_sha="head-488", + pr_comments=[], + followups=[ApprovedFollowup(reviewer="Claude", text="Capture salvage missed by an empty HEAD diff.")], + source_context=_source(parent=473), + semantic_transport=transport, + ) + assert runner.issues == [] + assert runner.comments == [] + + +def test_replay_skips_search_and_model(tmp_path): + runner = FakeRunner() + config = make_config(tmp_path, approved_followups="issue") + marker = "" + comments = [SimpleNamespace(body=marker)] + + def transport(*_args): + raise AssertionError("semantic model must not run during replay") + + assert _publish_approved_followups( + runner, + config=config, + pr_number=488, + head_sha="head-488", + pr_comments=comments, + followups=[ApprovedFollowup(reviewer="Claude", text="later")], + source_context=_source(parent=473), + semantic_transport=transport, + ) is False + assert runner.search_issues_calls == [] From 786a81ccf2cbda9413bc1a41e6023fe24b19badb Mon Sep 17 00:00:00 2001 From: Wild Wind Date: Tue, 8 Sep 2026 00:35:51 -0700 Subject: [PATCH 2/2] Fix semantic follow-up dedupe review blockers --- README.md | 11 + helpers/skill_runner.py | 17 +- src/coding_review_agent_loop/followups.py | 141 +++---------- src/coding_review_agent_loop/orchestrator.py | 2 - .../semantic_dedupe.py | 139 +++++++++---- tests/test_followups.py | 193 +++++++++++++++++- tests/test_orchestrator_pr.py | 4 + tests/test_skill_helpers.py | 4 +- 8 files changed, 335 insertions(+), 176 deletions(-) diff --git a/README.md b/README.md index f228c8e..15926a8 100644 --- a/README.md +++ b/README.md @@ -278,6 +278,17 @@ work. Read [Phased decomposition versus split materialization](docs/local_agent_loop.md#phased-decomposition-versus-split-materialization) before filing child issues. +### Approved follow-up dedupe + +When approved future follow-ups are summarized or filed, semantic reuse is +enabled by default after deterministic narrowing. Use +`--no-semantic-followup-dedupe` for deterministic-only operation. The provider +and its bounds are configurable with `--semantic-followup-backend`, +`--semantic-followup-model`, `--semantic-followup-timeout-seconds`, +`--semantic-followup-max-calls`, `--semantic-followup-max-candidates`, and +`--semantic-followup-prompt-char-limit`. Only high-confidence matches suppress +or merge work; uncertain matches are filed with a possible-duplicate note. + ## Discuss Mode Discuss mode asks agents to evaluate an issue without modifying the repository. diff --git a/helpers/skill_runner.py b/helpers/skill_runner.py index fe644a2..359d6c0 100644 --- a/helpers/skill_runner.py +++ b/helpers/skill_runner.py @@ -92,7 +92,6 @@ import argparse import dataclasses import hashlib -import inspect import json import os import re @@ -885,6 +884,7 @@ def _publish_pr_followups( pr_comments: list, *, dry_run: bool, + parent_issue_number: int | None = None, ) -> dict: """Publish approved future follow-ups for a PR via the library logic (#300). @@ -921,18 +921,10 @@ def _publish_pr_followups( source_kind="pr", source_number=pr, source_identity=head_sha, + parent_issue_numbers=(parent_issue_number,) if parent_issue_number is not None else (), related_pr_numbers=(pr,), ), } - # Keep old test/integration adapters that monkeypatch the publisher with - # the pre-context signature callable; the production call always carries - # explicit source context. - signature = inspect.signature(_publish_approved_followups) - if "source_context" not in signature.parameters and not any( - parameter.kind == inspect.Parameter.VAR_KEYWORD - for parameter in signature.parameters.values() - ): - publish_kwargs.pop("source_context") published = _publish_approved_followups(Runner(dry_run=False), **publish_kwargs) return {"mode": mode, "published": bool(published), "count": len(approved)} @@ -2541,8 +2533,9 @@ def cmd_run_pr_round(args: argparse.Namespace) -> None: else: round_approved_reviewers.append(str(record.get("reviewer_name", ""))) - pr_diff = _fetch_pr_diff(repo, pr) issue_dict = _fetch_pr_json(repo, pr) + parent_issue_number = _linked_issue_number_from_pr(issue_dict) + pr_diff = _fetch_pr_diff(repo, pr) # Repo-scoped agent memory for reviewer orientation (#306), prepared once. memory = None @@ -2717,7 +2710,7 @@ def cmd_run_pr_round(args: argparse.Namespace) -> None: ] result_json["approved_followups"] = _publish_pr_followups( repo, pr, head_sha, followups_mode, round_future_items, pr_comments, - dry_run=dry_run, + dry_run=dry_run, parent_issue_number=parent_issue_number, ) usage = _aggregate_reviewer_usage(round_reviewer_records) if usage is not None: diff --git a/src/coding_review_agent_loop/followups.py b/src/coding_review_agent_loop/followups.py index 2c42331..e0ce378 100644 --- a/src/coding_review_agent_loop/followups.py +++ b/src/coding_review_agent_loop/followups.py @@ -376,12 +376,6 @@ def reconcile_approved_followups( candidate_index = int(suffix) - 1 if 0 <= candidate_index < len(grouped): existing_index = candidate_index - elif isinstance(target, int) and not isinstance(target, bool): - # Test/in-process matchers may use the natural zero-based - # group identity. Existing issue numbers are never valid - # here because batch groups carry string identities. - if 0 <= target < len(grouped): - existing_index = target if existing_index is None: indexes.update((key, len(grouped)) for key in keys) grouped.append(GroupedApprovedFollowup(text=followup.text, items=(followup,))) @@ -722,11 +716,7 @@ def add_query(value: str) -> None: add_query(f'{prefix} "future follow-up"') found: dict[int, FoundIssue] = {} - excluded_numbers = { - source_context.source_number - if source_context.source_kind == "plan" - else -1 - } + excluded_numbers = {source_context.source_number, *source_context.parent_issue_numbers} for query in queries: try: results = search_issues( @@ -817,6 +807,12 @@ def _format_publication_summary( deduplicated_count: int, skipped_by_cap: int, ) -> str: + filed = any(publication.status in {"created", "uncertain"} for publication in publications) + if not filed: + if any(publication.status == "reused" for publication in publications): + heading = f"Reused existing {heading.removeprefix('Created ')}" + else: + heading = "Approved future follow-up publication results:" lines = [heading, ""] shown_targets: set[str] = set() for publication in publications: @@ -876,6 +872,8 @@ def _followup_issue_body( f"Future follow-up from approved review on PR #{pr_number}.", "", ] + if source_context is not None: + lines.extend(["Source context:", f"- Lookup context: {source_context.render()}", ""]) reviewers = tuple(sanitize_historical_text(reviewer) for reviewer in followup.reviewers) if len(reviewers) == 1: lines.append(f"Reviewer: {reviewers[0]}") @@ -994,32 +992,6 @@ def _plan_followup_issue_body( return "\n".join(lines) -def _create_plan_approved_followup_issues( - runner: Runner, - *, - config: AgentLoopConfig, - issue_number: int, - plan_hash: str, - plan_subject: str, - reconciliation: PlanApprovedFollowupReconciliation, -) -> list[str]: - issue_urls: list[str] = [] - for followup in reconciliation.selected_groups: - issue_url = create_issue( - runner, - config=config, - title=_plan_followup_issue_title(followup), - body=_plan_followup_issue_body( - issue_number=issue_number, - plan_hash=plan_hash, - plan_subject=plan_subject, - followup=followup, - ), - ) - issue_urls.append(issue_url or "Created issue URL unavailable from GitHub CLI output.") - return issue_urls - - def _validated_created_issue_url(url: str | None, *, repo: str) -> tuple[int | None, str | None]: if not url: return None, None @@ -1129,7 +1101,7 @@ def _publish_issue_followup_groups( plan_hash: str | None = None, semantic_matcher: SemanticDedupeMatcher | None = None, semantic_transport: SemanticTransport | None = None, -) -> tuple[str, tuple[str, ...]]: +) -> tuple[str, tuple[str, ...], tuple[FollowupPublication, ...]]: followups = [ApprovedFollowup(reviewer=group.reviewers[0], text=group.text) for group in groups] candidates = _search_followup_trackers( runner, @@ -1287,67 +1259,7 @@ def _publish_issue_followup_groups( publication.issue_url for publication in publications if publication.issue_url - ) - - -def _create_approved_followup_issues( - runner: Runner, - *, - config: AgentLoopConfig, - pr_number: int, - reconciliation: ApprovedFollowupReconciliation, -) -> list[str]: - issue_urls: list[str] = [] - for followup in reconciliation.selected_groups: - issue_url = create_issue( - runner, - config=config, - title=_followup_issue_title( - ApprovedFollowup(reviewer=followup.reviewers[0], text=followup.text) - ), - body=_followup_issue_body(pr_number, followup), - ) - if issue_url is not None: - issue_urls.append(issue_url) - return issue_urls - - -def _format_created_followup_issue_summary( - pr_number: int, - issue_urls: list[str], - reconciliation: ApprovedFollowupReconciliation, -) -> str: - unique_issue_urls = list(dict.fromkeys(issue_urls)) - lines = [ - f"Created approved-review future follow-up issues for PR #{pr_number}:", - "", - ] - if unique_issue_urls: - lines.extend(f"- {issue_url}" for issue_url in unique_issue_urls) - else: - lines.append("- Created issue URL unavailable from GitHub CLI output.") - lines.extend( - [ - "", - ( - f"Reconciliation: {len(unique_issue_urls)} filed, " - f"{reconciliation.deduplicated_count} deduplicated, " - f"{reconciliation.skipped_by_cap} skipped by cap." - ), - "", - "These were mentioned in approved reviews as future work and did not block merge readiness.", - ] - ) - if reconciliation.skipped_by_cap > 0: - lines.extend( - [ - "", - f"Skipped {reconciliation.skipped_by_cap} additional item(s) to avoid issue noise; reviewers should reserve " - "this section for substantial independent follow-up work.", - ] - ) - lines.extend(["", "-- coding-review-agent-loop"]) - return "\n".join(lines) + ), tuple(publications) def _publish_approved_followups( @@ -1436,7 +1348,7 @@ def _publish_approved_followups( return True if mode == "issue": - publication_body, _issue_urls = _publish_issue_followup_groups( + publication_body, issue_urls, _publications = _publish_issue_followup_groups( runner, config=config, groups=groups, @@ -1447,11 +1359,9 @@ def _publish_approved_followups( semantic_matcher=semantic_provider_matcher, semantic_transport=semantic_transport, ) - if not _issue_urls: + if not issue_urls: # Preserve the historical fail-safe when GitHub did not return a - # usable identity for any newly-created issue. Reused trackers - # are included in _issue_urls and therefore still publish the - # durable audit record. + # usable identity for any newly-created or reused tracker. return False body = _append_approved_followups_marker( publication_body, @@ -1500,9 +1410,10 @@ def _format_plan_approval_summary_with_followups( ] if reconciliation is not None and reconciliation.selected_groups: if filing_enabled: - lines.extend(["", "Filed future follow-up issues:", ""]) unique_issue_urls = list(dict.fromkeys(issue_urls)) - lines.extend(f"- {issue_url}" for issue_url in unique_issue_urls) + if unique_issue_urls: + lines.extend(["", "Filed future follow-up issues:", ""]) + lines.extend(f"- {issue_url}" for issue_url in unique_issue_urls) if publication_details: detail_lines = publication_details.splitlines() if detail_lines and detail_lines[0].endswith(":"): @@ -1510,16 +1421,6 @@ def _format_plan_approval_summary_with_followups( detail_lines = [line for line in detail_lines if line != "-- coding-review-agent-loop"] if detail_lines: lines.extend(["", "Publication details:", *detail_lines]) - lines.extend( - [ - "", - ( - f"Reconciliation: {len(reconciliation.selected_groups)} filed, " - f"{reconciliation.deduplicated_count} deduplicated, " - f"{reconciliation.skipped_by_cap} skipped by cap." - ), - ] - ) else: lines.extend( [ @@ -1631,7 +1532,7 @@ def _publish_plan_approved_followups( f"{reconciliation.skipped_by_cap} skipped by cap", ) if filing_enabled: - publication_details, created_urls = _publish_issue_followup_groups( + publication_details, _issue_urls, publications = _publish_issue_followup_groups( runner, config=config, groups=reconciliation.groups, @@ -1645,7 +1546,11 @@ def _publish_plan_approved_followups( semantic_matcher=semantic_provider_matcher, semantic_transport=semantic_transport, ) - issue_urls = list(created_urls) + issue_urls = [ + publication.issue_url + for publication in publications + if publication.status in {"created", "uncertain"} and publication.issue_url + ] # The approved plan is a re-rendered historical GitHub artifact. Its # encoded plan metadata may contain durable records, but those records are diff --git a/src/coding_review_agent_loop/orchestrator.py b/src/coding_review_agent_loop/orchestrator.py index e76cc7e..75771a4 100644 --- a/src/coding_review_agent_loop/orchestrator.py +++ b/src/coding_review_agent_loop/orchestrator.py @@ -340,13 +340,11 @@ _append_approved_followups_marker, _approved_followup_from_unresolved_item, _approved_followups_marker, - _create_approved_followup_issues, _dedupe_approved_followups, _followup_heading_key, _followup_issue_body, _followup_issue_title, _format_approved_followup_summary, - _format_created_followup_issue_summary, _format_same_pr_followups, _has_approved_followups_marker, _plan_followup_source_from_unresolved_item, diff --git a/src/coding_review_agent_loop/semantic_dedupe.py b/src/coding_review_agent_loop/semantic_dedupe.py index 385a39c..3eb8df0 100644 --- a/src/coding_review_agent_loop/semantic_dedupe.py +++ b/src/coding_review_agent_loop/semantic_dedupe.py @@ -125,56 +125,101 @@ def build_semantic_prompt( raise AgentLoopError("semantic dedupe requires at least one candidate") if prompt_char_limit <= 0: raise AgentLoopError("semantic prompt budget must be positive") - candidate_budget = max(256, prompt_char_limit // len(candidates)) - lines = [ + + instruction_lines = [ "You are a cheap semantic duplicate classifier for approved future follow-up issues.", "Do not use tools, browse, execute commands, or infer facts outside this prompt.", "Compare the actual deliverable, not merely shared topic words.", "Return one strict JSON object and no markdown or explanatory text:", '{"duplicate_of": null, "confidence": "low", "reason": "No candidate tracks the same deliverable."}', "Use duplicate_of=null when no candidate is equivalent. Only use high confidence when the proposed work is the same deliverable; related or complementary work is not a duplicate.", - f"Source context: {_excerpt(source_context, min(1200, prompt_char_limit // 4))}", - f"Proposed follow-up: {_excerpt(proposed, min(1600, prompt_char_limit // 3))}", - "Candidates:", ] - for candidate in candidates: - lines.extend( - [ - f"- {_identity_label(candidate.identity)}:", - f" title: {_excerpt(candidate.title, candidate_budget // 3)}", - f" body: {_excerpt(candidate.body, candidate_budget)}", - ] + + # Reserve the candidate portion explicitly. The old implementation gave + # each candidate a budget before accounting for these instructions and + # then sliced the completed prompt, which could remove the tail of a + # candidate entry while leaving that identity in the allowed-id set. + base_prefix = "\n".join((*instruction_lines, "Source context: ", "Proposed follow-up: ", "Candidates:")) + available = prompt_char_limit - len(base_prefix) - 1 + if available <= 0: + raise AgentLoopError("semantic prompt budget is too small for the matcher instructions") + source_limit = min(1200, max(0, available // 8)) + proposed_limit = min(1600, max(0, available // 5)) + prefix = "\n".join( + ( + *instruction_lines, + f"Source context: {_excerpt(source_context, source_limit)}", + f"Proposed follow-up: {_excerpt(proposed, proposed_limit)}", + "Candidates:", + ) + ) + # ``str.join`` inserts one separator before every candidate entry. + remaining = prompt_char_limit - len(prefix) - len(candidates) + candidate_budget = remaining // len(candidates) + if candidate_budget <= 0: + raise AgentLoopError("semantic prompt budget is too small for candidate identities") + + def render_candidate(candidate: SemanticCandidate) -> str: + label = _identity_label(candidate.identity) + fixed = len(f"- {label}:\n title: \n body: ") + if fixed > candidate_budget: + raise AgentLoopError("semantic prompt budget is too small for candidate identities") + text_budget = candidate_budget - fixed + title_limit = text_budget // 3 + body_limit = text_budget - title_limit + return "\n".join( + ( + f"- {label}:", + f" title: {_excerpt(candidate.title, title_limit)}", + f" body: {_excerpt(candidate.body, body_limit)}", + ) ) - return "\n".join(lines)[:prompt_char_limit] + + candidate_lines: list[str] = [] + for candidate in candidates: + candidate_lines.append(render_candidate(candidate)) + prompt = "\n".join((prefix, *candidate_lines)) + if len(prompt) > prompt_char_limit: + raise AgentLoopError("semantic prompt candidate packing exceeded its budget") + return prompt def _isolated_provider_config(config: "AgentLoopConfig", backend: AgentName, model: str): """Remove configured tool flags and place the provider in an empty temp dir.""" # No configured checkout or dangerous-agent flag is exposed to this # read-only classification turn. - isolated = Path(tempfile.mkdtemp(prefix="coding-review-followup-dedupe-")) - values: dict[str, object] = { - "coder": backend, - "reviewer": (backend,), - "claude_dir": isolated, - "codex_dir": isolated, - "gemini_dir": isolated, - "antigravity_dir": isolated, - "claude_args": (), - "codex_args": (), - "gemini_args": (), - "antigravity_args": (), - "dry_run": False, - } - if backend == "claude": - values["claude_model"] = model - elif backend == "codex": - values["codex_model"] = model - elif backend == "gemini": - values["gemini_model"] = model - elif backend == "antigravity": - values["antigravity_model"] = model or config.antigravity_models[0] - return replace(config, **values), isolated + isolated: Path | None = None + try: + isolated = Path(tempfile.mkdtemp(prefix="coding-review-followup-dedupe-")) + values: dict[str, object] = { + "coder": backend, + "reviewer": (backend,), + "claude_dir": isolated, + "codex_dir": isolated, + "gemini_dir": isolated, + "antigravity_dir": isolated, + "claude_args": (), + "codex_args": (), + "gemini_args": (), + "antigravity_args": (), + "dry_run": False, + } + if backend == "claude": + values["claude_model"] = model + elif backend == "codex": + values["codex_model"] = model + elif backend == "gemini": + values["gemini_model"] = model + elif backend == "antigravity": + values["antigravity_model"] = None + values["antigravity_models"] = (model or config.antigravity_models[0],) + return replace(config, **values), isolated + except Exception: + if isolated is not None: + import shutil + + shutil.rmtree(isolated, ignore_errors=True) + raise def default_semantic_transport( @@ -185,8 +230,9 @@ def default_semantic_transport( ) -> SemanticProviderResult: backend = config.semantic_followup_backend model = config.semantic_followup_model.strip() - isolated_config, isolated_dir = _isolated_provider_config(config, backend, model) + isolated_dir: Path | None = None try: + isolated_config, isolated_dir = _isolated_provider_config(config, backend, model) result = run_agent_result( runner, agent=backend, @@ -203,7 +249,8 @@ def default_semantic_transport( # from exception handling so provider/control-flow errors propagate. import shutil - shutil.rmtree(isolated_dir, ignore_errors=True) + if isolated_dir is not None: + shutil.rmtree(isolated_dir, ignore_errors=True) class SemanticDedupeMatcher: @@ -255,11 +302,12 @@ def match( else: raw = response.text provider_result = response.result + usage_record = None if self.usage_context is not None and provider_result is not None: from .usage import estimate_usage usage = getattr(provider_result, "usage", None) or estimate_usage(prompt, raw) - self.usage_context.add_record( + usage_record = self.usage_context.add_record( agent=self.config.semantic_followup_backend, session_id=getattr(provider_result, "session_id", None), returncode=getattr(provider_result, "returncode", 0), @@ -279,9 +327,18 @@ def match( if getattr(provider_result, "containment", None) is not None and hasattr(getattr(provider_result, "containment", None), "to_dict") else None), - ).validation_status = "validated" + ) allowed = {candidate.identity for candidate in candidates} - return parse_semantic_match(raw, allowed_ids=allowed) + try: + match = parse_semantic_match(raw, allowed_ids=allowed) + except Exception: + if usage_record is not None: + usage_record.outcome = "invalid_output" + usage_record.validation_status = "invalid" + raise + if usage_record is not None: + usage_record.validation_status = "validated" + return match class BudgetExhausted(AgentLoopError): diff --git a/tests/test_followups.py b/tests/test_followups.py index 2c0d4c9..c4a2a81 100644 --- a/tests/test_followups.py +++ b/tests/test_followups.py @@ -8,13 +8,24 @@ import pytest from agent_loop_helpers import FakeRunner, make_config -from coding_review_agent_loop.errors import QuotaResetExceededError +from coding_review_agent_loop.errors import AgentLoopError, QuotaResetExceededError from coding_review_agent_loop.followups import ( FollowupSourceContext, + _followup_issue_body, + _semantic_batch_matcher, _publish_approved_followups, + reconcile_approved_followups, ) from coding_review_agent_loop.protocol import ApprovedFollowup -from coding_review_agent_loop.semantic_dedupe import parse_semantic_match +from coding_review_agent_loop.semantic_dedupe import ( + SemanticCandidate, + SemanticDedupeMatcher, + SemanticProviderResult, + _isolated_provider_config, + build_semantic_prompt, + parse_semantic_match, +) +from coding_review_agent_loop.usage import RunUsageContext def _source(*, parent: int) -> FollowupSourceContext: @@ -175,3 +186,181 @@ def transport(*_args): semantic_transport=transport, ) is False assert runner.search_issues_calls == [] + + +def test_semantic_batch_matcher_merges_high_confidence_group_identity(tmp_path): + runner = FakeRunner() + config = make_config(tmp_path, approved_followups="issue") + calls: list[str] = [] + + def transport(prompt, _runner, _config, _timeout): + calls.append(prompt) + return json.dumps( + { + "duplicate_of": "group-1", + "confidence": "high", + "reason": "Both describe preserving worktree-only recovery data.", + } + ) + + matcher = SemanticDedupeMatcher(runner=runner, config=config, transport=transport) + reconciliation = reconcile_approved_followups( + [ + ApprovedFollowup( + reviewer="Claude", + text="Preserve recovery artifacts for files that never enter the index.", + ), + ApprovedFollowup( + reviewer="Gemini", + text="Capture worktree-only files in the salvage artifact when the repository diff is empty.", + ), + ], + semantic_matcher=_semantic_batch_matcher(matcher, source_context=_source(parent=473)), + ) + + assert len(calls) == 1 + assert len(reconciliation.groups) == 1 + assert reconciliation.groups[0].reviewers == ("Claude", "Gemini") + assert reconciliation.deduplicated_count == 1 + + +def test_medium_confidence_files_with_possible_duplicate_note(tmp_path): + runner = FakeRunner( + search_issues_payload=[ + { + "number": 484, + "title": "Follow up future plan-review note: salvage recovery", + "url": "https://github.com/OWNER/REPO/issues/484", + "body": "Future follow-up from approved planning for issue #473. Preserve salvage artifacts.", + } + ], + issue_urls=["https://github.com/OWNER/REPO/issues/900"], + ) + config = make_config(tmp_path, approved_followups="issue") + + def transport(_prompt, _runner, _config, _timeout): + return json.dumps( + { + "duplicate_of": 484, + "confidence": "medium", + "reason": "The tracker may cover the same salvage work.", + } + ) + + assert _publish_approved_followups( + runner, + config=config, + pr_number=488, + head_sha="head-488", + pr_comments=[], + followups=[ + ApprovedFollowup( + reviewer="Claude", + text="Capture recovery data for files that exist only in the worktree.", + ) + ], + source_context=_source(parent=473), + semantic_transport=transport, + ) is True + + assert len(runner.issues) == 1 + assert "Possible duplicate (not suppressed because semantic confidence was not high):" in runner.issues[0]["body"] + assert "1 filed" in runner.comments[-1] + assert "0 reused; 1 uncertain" in runner.comments[-1] + + +def test_pr_followup_body_renders_lookup_context(): + followup = ApprovedFollowup(reviewer="Claude", text="Track the deferred recovery work.") + reconciliation = reconcile_approved_followups([followup]) + + body = _followup_issue_body( + 488, + reconciliation.selected_groups[0], + source_context=_source(parent=473), + ) + + assert "Lookup context:" in body + assert "parent issue(s)=#473" in body + + +def test_existing_parent_issue_is_not_reused_as_followup_tracker(tmp_path): + runner = FakeRunner( + search_issues_payload=[ + { + "number": 473, + "title": "Follow up future work", + "url": "https://github.com/OWNER/REPO/issues/473", + "body": "Future follow-up from approved review on PR #472.", + } + ], + issue_urls=["https://github.com/OWNER/REPO/issues/900"], + ) + config = make_config(tmp_path, approved_followups="issue", semantic_followup_dedupe=False) + + assert _publish_approved_followups( + runner, + config=config, + pr_number=488, + head_sha="head-488", + pr_comments=[], + followups=[ApprovedFollowup(reviewer="Claude", text="Track deferred recovery work.")], + source_context=_source(parent=473), + ) is True + assert len(runner.issues) == 1 + + +def test_semantic_prompt_keeps_all_candidate_entries_within_budget(): + candidates = tuple( + # Long excerpts make the old post-render slice drop later candidates. + SemanticCandidate( + identity=f"group-{index}", + title="candidate title " * 20, + body="candidate body " * 80, + ) + for index in range(1, 51) + ) + prompt = build_semantic_prompt( + proposed="proposed follow-up", + candidates=candidates, + source_context="repository=OWNER/REPO; source=pr#488; parent issue(s)=#473", + prompt_char_limit=12_000, + ) + + assert len(prompt) <= 12_000 + assert all(f"- group-{index}:" in prompt for index in range(1, 51)) + + +def test_antigravity_isolated_config_replaces_the_model_chain(tmp_path): + config = make_config( + tmp_path, + semantic_followup_backend="antigravity", + semantic_followup_model="Model X", + ) + isolated_config, isolated_dir = _isolated_provider_config(config, "antigravity", "Model X") + try: + assert isolated_config.antigravity_model is None + assert isolated_config.antigravity_models == ("Model X",) + finally: + import shutil + + shutil.rmtree(isolated_dir, ignore_errors=True) + + +def test_invalid_semantic_result_is_not_counted_as_validated_usage(tmp_path): + usage = RunUsageContext(run_id="semantic-invalid", summary_path=tmp_path / "usage.json") + matcher = SemanticDedupeMatcher( + runner=FakeRunner(), + config=make_config(tmp_path), + usage_context=usage, + transport=lambda *_args: SemanticProviderResult(text="not-json", result=SimpleNamespace()), + ) + + with pytest.raises(AgentLoopError): + matcher.match( + proposed="Track the deferred recovery work.", + candidates=(SemanticCandidate(identity=484, title="Existing", body="Recovery"),), + source_context=_source(parent=473).render(), + ) + + assert usage.records[0].outcome == "invalid_output" + assert usage.records[0].validation_status == "invalid" diff --git a/tests/test_orchestrator_pr.py b/tests/test_orchestrator_pr.py index 78f0b89..6f206aa 100644 --- a/tests/test_orchestrator_pr.py +++ b/tests/test_orchestrator_pr.py @@ -3121,6 +3121,8 @@ def test_pr_loop_creates_issues_for_approved_followups(tmp_path): "title": "Follow up future review note: Add cleanup docs.", "body": ( "Future follow-up from approved review on PR #77.\n\n" + "Source context:\n" + "- Lookup context: repository=OWNER/REPO; source=pr#77; identity=abc123; related PR(s)=#77\n\n" "Reviewer: Codex\n\n" "Follow-up:\n" "- Add cleanup docs.\n\n" @@ -3133,6 +3135,8 @@ def test_pr_loop_creates_issues_for_approved_followups(tmp_path): "title": "Follow up future review note: Add regression coverage.", "body": ( "Future follow-up from approved review on PR #77.\n\n" + "Source context:\n" + "- Lookup context: repository=OWNER/REPO; source=pr#77; identity=abc123; related PR(s)=#77\n\n" "Reviewer: Claude\n\n" "Follow-up:\n" "- Add regression coverage.\n\n" diff --git a/tests/test_skill_helpers.py b/tests/test_skill_helpers.py index e5b4dff..a3a413e 100644 --- a/tests/test_skill_helpers.py +++ b/tests/test_skill_helpers.py @@ -1836,12 +1836,13 @@ def test_publish_summarize_threads_mode_and_followups(self, monkeypatch) -> None import helpers.skill_runner as sr captured = {} - def fake_publish(runner, *, config, pr_number, head_sha, pr_comments, followups): + def fake_publish(runner, *, config, pr_number, head_sha, pr_comments, followups, **kwargs): captured["mode"] = config.approved_followups captured["pr_number"] = pr_number captured["head_sha"] = head_sha captured["followup_texts"] = [f.text for f in followups] captured["reviewers"] = [f.reviewer for f in followups] + captured["source_context"] = kwargs["source_context"] return True monkeypatch.setattr(sr, "_publish_approved_followups", fake_publish) @@ -1854,6 +1855,7 @@ def fake_publish(runner, *, config, pr_number, head_sha, pr_comments, followups) assert captured["pr_number"] == 42 and captured["head_sha"] == "deadbeef" assert captured["followup_texts"] == ["ship docs"] assert captured["reviewers"] == ["Gemini"] + assert captured["source_context"].parent_issue_numbers == () def test_publish_idempotent_with_existing_marker(self) -> None: # Exercises the real _publish_approved_followups: a pre-existing marker for