From f5fc22e70d0e94baa4d33c2a0fd3b6d2aad43ea8 Mon Sep 17 00:00:00 2001 From: jonahgabriel Date: Thu, 27 Aug 2026 21:12:21 -0400 Subject: [PATCH 1/2] fix(OMN-16808): the autoclose sweep must not repeat itself on a ticket it has already commented on The sweep enumerated OCC companions by a bare `now - lookback_hours` window with no cursor or watermark, kept `seen_tickets` as a per-run local, and let all three gap paths call `create_comment` unconditionally under `apply=True` through a `commentCreate` mutation that never read what was already on the ticket. Under the shipped schedule (`*/30` cron, 2h lookback) one merged companion sits inside four consecutive windows, so a gapped ticket accrues an identical comment per run, forever. Proven, not inferred: OMN-16037 / OMN-16373 / OMN-16757 / OMN-16759 were classified gap in BOTH run 33098307405 and run 33128661860. Dry-run masked it entirely (`linear_comment_posted: false` everywhere), which is why arming `--apply` was held. Fix - read-before-write, fail closed: * Every sweep gap comment ends with a stable marker keyed on (gap class, verdict fingerprint). Markdown hides it in the UI; the raw body is exact-matchable. State lives on the projection (the ticket), not in the job. * All three gap classes now funnel through one `_emit_gap_comment` write path, so the rule cannot hold on two paths and be forgotten on the third - which is exactly how the defect existed. * The key deliberately excludes the companion PR: the same verdict re-derived from a later companion is the same statement, so each (ticket, gap class) carries at most one live comment. A CHANGED verdict (3/6 -> 5/6, GAP_POSTED -> GAP_NO_BEHAVIOR_PROOF) fingerprints differently and IS posted. * An unreadable comment history is ERROR_LINEAR_API and writes nothing. "I could not check" must never resolve to "so I will post". The paginated read fails closed on an API error, an uninterpretable payload, a missing cursor, and a history longer than the page cap. * The dedup read runs in DRY-RUN too. It is a read, so DRY-RUN stays zero-write, and the report becomes an honest preview of what --apply would actually post rather than promising a comment the write path would suppress. * The FLIPPED audit comment is deliberately ungated: it is reached only after `issueUpdate` has moved the ticket to a completed state, which the next run short-circuits at SKIPPED_ALREADY_DONE. The flip path is self-limiting by its own terminal state; the gap paths have none, which is why they need the gate. Also OMN-16792 (in scope, one line): lookback 2h -> 6h. GitHub drops scheduled ticks under load rather than queueing them - 194 of 238 expected runs over 2026-08-22..27 (81%), with createdAt->startedAt 0s median AND 0s max across all 194, so nothing was slow; the missing runs were never created. Observed inter-run gap p100 was 236 min, larger than the 2h lookback, so a companion merged in the uncovered window was scanned by no run, ever. Widening was previously unsafe because more overlap meant more duplicate comments; with the dedup above, re-scanning a companion costs nothing observable. Residual recorded on the workflow and on OMN-16792: `max_companions` is capped at 50 and a 10h window hit it (run 33128661860), so the cap - not the lookback - bounds how wide the window can usefully go. Tests: tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py runs the same fixture window twice against one stateful fake Linear and asserts exactly one comment per (ticket, gap class, verdict), across all three gap classes, plus the changed-verdict, cross-companion, unreadable-history and dry-run directions. The two existing doubles now serve their own write log back through `fetch_comment_bodies` - a fake that forgets cannot see this defect. Refs OMN-16808, OMN-16792, OMN-16106. --- .../workflows/evidence-autoclose-sweep.yml | 37 +- .../contract.yaml | 12 + .../handler_evidence_autoclose_sweep.py | 381 ++++++++++++----- .../enum_evidence_autoclose_decision.py | 11 + .../test_handler_evidence_autoclose_sweep.py | 8 + .../test_omn_15911_behavior_proof_gate.py | 8 + .../test_omn_16808_comment_idempotency.py | 382 ++++++++++++++++++ 7 files changed, 728 insertions(+), 111 deletions(-) create mode 100644 tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py diff --git a/.github/workflows/evidence-autoclose-sweep.yml b/.github/workflows/evidence-autoclose-sweep.yml index 4b7de4f160..cd9f168103 100644 --- a/.github/workflows/evidence-autoclose-sweep.yml +++ b/.github/workflows/evidence-autoclose-sweep.yml @@ -67,7 +67,40 @@ on: lookback_hours: description: 'Scan companions merged within this many hours of now.' required: false - default: '2' + # 6h, raised from 2h (OMN-16792, landed with OMN-16808). + # + # `schedule` is best-effort: GitHub drops ticks under load rather than + # queueing them. Measured over 2026-08-22T20:03Z -> 2026-08-27T19:05Z, + # 194 scheduled runs fired against 238 expected at `*/30` (81%), and + # the discriminating measurement is createdAt->startedAt = 0s median + # AND 0s max across all 194: nothing was slow, the missing runs were + # never created. Observed inter-run gaps: median 30 min, p90 43 min, + # p100 236 min (3h56m). + # + # A 2h lookback is smaller than that p100 gap, so a companion merged + # inside the uncovered window was scanned by NO run, ever — the sweep + # only looks backwards a fixed window and never revisits. 6h clears the + # observed p100 by ~1.5x, so a dropped tick is absorbed rather than + # lost. + # + # The reason this is a safe one-line change NOW and was not before: + # widening the window widens the OVERLAP between consecutive runs, and + # before OMN-16808 every overlapping run re-posted an identical gap + # comment on every gapped ticket it re-saw. The read-before-write dedup + # in the handler makes re-scanning a companion cost nothing observable, + # so the lookback can be sized for the worst measured gap instead of + # for comment noise. That is OMN-16792 option 1, chosen because it + # removes the data-loss risk without deciding anything architectural; + # options 2 (event trigger) and 3 (runtime scheduler) stay open on that + # ticket. + # + # Residual, recorded not fixed here: `max_companions` is capped at 50 + # per run. A 10h window hit that cap on 2026-08-28 (run 33128661860, + # companions_scanned=50), so a window wide enough to truncate would + # reintroduce the same loss by a different door. At the observed OCC + # merge rate (~50 per 10h) a 6h window lands around 30 companions, + # under the cap. Sizing the cap belongs to OMN-16792. + default: '6' diagnose_tickets: description: >- OMN-16788 diagnostic. Comma-separated ticket ids (e.g. @@ -481,7 +514,7 @@ jobs: exit 1 fi uv run onex skill evidence_autoclose_sweep \ - --lookback-hours "${{ github.event.inputs.lookback_hours || '2' }}" + --lookback-hours "${{ github.event.inputs.lookback_hours || '6' }}" - name: Post summary if: always() diff --git a/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/contract.yaml b/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/contract.yaml index 1541fa0036..0a61496d31 100644 --- a/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/contract.yaml +++ b/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/contract.yaml @@ -139,6 +139,18 @@ error_handling: description: > The verdict carried no `result.terminal_payload.behavior_proving_count` — the verifier predates OMN-15911 and cannot say whether any green check proved behavior. Fails closed as ERROR_VERIFY_UNPARSEABLE rather than inferring a value: reading the absence as "zero behavior proof" would post a Linear comment stating a finding the run never made, and reading it as satisfied would flip on exactly the arithmetic the guard exists to stop. + recoverable: true + retry_strategy: "none" + - name: "DUPLICATE_SWEEP_COMMENT" + description: > + The sweep reached a gap verdict it has ALREADY posted on this ticket and did not repeat it (OMN-16808). Enumeration is a bare `now - lookback_hours` window with no cursor, so one merged companion sits inside several consecutive scheduled windows; before this guard every one of them posted an identical comment. Every sweep comment now carries a stable marker keyed on (gap class, verdict fingerprint) — NOT on the companion PR, because the same verdict re-derived from a later companion is the same statement — and the ticket's existing comments are read before any write. Recorded as SKIPPED_DUPLICATE_COMMENT and counted under tickets_skipped: the gap is real and still open, but this run added no information, and counting it under tickets_gap_posted would report comments that were never written. A CHANGED verdict has a different fingerprint and IS posted. + + recoverable: false + retry_strategy: "none" + - name: "COMMENT_HISTORY_UNREADABLE" + description: > + The ticket's existing comments could not be read (Linear API failure, an uninterpretable payload, or a history longer than the paginated read cap), so the sweep cannot establish whether it has already made this statement. Fails closed as ERROR_LINEAR_API and writes nothing (OMN-16808): "I could not check" must never resolve to "so I will post". The flip is withheld on this path regardless — it is only ever reached from a gap classification. + recoverable: true retry_strategy: "none" - name: "GITHUB_FILES_PAYLOAD_UNREADABLE" diff --git a/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/handlers/handler_evidence_autoclose_sweep.py b/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/handlers/handler_evidence_autoclose_sweep.py index 0cccd8189f..1f7ead95b7 100644 --- a/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/handlers/handler_evidence_autoclose_sweep.py +++ b/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/handlers/handler_evidence_autoclose_sweep.py @@ -48,9 +48,23 @@ GAP_NO_BEHAVIOR_PROOF. A verdict carrying no such field at all (an omnimarket predating the change — i.e. every historical receipt) is an ERROR, not an inference. -8. ``apply=False`` (the default) performs every read above but never calls +8. COMMENT IDEMPOTENCY (OMN-16808). Enumeration is a bare + ``now - lookback_hours`` window with no cursor or watermark, and + ``seen_tickets`` is a per-run local, so one merged companion sits inside + several consecutive scheduled windows (``*/30`` cron against a multi-hour + lookback). Before ANY gap comment the ticket's existing comments are read + and matched against a stable marker embedded in every comment the sweep + writes; an equivalent statement already on the ticket is recorded as + SKIPPED_DUPLICATE_COMMENT and not repeated. Fail-closed: a comment history + that cannot be read is ERROR_LINEAR_API and writes nothing, because "I + could not check" must never resolve to "so I will post". The FLIPPED path + needs no such guard — it terminates in a completed state that + SKIPPED_ALREADY_DONE short-circuits on the next run. +9. ``apply=False`` (the default) performs every read above but never calls a Linear mutation — every decision is logged as "would-do". ``apply=True`` - performs the real ``issueUpdate``/``commentCreate`` mutation. + performs the real ``issueUpdate``/``commentCreate`` mutation. The dedup + read runs in BOTH modes: it is a read, so DRY-RUN stays zero-write, and a + DRY-RUN report is then an honest preview of what ``--apply`` would post. Non-blocking Design -------------------- @@ -62,12 +76,14 @@ from __future__ import annotations import asyncio +import hashlib import json import logging import os import re from collections.abc import Awaitable, Callable from datetime import UTC, datetime, timedelta +from typing import Literal from uuid import uuid4 import httpx @@ -174,6 +190,28 @@ } """ +# OMN-16808. The read half of read-before-write: what has this sweep already +# said on this ticket? Paginated, because a partial history is indistinguishable +# from "no marker present" and would resolve straight into a duplicate post. +_ISSUE_COMMENTS_QUERY = """ +query IssueComments($id: String!, $after: String) { + issue(id: $id) { + id + comments(first: 100, after: $after) { + pageInfo { hasNextPage endCursor } + nodes { id body } + } + } +} +""" + +# Page cap for the comment-history read. Exhausting it WITHOUT reaching the end +# of the connection is an unreadable history, not an empty one — the caller +# fails closed. 5 x 100 comfortably exceeds any real ticket's comment count; +# a ticket that somehow exceeds it stops receiving sweep comments rather than +# receiving duplicates, which is the correct direction to be wrong in. +_MAX_COMMENT_PAGES = 5 + async def _reap_timed_out_process(proc: asyncio.subprocess.Process) -> None: """Kill and reap a subprocess whose ``communicate()`` await timed out. @@ -384,6 +422,41 @@ def _ac_coverage_gap( return "", () +# -- comment idempotency marker (OMN-16808) -------------------------------- +# +# Every comment the sweep writes ends with an HTML-comment marker. Linear +# renders markdown, so the marker is invisible in the UI and exact-matchable in +# the raw body — the sweep's own durable record that it has already made this +# statement on this ticket, carried by the projection (the ticket) rather than +# by the job. That is the same shape as every other piece of truth here: state +# is read back from the surface it was written to, not held in the runner. +# +# The key is (gap class, verdict fingerprint) — NOT the companion PR. Two +# facts drive that choice: +# * the same verdict re-derived from a later companion is the SAME statement, +# and keying on the companion would let every new OCC merge re-open the +# same gap comment; +# * a CHANGED verdict (3/6 -> 5/6, or GAP_POSTED -> GAP_NO_BEHAVIOR_PROOF) is +# new information, gets a different fingerprint, and is posted. +# So each (ticket, gap class) carries at most one live statement, refreshed +# when the underlying verdict moves and silent when it does not. +_SWEEP_COMMENT_MARKER_PREFIX = "onex-autoclose-sweep" +_SWEEP_COMMENT_MARKER_VERSION = "v1" + + +def _sweep_comment_marker( + decision: EnumEvidenceAutocloseDecision, fingerprint_parts: tuple[str, ...] +) -> str: + """Build the stable per-(ticket, gap class, verdict) comment marker.""" + digest = hashlib.sha256("\x1f".join(fingerprint_parts).encode("utf-8")).hexdigest()[ + :16 + ] + return ( + f"" + ) + + def _format_uncovered(uncovered: tuple[str, ...]) -> str: """Render the uncovered criteria as a bounded markdown list.""" shown = uncovered[:_MAX_UNCOVERED_LISTED] @@ -478,6 +551,51 @@ async def update_issue_state(self, issue_id: str, state_id: str) -> bool: update = data.get("issueUpdate") return bool(isinstance(update, dict) and update.get("success")) + async def fetch_comment_bodies(self, issue_id: str) -> tuple[str, ...] | None: + """Every existing comment body on an issue, or None if unreadable. + + None means "could not determine", NEVER "there are none" (OMN-16808). + The caller uses this to decide whether it has already said something on + this ticket, so an empty tuple and a failed read must not collapse into + the same value — one releases a write, the other must block it. + + Fails closed on: an API/GraphQL failure, a payload shape this code + cannot interpret, a cursor the connection did not supply, and a history + longer than ``_MAX_COMMENT_PAGES`` pages (an unread tail could hold the + marker). + """ + bodies: list[str] = [] + cursor: str | None = None + for _ in range(_MAX_COMMENT_PAGES): + data = await self._query( + _ISSUE_COMMENTS_QUERY, {"id": issue_id, "after": cursor} + ) + if data is None: + return None + issue = data.get("issue") + if not isinstance(issue, dict): + return None + connection = issue.get("comments") + if not isinstance(connection, dict): + return None + nodes = connection.get("nodes") + if not isinstance(nodes, list): + return None + for node in nodes: + if isinstance(node, dict): + body = node.get("body") + if isinstance(body, str): + bodies.append(body) + page_info = connection.get("pageInfo") + page_info = page_info if isinstance(page_info, dict) else {} + if not page_info.get("hasNextPage"): + return tuple(bodies) + end_cursor = page_info.get("endCursor") + if not isinstance(end_cursor, str) or not end_cursor: + return None + cursor = end_cursor + return None + async def create_comment(self, issue_id: str, body: str) -> bool: """Post a comment on an issue. False on any failure.""" data = await self._query( @@ -826,6 +944,10 @@ async def handle( EnumEvidenceAutocloseDecision.SKIPPED_ALREADY_DONE, EnumEvidenceAutocloseDecision.SKIPPED_NO_BINDING, EnumEvidenceAutocloseDecision.SKIPPED_AMBIGUOUS_BINDING, + # OMN-16808: the gap is real and still open, but this run said + # nothing new. A skip, not a gap post — counting it under + # `gap_posted` would report comments that were never written. + EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT, ) ) errored = sum( @@ -852,6 +974,98 @@ async def handle( outcomes=tuple(outcomes), ) + # -- comment idempotency funnel (OMN-16808) -------------------------- + + async def _dedup_gate( + self, *, issue_id: str, marker: str + ) -> tuple[Literal["post", "duplicate", "unreadable"], str]: + """Decide whether this exact statement may be written to this ticket. + + Returns ``(gate, detail)``. ``unreadable`` is the fail-closed verdict: + the sweep could not establish whether it has already commented, so it + must not comment. Never returns ``post`` on a read it could not make. + """ + if not issue_id: + return "unreadable", "Linear issue payload carried no issue id" + bodies = await self._linear.fetch_comment_bodies(issue_id) + if bodies is None: + return ( + "unreadable", + "the ticket's existing comments could not be read from Linear", + ) + if any(marker in body for body in bodies): + return "duplicate", marker + return "post", "" + + async def _emit_gap_comment( + self, + *, + base: ModelEvidenceAutocloseOutcome, + apply: bool, + issue_id: str, + marker: str, + comment_body: str, + ) -> ModelEvidenceAutocloseOutcome: + """Single write path for every gap comment (OMN-16808). + + All three gap classes funnel through here so the read-before-write rule + cannot be satisfied on two paths and forgotten on the third — which is + exactly how the defect existed: three call sites, three unconditional + ``create_comment`` calls, no shared gate. + + The dedup read runs in DRY-RUN too. It is a read, so DRY-RUN stays + zero-write, and the report then previews what ``--apply`` would + actually do rather than promising a comment the write path would + suppress. + """ + gate, detail = await self._dedup_gate(issue_id=issue_id, marker=marker) + if gate == "unreadable": + return base.model_copy( + update={ + "decision": EnumEvidenceAutocloseDecision.ERROR_LINEAR_API, + "reason": ( + f"Refusing to comment — {detail}, so the sweep cannot " + "establish that it has not already said this here " + "(OMN-16808). Fails closed rather than risk a duplicate. " + f"Flip withheld regardless: {base.reason}" + ), + } + ) + if gate == "duplicate": + return base.model_copy( + update={ + "decision": EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT, + "reason": ( + "This exact verdict is already posted on the ticket " + f"({marker}) — not repeating it. The gap itself is " + f"unchanged and still open: {base.reason}" + ), + } + ) + if not apply: + logger.info( + "[DRY-RUN] Would post %s comment on %s (%s)", + base.decision.value, + base.ticket_id, + base.reason, + ) + return base.model_copy(update={"reason": f"[DRY-RUN] {base.reason}"}) + + commented = await self._linear.create_comment( + issue_id, f"{comment_body}\n\n{marker}" + ) + if not commented: + return base.model_copy( + update={ + "decision": EnumEvidenceAutocloseDecision.ERROR_LINEAR_API, + "reason": ( + "commentCreate mutation failed while posting the gap: " + f"{base.reason}" + ), + } + ) + return base.model_copy(update={"linear_comment_posted": True, "applied": True}) + async def _behavior_proof_outcome( self, *, @@ -890,30 +1104,21 @@ async def _behavior_proof_outcome( dod_verify_failed_count=failed_count, dod_verify_behavior_proving_count=0, ) - if not apply: - logger.info( - "[DRY-RUN] Would withhold the Done flip on %s (%s)", ticket_id, reason - ) - return base.model_copy(update={"reason": f"[DRY-RUN] {reason}"}) - - if not issue_id: - return ModelEvidenceAutocloseOutcome( - ticket_id=ticket_id, - companion_pr_number=companion_pr_number, - companion_pr_url=companion_pr_url, - decision=EnumEvidenceAutocloseDecision.ERROR_LINEAR_API, - reason=( - "Linear issue payload missing id — refusing to comment. " - f"Flip withheld regardless: {reason}" - ), - dod_verify_total_checks=total_checks, - dod_verify_verified_count=verified_count, - dod_verify_failed_count=failed_count, - ) - - commented = await self._linear.create_comment( - issue_id, + marker = _sweep_comment_marker( + EnumEvidenceAutocloseDecision.GAP_NO_BEHAVIOR_PROOF, ( + str(total_checks), + str(verified_count), + str(failed_count), + "behavior=0", + ), + ) + return await self._emit_gap_comment( + base=base, + apply=apply, + issue_id=issue_id, + marker=marker, + comment_body=( "Proof-class gap (OMN-16106 evidence autoclose sweep) — NOT " "flipped.\n\n" f"Merged evidence companion: {companion_pr_url}\n" @@ -925,9 +1130,6 @@ async def _behavior_proof_outcome( "is already established by the companion." ), ) - return base.model_copy( - update={"linear_comment_posted": commented, "applied": True} - ) async def _ac_coverage_outcome( self, @@ -961,36 +1163,25 @@ async def _ac_coverage_outcome( dod_verify_behavior_proving_count=behavior_proving_count, uncovered_acceptance_criteria=uncovered, ) - if not apply: - logger.info( - "[DRY-RUN] Would withhold the Done flip on %s (%s)", - ticket_id, - ac_gap_reason, - ) - return base.model_copy(update={"reason": f"[DRY-RUN] {ac_gap_reason}"}) - - if not issue_id: - # A missing issue id cannot be commented on -- but the flip is - # already withheld, so this stays a Linear-API error rather than - # silently degrading into "no gap found". - return ModelEvidenceAutocloseOutcome( - ticket_id=ticket_id, - companion_pr_number=companion_pr_number, - companion_pr_url=companion_pr_url, - decision=EnumEvidenceAutocloseDecision.ERROR_LINEAR_API, - reason=( - "Linear issue payload missing id — refusing to comment. " - f"Flip withheld regardless: {ac_gap_reason}" - ), - dod_verify_total_checks=total_checks, - dod_verify_verified_count=verified_count, - dod_verify_failed_count=failed_count, - uncovered_acceptance_criteria=uncovered, - ) - - commented = await self._linear.create_comment( - issue_id, + # The uncovered criteria are part of the statement, so they are part of + # the fingerprint: a description whose unchecked boxes changed is a + # different gap and does get a fresh comment. + marker = _sweep_comment_marker( + EnumEvidenceAutocloseDecision.GAP_AC_COVERAGE, ( + str(total_checks), + str(verified_count), + str(failed_count), + f"behavior={behavior_proving_count}", + *uncovered, + ), + ) + return await self._emit_gap_comment( + base=base, + apply=apply, + issue_id=issue_id, + marker=marker, + comment_body=( "AC-coverage gap (OMN-16106 evidence autoclose sweep) — NOT flipped.\n\n" f"Merged evidence companion: {companion_pr_url}\n" f"dod_verify: {verified_count}/{total_checks} ACs verified, " @@ -1004,9 +1195,6 @@ async def _ac_coverage_outcome( "done) — the sweep will flip it on the next run." ), ) - return base.model_copy( - update={"linear_comment_posted": commented, "applied": True} - ) async def _process_ticket( self, @@ -1237,6 +1425,13 @@ async def _process_ticket( dod_verify_failed_count=failed_count, dod_verify_behavior_proving_count=behavior_proving_count, ) + # No dedup gate on the flip audit comment (OMN-16808), and that is + # deliberate: this line is reached only after `issueUpdate` has + # already moved the ticket into a completed state, which the next + # run short-circuits at SKIPPED_ALREADY_DONE before any verdict is + # reached. The flip path is self-limiting by its own terminal + # state; the gap paths have no such terminal state, which is why + # they need the read-before-write gate and this does not. commented = await self._linear.create_comment( issue_id, ( @@ -1268,53 +1463,7 @@ async def _process_ticket( f"dod_verify: {verified_count}/{total_checks} ACs verified, " f"{failed_count} failed — not all ACs are receipt-proven." ) - if not request.apply: - logger.info( - "[DRY-RUN] Would post gap comment on %s (%s)", ticket_id, gap_reason - ) - return ModelEvidenceAutocloseOutcome( - ticket_id=ticket_id, - companion_pr_number=companion_pr_number, - companion_pr_url=companion_pr_url, - decision=EnumEvidenceAutocloseDecision.GAP_POSTED, - reason=f"[DRY-RUN] {gap_reason}", - dod_verify_total_checks=total_checks, - dod_verify_verified_count=verified_count, - dod_verify_failed_count=failed_count, - applied=False, - ) - if not issue_id: - return ModelEvidenceAutocloseOutcome( - ticket_id=ticket_id, - companion_pr_number=companion_pr_number, - companion_pr_url=companion_pr_url, - decision=EnumEvidenceAutocloseDecision.ERROR_LINEAR_API, - reason="Linear issue payload missing id — refusing to comment.", - dod_verify_total_checks=total_checks, - dod_verify_verified_count=verified_count, - dod_verify_failed_count=failed_count, - ) - commented = await self._linear.create_comment( - issue_id, - ( - "Evidence gap (OMN-16106 evidence autoclose sweep) — NOT flipped.\n\n" - f"Merged evidence companion: {companion_pr_url}\n" - f"dod_verify: {verified_count}/{total_checks} ACs verified, " - f"{failed_count} failed." - ), - ) - if not commented: - return ModelEvidenceAutocloseOutcome( - ticket_id=ticket_id, - companion_pr_number=companion_pr_number, - companion_pr_url=companion_pr_url, - decision=EnumEvidenceAutocloseDecision.ERROR_LINEAR_API, - reason="commentCreate mutation failed while posting the gap.", - dod_verify_total_checks=total_checks, - dod_verify_verified_count=verified_count, - dod_verify_failed_count=failed_count, - ) - return ModelEvidenceAutocloseOutcome( + gap_base = ModelEvidenceAutocloseOutcome( ticket_id=ticket_id, companion_pr_number=companion_pr_number, companion_pr_url=companion_pr_url, @@ -1323,6 +1472,20 @@ async def _process_ticket( dod_verify_total_checks=total_checks, dod_verify_verified_count=verified_count, dod_verify_failed_count=failed_count, - linear_comment_posted=True, - applied=True, + ) + marker = _sweep_comment_marker( + EnumEvidenceAutocloseDecision.GAP_POSTED, + (str(total_checks), str(verified_count), str(failed_count)), + ) + return await self._emit_gap_comment( + base=gap_base, + apply=request.apply, + issue_id=issue_id, + marker=marker, + comment_body=( + "Evidence gap (OMN-16106 evidence autoclose sweep) — NOT flipped.\n\n" + f"Merged evidence companion: {companion_pr_url}\n" + f"dod_verify: {verified_count}/{total_checks} ACs verified, " + f"{failed_count} failed." + ), ) diff --git a/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/models/enum_evidence_autoclose_decision.py b/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/models/enum_evidence_autoclose_decision.py index 4247c8fe3f..fa9eb055eb 100644 --- a/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/models/enum_evidence_autoclose_decision.py +++ b/src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/models/enum_evidence_autoclose_decision.py @@ -44,6 +44,17 @@ class EnumEvidenceAutocloseDecision(StrEnum): SKIPPED_NO_BINDING = "skipped_no_binding" # More than one distinct ticket id bound to the same merged companion. SKIPPED_AMBIGUOUS_BINDING = "skipped_ambiguous_binding" + # The sweep reached the same gap verdict it has ALREADY posted on this + # ticket, and did not repeat itself (OMN-16808). Enumeration is a bare + # `now - lookback_hours` window with no cursor, so one merged companion sits + # inside several consecutive scheduled windows; without a read-before-write + # check every one of them posts an identical comment. Counted as a skip, not + # a gap: the gap is real and still open, but this run added no information. + # + # Keyed on (ticket, gap class, verdict fingerprint) and NOT on the companion + # PR — the same verdict re-derived from a later companion is the same + # statement. A CHANGED verdict has a different fingerprint and does comment. + SKIPPED_DUPLICATE_COMMENT = "skipped_duplicate_comment" # `uv run onex skill dod_verify ` exited non-zero (dispatch/runtime # failure, not a normal verified/failed verdict) -> fail closed, never flip. ERROR_VERIFY_NONZERO_EXIT = "error_verify_nonzero_exit" diff --git a/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_handler_evidence_autoclose_sweep.py b/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_handler_evidence_autoclose_sweep.py index 594a9efe02..18bd7cb838 100644 --- a/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_handler_evidence_autoclose_sweep.py +++ b/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_handler_evidence_autoclose_sweep.py @@ -138,6 +138,14 @@ async def create_comment(self, issue_id: str, body: str) -> bool: self.comments.append((issue_id, body)) return True + async def fetch_comment_bodies(self, issue_id: str) -> tuple[str, ...] | None: + # OMN-16808: the sweep reads what it has already said on a ticket + # before it says anything else. This double serves its own write log + # back, so a second run over the same window is distinguishable from a + # first — a fake that always returned () would let the duplicate-post + # defect pass unnoticed here. + return tuple(body for target, body in self.comments if target == issue_id) + def _issue( *, diff --git a/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_15911_behavior_proof_gate.py b/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_15911_behavior_proof_gate.py index e2c0d61915..cd24592cca 100644 --- a/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_15911_behavior_proof_gate.py +++ b/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_15911_behavior_proof_gate.py @@ -135,6 +135,14 @@ async def create_comment(self, issue_id: str, body: str) -> bool: self.comments.append((issue_id, body)) return True + async def fetch_comment_bodies(self, issue_id: str) -> tuple[str, ...] | None: + # OMN-16808: the sweep reads what it has already said on a ticket + # before it says anything else. This double serves its own write log + # back, so a second run over the same window is distinguishable from a + # first — a fake that always returned () would let the duplicate-post + # defect pass unnoticed here. + return tuple(body for target, body in self.comments if target == issue_id) + def _handler( skill_result: dict[str, object], linear: _FakeLinear diff --git a/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py b/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py new file mode 100644 index 0000000000..625309037a --- /dev/null +++ b/tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py @@ -0,0 +1,382 @@ +# SPDX-FileCopyrightText: 2025 OmniNode.ai Inc. +# SPDX-License-Identifier: MIT +"""OMN-16808 — the sweep must not say the same thing twice on the same ticket. + +The autoclose sweep enumerates OCC companions by a bare ``now - lookback_hours`` +window and keeps no cross-run state: ``seen_tickets`` is a local discarded when +``handle()`` returns, and every gap path called ``create_comment`` +unconditionally under ``apply=True`` through a bare ``commentCreate`` mutation +that never read what was already on the ticket. + +Under the shipped schedule (``cron: '*/30'``, lookback 2h) one merged companion +sits inside four consecutive windows, so a ticket that stays gapped accrues an +identical comment per run, forever. Proven empirically before this fix: OMN-16037 +/ OMN-16373 / OMN-16757 / OMN-16759 were classified gap in BOTH run 33098307405 +and run 33128661860. + +This suite pins the read-before-write rule and its fail-closed direction: + +* the same window scanned twice writes exactly ONE comment per (ticket, gap class, + verdict) — the second run records SKIPPED_DUPLICATE_COMMENT; +* dedup is keyed on ticket + gap class + verdict fingerprint, NOT on the companion + PR, so a second companion binding the same ticket with the same verdict is also + suppressed; +* a CHANGED verdict is new information and still gets a comment; +* an unreadable comment history is an ERROR outcome and writes nothing — the sweep + never guesses that it has not already commented; +* DRY-RUN stays zero-write on every one of those paths. +""" + +from __future__ import annotations + +from datetime import UTC, datetime, timedelta +from uuid import uuid4 + +import pytest + +from omnibase_infra.nodes.node_evidence_autoclose_sweep_effect.handlers.handler_evidence_autoclose_sweep import ( + HandlerEvidenceAutocloseSweep, +) +from omnibase_infra.nodes.node_evidence_autoclose_sweep_effect.models.enum_evidence_autoclose_decision import ( + EnumEvidenceAutocloseDecision, +) +from omnibase_infra.nodes.node_evidence_autoclose_sweep_effect.models.model_evidence_autoclose_sweep_request import ( + ModelEvidenceAutocloseSweepRequest, +) + +pytestmark = pytest.mark.unit + +_OCC_REPO = "OmniNode-ai/onex_change_control" +_TICKET = "OMN-9999" +_ISSUE_ID = "issue-uuid-1" + + +def _merged_pr(number: int) -> dict[str, object]: + recent = (datetime.now(tz=UTC) - timedelta(hours=1)).strftime("%Y-%m-%dT%H:%M:%SZ") + return { + "number": number, + "html_url": f"https://github.com/{_OCC_REPO}/pull/{number}", + "title": f"evidence({_TICKET}): OCC companion", + "updated_at": recent, + "merged_at": recent, + } + + +def _skill_result( + *, + total: int, + verified: int, + failed: int, + behavior_proving: int = 1, +) -> dict[str, object]: + """A ModelSkillResult shaped like ``onex skill dod_verify`` prints.""" + return { + "skill_name": "dod_verify", + "node_name": "node_dod_verify", + "status": "success" if failed == 0 else "failed", + "exit_code": 0 if failed == 0 else 1, + "result": { + "workflow_result": "completed", + "exit_code": 0 if failed == 0 else 1, + "terminal_payload": { + "correlation_id": str(uuid4()), + "ticket_id": _TICKET, + "status": "verified" if failed == 0 else "failed", + "dry_run": False, + "checks": [], + "total_checks": total, + "verified_count": verified, + "failed_count": failed, + "skipped_count": 0, + "superseded_count": 0, + "behavior_proving_count": behavior_proving, + "error_message": None, + }, + }, + } + + +class _StatefulLinear: + """A Linear double that REMEMBERS what the sweep posted. + + The production defect is invisible to a double that forgets: the old fake + only appended to a write log and served no comment history back, so two + runs looked identical to one. This one serves ``create_comment`` writes + back through ``fetch_comment_bodies``, which is what makes a second run + distinguishable from a first. + """ + + def __init__( + self, + *, + description: str | None = None, + comment_history_readable: bool = True, + ) -> None: + self.comments: list[tuple[str, str]] = [] + self.state_updates: list[tuple[str, str]] = [] + self.comment_history_readable = comment_history_readable + self.fetch_comment_calls: list[str] = [] + self._description = description + + async def fetch_issue(self, ticket_id: str) -> dict[str, object] | None: + return { + "id": _ISSUE_ID, + "identifier": _TICKET, + "state": {"id": "s1", "name": "In Progress", "type": "started"}, + "labels": {"nodes": []}, + "team": {"id": "team-1"}, + "description": self._description, + } + + async def fetch_done_state_id(self, team_id: str) -> str | None: + return "state-done-id" + + async def update_issue_state(self, issue_id: str, state_id: str) -> bool: + self.state_updates.append((issue_id, state_id)) + return True + + async def create_comment(self, issue_id: str, body: str) -> bool: + self.comments.append((issue_id, body)) + return True + + async def fetch_comment_bodies(self, issue_id: str) -> tuple[str, ...] | None: + self.fetch_comment_calls.append(issue_id) + if not self.comment_history_readable: + return None + return tuple(body for target, body in self.comments if target == issue_id) + + +def _gh_fake(companions: list[dict[str, object]]): + async def fake_run_gh_command(args: list[str], timeout: float): + path = args[2] + if "/files" in path: + return [{"filename": f"contracts/{_TICKET}.yaml"}], "" + if "page=1" in path: + return companions, "" + return [], "" + + return fake_run_gh_command + + +def _handler( + skill_results: list[dict[str, object]], + linear: _StatefulLinear, + companions: list[dict[str, object]], +) -> HandlerEvidenceAutocloseSweep: + """Handler whose dod_verify double serves ``skill_results`` in order. + + The last entry is reused once exhausted, so a single-verdict test passes a + one-element list and both runs see the same verdict. + """ + calls = {"n": 0} + + async def fake_dod_verify(ticket_id: str, cwd: str, timeout: float): + index = min(calls["n"], len(skill_results) - 1) + calls["n"] += 1 + return skill_results[index], 0, "" + + return HandlerEvidenceAutocloseSweep( + linear_client=linear, # type: ignore[arg-type] + autoclose_disabled=False, + run_gh_command=_gh_fake(companions), + run_dod_verify_command=fake_dod_verify, + ) + + +def _request(*, apply: bool) -> ModelEvidenceAutocloseSweepRequest: + return ModelEvidenceAutocloseSweepRequest( + correlation_id=uuid4(), + occ_repo=_OCC_REPO, + lookback_hours=2, + apply=apply, + ) + + +@pytest.mark.asyncio +class TestOmn16808CommentIdempotency: + async def test_same_window_twice_posts_exactly_one_gap_comment(self) -> None: + """The headline AC: one fixture window, two runs, one comment.""" + linear = _StatefulLinear() + companions = [_merged_pr(7001)] + handler = _handler( + [_skill_result(total=6, verified=3, failed=3)], linear, companions + ) + + first = await handler.handle(_request(apply=True)) + second = await handler.handle(_request(apply=True)) + + assert first.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_POSTED + assert first.outcomes[0].linear_comment_posted is True + assert ( + second.outcomes[0].decision + == EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT + ) + assert second.outcomes[0].linear_comment_posted is False + assert second.outcomes[0].applied is False + assert len(linear.comments) == 1, linear.comments + + async def test_posted_comment_carries_a_stable_marker(self) -> None: + linear = _StatefulLinear() + handler = _handler( + [_skill_result(total=6, verified=3, failed=3)], linear, [_merged_pr(7001)] + ) + + await handler.handle(_request(apply=True)) + + body = linear.comments[0][1] + assert "onex-autoclose-sweep" in body + assert "class=gap_posted" in body + assert "fingerprint=" in body + + async def test_dedup_holds_across_a_different_companion(self) -> None: + """A second companion binding the same ticket must not re-say it. + + ``seen_tickets`` already suppresses this WITHIN one run. The failure + mode is across runs: window N sees companion #7001, window N+1 sees + #7002 bound to the same ticket with the same verdict. The dedup key is + deliberately (ticket, gap class, verdict) and excludes the companion. + """ + linear = _StatefulLinear() + verdict = _skill_result(total=6, verified=3, failed=3) + + first = await _handler([verdict], linear, [_merged_pr(7001)]).handle( + _request(apply=True) + ) + second = await _handler([verdict], linear, [_merged_pr(7002)]).handle( + _request(apply=True) + ) + + assert first.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_POSTED + assert ( + second.outcomes[0].decision + == EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT + ) + assert len(linear.comments) == 1, linear.comments + + async def test_changed_verdict_still_gets_a_comment(self) -> None: + """3/6 -> 5/6 is new information and must not be swallowed.""" + linear = _StatefulLinear() + handler = _handler( + [ + _skill_result(total=6, verified=3, failed=3), + _skill_result(total=6, verified=5, failed=1), + ], + linear, + [_merged_pr(7001)], + ) + + first = await handler.handle(_request(apply=True)) + second = await handler.handle(_request(apply=True)) + + assert first.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_POSTED + assert second.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_POSTED + assert len(linear.comments) == 2, linear.comments + + async def test_unreadable_comment_history_fails_closed(self) -> None: + """Cannot prove it has not commented => must not comment.""" + linear = _StatefulLinear(comment_history_readable=False) + handler = _handler( + [_skill_result(total=6, verified=3, failed=3)], linear, [_merged_pr(7001)] + ) + + result = await handler.handle(_request(apply=True)) + + assert ( + result.outcomes[0].decision + == EnumEvidenceAutocloseDecision.ERROR_LINEAR_API + ) + assert result.tickets_errored == 1 + assert linear.comments == [] + + async def test_behavior_proof_gap_dedupes(self) -> None: + linear = _StatefulLinear() + handler = _handler( + [_skill_result(total=6, verified=6, failed=0, behavior_proving=0)], + linear, + [_merged_pr(7001)], + ) + + first = await handler.handle(_request(apply=True)) + second = await handler.handle(_request(apply=True)) + + assert ( + first.outcomes[0].decision + == EnumEvidenceAutocloseDecision.GAP_NO_BEHAVIOR_PROOF + ) + assert ( + second.outcomes[0].decision + == EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT + ) + assert len(linear.comments) == 1 + assert "class=gap_no_behavior_proof" in linear.comments[0][1] + + async def test_ac_coverage_gap_dedupes(self) -> None: + linear = _StatefulLinear( + description="## Acceptance criteria\n\n- [ ] AC1 not done yet\n" + ) + handler = _handler( + [_skill_result(total=6, verified=6, failed=0, behavior_proving=2)], + linear, + [_merged_pr(7001)], + ) + + first = await handler.handle(_request(apply=True)) + second = await handler.handle(_request(apply=True)) + + assert ( + first.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_AC_COVERAGE + ) + assert ( + second.outcomes[0].decision + == EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT + ) + assert len(linear.comments) == 1 + assert "class=gap_ac_coverage" in linear.comments[0][1] + + async def test_dry_run_writes_nothing_on_any_dedup_path(self) -> None: + """DRY-RUN reads the history (honest preview) and writes nothing.""" + linear = _StatefulLinear() + handler = _handler( + [_skill_result(total=6, verified=3, failed=3)], linear, [_merged_pr(7001)] + ) + + first = await handler.handle(_request(apply=False)) + second = await handler.handle(_request(apply=False)) + + # Nothing was written, so nothing is a duplicate: both dry-runs report + # the gap they would post. + assert first.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_POSTED + assert second.outcomes[0].decision == EnumEvidenceAutocloseDecision.GAP_POSTED + assert linear.comments == [] + assert linear.state_updates == [] + + async def test_dry_run_previews_the_suppression_after_a_real_post(self) -> None: + """Once a marker exists, DRY-RUN reports the suppression, not a gap.""" + linear = _StatefulLinear() + verdict = _skill_result(total=6, verified=3, failed=3) + await _handler([verdict], linear, [_merged_pr(7001)]).handle( + _request(apply=True) + ) + + preview = await _handler([verdict], linear, [_merged_pr(7001)]).handle( + _request(apply=False) + ) + + assert ( + preview.outcomes[0].decision + == EnumEvidenceAutocloseDecision.SKIPPED_DUPLICATE_COMMENT + ) + assert len(linear.comments) == 1 + + async def test_duplicate_suppression_counts_as_skipped_not_gap(self) -> None: + linear = _StatefulLinear() + handler = _handler( + [_skill_result(total=6, verified=3, failed=3)], linear, [_merged_pr(7001)] + ) + + await handler.handle(_request(apply=True)) + second = await handler.handle(_request(apply=True)) + + assert second.tickets_skipped == 1 + assert second.tickets_gap_posted == 0 + assert second.tickets_errored == 0 From 96da7b96430a52942946f248d5a01add613fd274 Mon Sep 17 00:00:00 2001 From: jonahgabriel Date: Thu, 27 Aug 2026 22:16:28 -0400 Subject: [PATCH 2/2] fix(OMN-16808): bump release identity for autoclose sweep fix --- docker/runners/runner-image.lock.json | 4 ++-- pyproject.toml | 2 +- uv.lock | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docker/runners/runner-image.lock.json b/docker/runners/runner-image.lock.json index 029c4b2428..a0efe82920 100644 --- a/docker/runners/runner-image.lock.json +++ b/docker/runners/runner-image.lock.json @@ -1,12 +1,12 @@ { "base_image_digest": "sha256:3ba65aa20f86a0fad9df2b2c259c613df006b2e6d0bfcc8a146afb8c525a9751", "gh_version": "2.67.0", - "identity_digest": "47224bdefe93869e4dd05501c506eb18", + "identity_digest": "7fae4af5f1fb6695a6e032a366e12613", "image_version": 7, "kubectl_version": "1.32.1", "python_version": "3.12", "runner_version": "2.336.0", - "shared_env_digest": "ac02631ce31562210f83ee2f", + "shared_env_digest": "a7a173a77e4d15465404b3b3", "shared_env_install_args": "--frozen --all-extras --all-groups --no-install-project", "uv_version": "0.6.14" } diff --git a/pyproject.toml b/pyproject.toml index 0de3bd6a35..fa213c811a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "omnibase_infra" -version = "0.38.12" +version = "0.38.13" description = "ONEX Infrastructure - Service integration and database infrastructure tools" authors = [{ name = "OmniNode.ai", email = "contact@omninode.ai" }] license = {text = "MIT"} diff --git a/uv.lock b/uv.lock index 8c99080f1f..fd4bb6103a 100644 --- a/uv.lock +++ b/uv.lock @@ -2132,7 +2132,7 @@ wheels = [ [[package]] name = "omnibase-infra" -version = "0.38.12" +version = "0.38.13" source = { editable = "." } dependencies = [ { name = "a2a-sdk" },