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/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..359d6c0 100644 --- a/helpers/skill_runner.py +++ b/helpers/skill_runner.py @@ -114,6 +114,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, ) @@ -883,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). @@ -908,14 +910,22 @@ 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, + parent_issue_numbers=(parent_issue_number,) if parent_issue_number is not None else (), + related_pr_numbers=(pr,), + ), + } + published = _publish_approved_followups(Runner(dry_run=False), **publish_kwargs) return {"mode": mode, "published": bool(published), "count": len(approved)} @@ -2523,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 @@ -2699,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/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..e0ce378 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,19 @@ 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 if existing_index is None: indexes.update((key, len(grouped)) for key in keys) grouped.append(GroupedApprovedFollowup(text=followup.text, items=(followup,))) @@ -319,6 +400,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 +411,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,15 +589,291 @@ 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, *source_context.parent_issue_numbers} + 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: + 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: + 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}.", "", ] + 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]}") @@ -523,6 +887,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 +931,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 +957,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 +974,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( [ "", @@ -608,90 +992,274 @@ def _plan_followup_issue_body( return "\n".join(lines) -def _create_plan_approved_followup_issues( +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, - 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, - ), + 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)}", ) - issue_urls.append(issue_url or "Created issue URL unavailable from GitHub CLI output.") - return issue_urls + 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 _create_approved_followup_issues( +def _publish_issue_followup_groups( 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( + 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, ...], tuple[FollowupPublication, ...]]: + 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, - title=_followup_issue_title( - ApprovedFollowup(reviewer=followup.reviewers[0], text=followup.text) - ), - body=_followup_issue_body(pr_number, followup), + followup=proposed, + candidates=candidates, + source_context=source_context, + matcher=matcher, + revalidated=revalidated, ) - 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.", - ] + 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, + ) ) - lines.extend(["", "-- coding-review-agent-loop"]) - return "\n".join(lines) + 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 + ), tuple(publications) def _publish_approved_followups( @@ -702,14 +1270,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 +1331,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 +1347,35 @@ 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, _publications = _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 or reused tracker. + 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 +1397,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}.", @@ -814,19 +1410,17 @@ 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) - lines.extend( - [ - "", - ( - f"Reconciliation: {len(reconciliation.selected_groups)} filed, " - f"{reconciliation.deduplicated_count} deduplicated, " - f"{reconciliation.skipped_by_cap} skipped by cap." - ), - ] - ) + 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(":"): + 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]) else: lines.extend( [ @@ -881,7 +1475,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 +1495,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 +1532,25 @@ 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, _issue_urls, publications = _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 = [ + 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 @@ -931,6 +1561,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..75771a4 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, @@ -339,19 +340,18 @@ _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, _publish_plan_approved_followups, _normalize_followup_key, _publish_approved_followups, + FollowupSourceContext, ) from .round_state import ( PostedRoundMetadata, @@ -5777,7 +5777,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 +6988,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 +7027,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 +7038,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 +7538,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 +8564,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 +8635,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 +8675,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 +8725,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 +8761,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 +8829,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 +8900,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..3eb8df0 --- /dev/null +++ b/src/coding_review_agent_loop/semantic_dedupe.py @@ -0,0 +1,359 @@ +"""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") + + 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.", + ] + + # 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)}", + ) + ) + + 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 | 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( + prompt: str, + runner: "Runner", + config: "AgentLoopConfig", + timeout_seconds: float, +) -> SemanticProviderResult: + backend = config.semantic_followup_backend + model = config.semantic_followup_model.strip() + isolated_dir: Path | None = None + try: + isolated_config, isolated_dir = _isolated_provider_config(config, backend, model) + 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 + + if isolated_dir is not None: + 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 + 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) + 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), + 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), + ) + allowed = {candidate.identity for candidate in candidates} + 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): + """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..c4a2a81 --- /dev/null +++ b/tests/test_followups.py @@ -0,0 +1,366 @@ +"""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 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 ( + 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: + 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 == [] + + +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