Skip to content

fix(OMN-16808): the autoclose sweep must not repeat itself on a ticket it already commented on - #2954

Merged
github-actions[bot] merged 2 commits into
devfrom
jonah/omn-16808-sweep-comment-idempotency
Aug 28, 2026
Merged

github-actions[bot] merged 2 commits into
devfrom
jonah/omn-16808-sweep-comment-idempotency

Conversation

@jonahgabriel

@jonahgabriel jonahgabriel commented Aug 28, 2026 •

Copy link
Copy Markdown
Collaborator

OMN-16808 — the autoclose sweep has no cross-run comment idempotency

Blocks arming OMN-16106 to --apply. Also lands the in-scope half of OMN-16792.

The defect (read from merged origin/dev @ 35d97fc51)

node_evidence_autoclose_sweep_effect enumerates OCC companions by a bare
now - lookback_hours window and keeps no cross-run state:

  1. handle() computes since_iso from the clock — no cursor, no watermark, no
    "already processed" record.
  2. seen_tickets is a local declared inside handle(). It suppresses a second
    companion for the same ticket within one run and is discarded when the run
    ends.
  3. All three gap paths (_behavior_proof_outcome, _ac_coverage_outcome, the
    GAP_POSTED tail of _process_ticket) called self._linear.create_comment(...)
    whenever apply was true, through a bare commentCreate that never reads
    existing comments.

Under the shipped schedule (cron: '*/30', lookback_hours 2) a companion
merged at T sits inside four consecutive windows. Each posts an identical
comment on the bound ticket, forever while the ticket stays gapped.

Proven empirically, not inferred. Two DRY-RUN dispatches ~7h apart, both
lookback_hours=10: OMN-16037, OMN-16373, OMN-16757 and OMN-16759 were
classified gap in both run 33098307405 and run 33128661860. Under
--apply each would have taken two identical comments from those two runs
alone. Run 33128661860 classified 16 tickets as gap in one window. Dry-run
masked the whole thing (linear_comment_posted: false everywhere), which is
why it had never been observed live.

The fix — read-before-write, fail closed

  • A stable marker on every sweep gap comment, keyed on
    (gap class, verdict fingerprint). It is an HTML comment, so Linear's
    markdown renderer hides it and the raw body stays exact-matchable. The
    sweep's record that it has already made a statement lives on the projection
    (the ticket), not in the job.
  • One write path. All three gap classes now funnel through
    _emit_gap_comment, so the rule cannot hold on two paths and be forgotten on
    the third — which is precisely how the defect existed: three call sites,
    three unconditional writes, no shared gate.
  • The key excludes the companion PR, deliberately. The same verdict
    re-derived from a later companion is the same statement; keying on the
    companion would let every new OCC merge re-open the same gap comment. Each
    (ticket, gap class) therefore carries at most one live statement — refreshed
    when the verdict moves, silent when it does not.
  • A changed verdict still comments. 3/6 → 5/6, or GAP_POSTED →
    GAP_NO_BEHAVIOR_PROOF, fingerprints differently and is posted. For
    GAP_AC_COVERAGE the uncovered criteria are part of the fingerprint.
  • An unreadable comment history writes nothing and is recorded as
    ERROR_LINEAR_API. "I could not check" must never resolve to "so I will
    post". The paginated read returns None (not an empty tuple) on an API
    failure, an uninterpretable payload, a missing cursor, and a history longer
    than the page cap.
  • DRY-RUN stays zero-write. The dedup read runs in both modes because it is
    a read — which also makes the DRY-RUN report an honest preview of what
    --apply would post, instead of promising a comment the write path would
    suppress.
  • The FLIPPED audit comment is ungated on purpose. It is reached only after
    issueUpdate 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 no terminal state, which is why
    they need the gate and this does not.

New decision SKIPPED_DUPLICATE_COMMENT, counted under tickets_skipped — the
gap is real and still open, but the run added no information, and counting it
under tickets_gap_posted would report comments that were never written.

OMN-16792, in scope: lookback 2h → 6h

GitHub drops scheduled ticks under load rather than queueing them. Over
2026-08-22T20:03Z → 2026-08-27T19:05Z: 194 scheduled runs against 238 expected
at */30 (81%), with 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. A 2h lookback is
smaller than that p100, so a companion merged in the uncovered window was
scanned by no run, ever.

Widening was previously unsafe for exactly the reason this PR fixes: more
overlap meant more duplicate comments. With the dedup above, re-scanning a
companion costs nothing observable, so the window can be sized for the worst
measured gap. That is OMN-16792 option 1; options 2 (event trigger) and 3
(runtime scheduler) stay open on that ticket.

Residual, recorded not fixed: max_companions is capped at 50 and a 10h
window hit that cap (run 33128661860, companions_scanned=50), so the cap —
not the lookback — bounds how wide the window can usefully go. Sizing it
belongs to OMN-16792. Its AC3 (kill-switch wiring end to end) is untouched here.

Tests (TDD, red first)

tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py
— 10 tests, all red before the fix (8 on the missing enum member / behavior, 2
green by construction):

  • the same fixture window run twice against one stateful fake Linear writes
    exactly one comment; the second run records SKIPPED_DUPLICATE_COMMENT;
  • the posted body carries the marker (class=, fingerprint=);
  • dedup holds across a different companion binding the same ticket;
  • a changed verdict does get a second comment;
  • an unreadable history is an error outcome with zero writes;
  • both other gap classes dedupe, with their own marker class;
  • DRY-RUN writes nothing on every path, and previews the suppression once a
    marker exists;
  • a suppressed duplicate counts as skipped, not gap_posted.

The two pre-existing doubles now serve their own write log back through
fetch_comment_bodies. A fake that forgets what it was told cannot see this
defect — which is why the suite stayed green while production repeated itself.

Local verification

  • uv run pytest tests/unit/nodes/node_evidence_autoclose_sweep_effect/ -q → 71 passed
  • uv run mypy src/ --strict → Success, 2858 source files
  • uv run ruff format / ruff check --fix → clean
  • pre-commit run --all-files → all hooks pass (the OMN-14350 ratchet reports
    two soft, report-only, non-gate-blocking _LinearClient candidates, one of
    them in a file this PR does not touch; it passes on re-run after regenerating
    its own baseline)
  • pre-push: the governed impacted-test selector (prepush_smart_tests.sh) ran
    as-is. No PREPUSH_FULL_SUITE, no ENABLE_SMART_TESTS toggle, no
    --no-verify, no skip tokens.

Refs OMN-16808, OMN-16792, OMN-16106.

Evidence-Ticket: OMN-16808
Evidence-Source: OCC#7360

@coderabbitai

coderabbitai Bot commented Aug 28, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 11 minutes.

View limit details

Limit details: You’ve used the included review currently available. Your 131 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: da4807bc-7085-4bc0-943c-a99d9ed0058e

📥 Commits

Reviewing files that changed from the base of the PR and between 93c42ad and 96da7b9.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (9)
  • .github/workflows/evidence-autoclose-sweep.yml
  • docker/runners/runner-image.lock.json
  • pyproject.toml
  • src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/contract.yaml
  • src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/handlers/handler_evidence_autoclose_sweep.py
  • src/omnibase_infra/nodes/node_evidence_autoclose_sweep_effect/models/enum_evidence_autoclose_decision.py
  • tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_handler_evidence_autoclose_sweep.py
  • tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_15911_behavior_proof_gate.py
  • tests/unit/nodes/node_evidence_autoclose_sweep_effect/test_omn_16808_comment_idempotency.py

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Hostile Reviewer — DEGRADED (informational)

Blocking findings (critical): 0
Total findings: 0
Models succeeded: none

Note: All reviewer models failed or were unavailable. Degraded results are informational during the pilot phase (OMN-8468/OMN-8524) and do not block merge. Error: all review endpoints [192.168.86.201:8000 192.168.86.201:8000 ] unreachable — preflight short-circuit (no models available)


Gate semantics (pilot phase)

Verdict Meaning Blocks merge?
passed No critical findings No
blocked CRITICAL findings found Yes
degraded All models unavailable (infra) No (pilot)

Powered by omniintelligence.review_pairing.cli_review — multi-model adversarial review (OMN-8468/OMN-8524)

…t 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant