LearnNewsExamplesServices
Frontmatter
titlefix(github-workflow): sentinel sweep for comment-deletion drift (#10092)
authortobiu
stateMerged
createdAtApr 19, 2026, 2:00 PM
updatedAtApr 19, 2026, 2:03 PM
closedAtApr 19, 2026, 2:03 PM
mergedAtApr 19, 2026, 2:03 PM
branchesdev ← agent/10092-comment-deletion-detection
urlhttps://github.com/neomjs/neo/pull/10093
Merged
tobiu
tobiu commented on Apr 19, 2026, 2:00 PM

Summary

Delta-sync uses issue.updatedAt as its invalidation signal, and GitHub does not bump updatedAt when a comment is deleted. Affected local files keep stale commentsCount + stale comment bodies indefinitely — the mode that kept #9535 frozen for weeks during the #10090 investigation. GraphQL exposes no ISSUE_COMMENT_DELETED_EVENT, so the timeline-scan pattern from #7948 (for sub-issue / blocking relationship drift) does not transfer here.

This PR adds the option-B "totals-only sentinel" approach from the #10092 ticket: a batched GraphQL query that compares live comments.totalCount against a new stored commentsTotal sentinel in metadata. Mismatches feed the existing refetchIssuesByNumber recovery primitive introduced in #10091 — no new surface, no polling, no since: filter. ~1 rate-limit unit per ~100 issues.

Architectural Reality

  • Distinct from #10090: That PR ensured full event retrieval when we DO fetch an issue (timelineItems pagination). This PR decides when to fetch the issues that updatedAt silently overlooks.
  • Distinct from #7948: That precedent catches relationship events (SubIssueAddedEvent, BlockedByAddedEvent) that also don't bump updatedAt — but those events still appear in the timeline. Comment deletions leave no event trace; the timeline only shows the surviving nodes. The only correctness-preserving primitive is comments.totalCount equality.
  • Lazy seeding: Entries missing the new commentsTotal field (pre-migration metadata) are seeded in place from the live value WITHOUT triggering a refetch. First sync after merge graduates every tracked issue cleanly; subsequent syncs catch drift.

Changes

  • issueQueries.mjs — buildIssueTotalsBatchQuery(numbers) function export (first non-template export in this file) that composes aliased GraphQL fields (issue42: issue(number:42) { comments { totalCount } }) for N issues in one request. Complexity ~3×N, safely under GitHub's per-query limit for the default batch size.
  • IssueSyncer.mjs — new detectStaleCommentsCounts(metadata) primitive. Lazily seeds missing commentsTotal entries in place; otherwise records stale entries for downstream recovery. Metadata writes in pullFromGitHub and refetchIssuesByNumber now capture commentsTotal alongside contentHash.
  • MetadataManager.mjs — persists commentsTotal in the pruned issue metadata shape.
  • SyncService.mjs — wires the sweep inside runFullSync after the delta pull; pipes mismatches into refetchIssuesByNumber; surfaces checked/stale/seeded/errors counts in the final log line and the return stats.
  • config.template.mjs — adds staleCommentsBatchSize: 100 knob.

Test Coverage

Extended IssueSyncer.spec.mjs with three cases (all pass, 608ms total for the 4-test suite):

  1. Lazy seed — metadata missing commentsTotal → seeded in place from live total, no refetch triggered.
  2. Happy path — stored === live for all issues → no drift, no refetches.
  3. Drift detection (#9535 pattern) — stored commentsTotal: 11, live totalCount: 10 → one stale entry recorded, stored sentinel untouched (caller runs refetchIssuesByNumber to authoritatively re-render).

Live Verification

Ran an in-process GraphQL probe against neomjs/neo with 5 real issue numbers to confirm the aliased-field batch query works end-to-end against live GitHub:

issue10090: #10090 commentsTotal=0
issue10092: #10092 commentsTotal=0
issue9535:  #9535  commentsTotal=10
issue9999:  #9999  commentsTotal=1
issue10030: #10030 commentsTotal=2
rateLimit:  {"cost":1,"remaining":4993,...}

Avoided Traps

  • Timeline-scan pattern (#7948 style). Won't work — no deletion event type in GraphQL timelineItems.
  • Raising sync frequency. Tightens the drift window; does not close it.
  • Polling a second API surface (REST /timeline has DeletedCommentEvent). New infra dependency for a case solvable via existing GraphQL primitives.
  • Full-scan sweep on a timer. Option A from the ticket; dropped since: means O(all issues) cost. Batched sentinel at O(issues/100) hits the same correctness at 1% of the cost.

Acceptance Criteria

  • buildIssueTotalsBatchQuery fetches {number, comments{totalCount}} with no since: filter
  • MetadataManager persists commentsTotal per issue
  • IssueSyncer.detectStaleCommentsCounts(metadata) runs inside runFullSync after the delta pull; mismatches feed refetchIssuesByNumber
  • Playwright spec covers happy path AND deletion-detection path
  • runFullSync logs + return-stats expose checked/stale/seeded/errors counts

Known Edge / Follow-up

  • GraphQL issue(number:N) errors hard if N is a PR instead of an Issue, which fails the entire batch. metadata.issues excludes PRs by contract (PRs live in metadata.pulls), so this is a theoretical edge — but per-alias error isolation (retry-as-singles on batch failure) would be more defensive. Not in scope for this PR; worth capturing if it ever fires in production logs.

Test plan

  • npm run test-unit -- test/playwright/unit/ai/mcp/server/github-workflow/IssueSyncer.spec.mjs — 4 passed
  • Live GraphQL probe against real repo — aliased batch query returns expected shape
  • (Reviewer) On first merge + sync_all, the log should show StaleCounts: ~500 checked, 0 drifted, ~500 seeded as the one-time seeding event. Subsequent syncs should show 0 seeded and 0 drifted except when real deletions occur.

Resolves #10092

Related: #10090 (pagination fix — introduced refetchIssuesByNumber, the recovery primitive this PR reuses), #7948 (analogous updatedAt-blind workaround for relationship events)

tobiu
tobiu commented on Apr 19, 2026, 2:01 PM

Input from Claude Opus 4.7 (Claude Code):

❋ # PR Review Summary

Status: Comment (ready for human merge; non-blocking follow-ups noted)

Self-Review Opening: Self-review of #10092. I chose option B (totals-only sentinel sweep via aliased GraphQL batch query) over option A (drop since: on a timer) and option C (per-repo events API polling). B was the only choice that closes the drift window at sub-linear cost without introducing a second API surface. Reusing refetchIssuesByNumber from #10091 as the recovery primitive was the architectural pivot: the sweep only has to detect, not heal — healing is already a solved primitive on dev. Gaps noted below, hunting blind spots harder than I did in the PR body.

📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 93 — The sentinel adds one query-builder, one service method, one runFullSync step, and one metadata field. Every orthogonal concern lives in its natural layer (issueQueries.mjs for GraphQL composition, IssueSyncer for the detection primitive, SyncService for orchestration, MetadataManager for persistence). Reuses refetchIssuesByNumber instead of reimplementing — one recovery primitive, multiple trigger sites. Lost 7 points for the function-vs-constant inconsistency in issueQueries.mjs: this file previously exported only static const templates, and I'm now introducing the first export function for dynamic shape. It's the correct call (aliased fields need N-parameterization) but worth flagging that future agents may want to see a header note about the mixed-export convention.

  • [CONTENT_COMPLETENESS]: 88 — JSDoc with @summary + Anchor & Echo on all new methods (buildIssueTotalsBatchQuery, detectStaleCommentsCounts, the SyncService orchestration block). PR body and commit message both follow Fat Ticket conventions with explicit cross-links to #10090 / #7948 / #9535. Lost points because the commentsTotal field on MetadataManager.save has a 3-line comment block explaining its role; a reader would still have to dig into detectStaleCommentsCounts to understand the lazy-seed semantics. An inline @see pointer would have closed that loop.

  • [EXECUTION_QUALITY]: 82 — Spec passes (608ms) and covers the three scenarios that matter (seed / no-drift / drift). Live GraphQL probe confirms the aliased batch shape works against real GitHub. What's missing: (a) no coverage of the error path (batch query failure mid-iteration — the loop should continue past errors per the code, but the spec doesn't enforce that contract); (b) no coverage of the batch-size boundary (what happens if issueNumbers.length straddles staleCommentsBatchSize?); (c) no test for the SyncService.runFullSync wire-up — if the detectStaleCommentsCounts → refetchIssuesByNumber pipeline breaks between those two calls, nothing catches it until a live user sees stale drift. A mocked SyncService orchestration spec would have been cheap insurance.

  • [PRODUCTIVITY]: 95 — All ticket acceptance criteria checked in one session, following #10091's momentum without context-switch overhead.

  • [IMPACT]: 78 — Closes a known silent-drift bug class. Lower than #10091 because the empirical blast radius is smaller: only issues with comment deletions drift, whereas #10091's timeline-cap drift hit any active Epic. Still load-bearing for content authoritativeness, hence not under 70.

  • [COMPLEXITY]: 50 — Six files, five of them adding ~20-100 lines, one spec. Single new GraphQL pattern (aliased fields) is the only real cognitive load; everything else is plumbing.

  • [EFFORT_PROFILE]: Quick Win — High-ROI detection primitive delivered at low complexity, leveraging existing recovery machinery.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #10092
  • Related Graph Nodes:
    • #10090 / PR #10091 (pagination fix — introduced refetchIssuesByNumber, this PR's recovery dependency)
    • #7948 (prior updatedAt-blind workaround for relationship events — this PR's conceptual precedent with a different primitive)
    • #9535 (the concrete 11→10 reproducer that surfaced the bug during #10090's detector sweep)

🧠 Graph Ingestion Notes

  • [KB_GAP]: The three distinct updatedAt-blind bug classes in the GitHub sync pipeline — relationship events (#7948), timeline truncation (#10090), comment deletions (this PR) — collectively point at a missing architectural abstraction: a "sync invalidation strategy registry" that enumerates every known updatedAt blind spot with its detection primitive. Today each one is patched inline with a bespoke mechanism (timeline-scan for events, pagination for truncation, totals-sentinel for deletions). A future refactor could formalize this into a pluggable InvalidationDetector interface. Out of scope here; worth capturing as framework R&D in learn/agentos/.

  • [KB_GAP]: GraphQL field aliasing is the right primitive for arbitrary-issue-number batch queries against repository.issue(number:). This isn't obvious from training data — many agents would reflexively reach for issues(first:, filterBy:) which filters but can't enumerate. Worth a Neo.mjs agentos note: "To batch-query arbitrary GitHub entities by ID without N round-trips, use aliased fields at ~3 complexity units per alias, safely under GitHub's 500-unit per-query ceiling for batches up to ~150."

  • [TOOLING_GAP]: GraphqlService.query throws the entire response away if the GraphQL endpoint returns errors — even if data is partially populated. GraphQL semantics explicitly allow partial data alongside per-field errors (an aliased field with issue(number: PR_NUMBER) returns null on that alias and populates all others). The current all-or-nothing error handling means a single bad number in a batch kills the whole batch for my sweep. Not a correctness issue (metadata.issues excludes PRs by contract), but it's a latent fragility that would benefit from graceful partial-data handling inside GraphqlService. Captured as the "per-alias error tolerance" follow-up in the PR body.

  • [TOOLING_GAP]: Verifying this PR required exercising detectStaleCommentsCounts against the live GitHub API, which the Playwright spec can't do (it mocks GraphqlService). I used a one-shot node --input-type=module -e '...' invocation to bootstrap the SDK facade and run a real aliased query. This worked but feels ceremonial — a reusable ai/scripts/probe-github-graphql.mjs harness for quick interactive queries would be more honest than inlining multi-line scripts in bash. Worth creating if this pattern recurs.

  • [RETROSPECTIVE]: The session-level arc — #10030 reproducer → #10090 ticket → detector → pagination fix → #10092 follow-up → sentinel sweep — is a good example of the "one ticket per discrete failure mode" discipline paying off. I consciously did NOT cram deletion detection into #10091 even though the detector already incidentally healed #9535. That restraint preserved the PR's surgical scope (reviewers could assess pagination on its own merits) AND exposed the distinct bug class cleanly for this follow-up. Collapsed, both PRs would have been harder to review and the #7948 → #10090 → #10092 architectural progression would have been invisible to future agents reading the merge history.

  • [RETROSPECTIVE]: Reusing refetchIssuesByNumber from #10091 as the recovery primitive was the key architectural move that kept this PR small. The sentinel sweep has zero rewrite-logic — it only detects. Every future "updatedAt blind spot" fix should reuse refetchIssuesByNumber as its healing endpoint rather than writing its own. This is the payoff pattern from making recovery a first-class SDK method in #10091.

📋 Required Actions

No hard blockers. Optional polish items:

  • (Optional) Add spec coverage for the batch-size boundary (e.g., 150 issues with staleCommentsBatchSize: 100 should produce 2 batches).
  • (Optional) Mocked SyncService.runFullSync integration spec that exercises the detect → refetch wire-up.
  • (Optional) Per-alias error tolerance in detectStaleCommentsCounts — on batch failure, retry as singles so one bad number doesn't blackhole ~100 legitimate ones.
  • (Optional) @see IssueSyncer.detectStaleCommentsCounts JSDoc pointer on the commentsTotal field comment in MetadataManager so readers don't have to hunt for the lazy-seed rationale.
  • (Optional) ai/scripts/probe-github-graphql.mjs reusable live-query harness for future PRs that need SDK-bootstrapped GraphQL verification.

Reviewer is free to merge as-is and let me fold any of these into a later PR, or ask for polish commits on this branch before merge. Either works — the core correctness is established by three passing unit cases + the live GraphQL probe.