LearnNewsExamplesServices
Frontmatter
titlefix(github-workflow): remove commentsTotal sentinel sweep (#10110)
authortobiu
stateMerged
createdAtApr 19, 2026, 11:53 PM
updatedAtApr 20, 2026, 12:15 AM
closedAtApr 20, 2026, 12:15 AM
mergedAtApr 20, 2026, 12:15 AM
branchesdevfix/10110-remove-commentsTotal-sweep
urlhttps://github.com/neomjs/neo/pull/10111
Merged
tobiu
tobiu commented on Apr 19, 2026, 11:53 PM

Summary

Removes the per-sync detectStaleCommentsCounts sentinel sweep introduced by PR #10093 (ticket #10092) and derives the commentsTotal metadata field from the already-fetched timelineItems.nodes — the authoritative post-#10090 pagination source. The sweep was paying ~15s of serial GraphQL round-trip cost every runFullSync to catch a narrow edge case (comment deletion on a subsequently-quiet issue) that is explicitly accepted here.

Architectural Impact

What's removed

Symbol File Rationale
IssueSyncer.detectStaleCommentsCounts services/sync/IssueSyncer.mjs 37 serial GraphQL round trips per sync over 3,669 tracked issues — dominant cost source
buildIssueTotalsBatchQuery services/queries/issueQueries.mjs Only consumer was the sweep
comments { totalCount } sub-selection FETCH_ISSUES_FOR_SYNC + FETCH_SINGLE_ISSUE No remaining consumer once commentsTotal is timeline-derived
$maxComments variable + maxCommentsPerIssue config knob issueQueries.mjs + config.template.mjs Orphaned by the sub-selection removal
staleCommentsBatchSize config knob config.template.mjs Sweep-only tuning knob
Stale-count stats + log line in runFullSync services/SyncService.mjs No longer produced
3 stale-count test cases IssueSyncer.spec.mjs Cover removed code

What's added

  • IssueSyncer.#countTimelineComments(issue) — private helper returning the count of ISSUE_COMMENT nodes in an exhausted timelineItems connection. Full JSDoc with preconditions.
  • New spec case: commentsTotal is derived from timelineItems.nodes filtered for IssueComment (#10110) — mixed-timeline mock proves both metadata.commentsTotal and frontmatter commentsCount come from the same derivation, preventing regression to a dual-source pattern.

What stays

  • refetchIssuesByNumber — the healing primitive from #10090. On-demand recovery path still available to agents and diagnostic scripts.
  • #exhaustTimelineItems — intrinsic to the fetch path; pagination unchanged. The regression test from PR #10091 remains and still passes.
  • Relationship-event timeline scan from #7948 — also intrinsic to pullFromGitHub; untouched.

Historical Context

The gh-workflow MCP server's first versions fetched comments from the dedicated comments endpoint, before unified timelines were introduced (primarily for the portal app's ticket rendering). Sourcing commentsTotal from the separate issue.comments.totalCount scalar was a pre-timeline-era leftover — every comment already lives in timelineItems.nodes as an ISSUE_COMMENT typed node once #10090's pagination-exhaust has run. The #10092 sentinel was secondary infrastructure built on top of that oversight; removing both restores a single source of truth.

Accepted Blind Spot

A deleted comment on an issue that subsequently receives zero activity (no new comments, no label/milestone changes, no reactions) will remain locally rendered until something eventually bumps the issue's updatedAt. This is explicitly accepted: the cost of the sentinel sweep (~15s per every sync) is not justified by the narrow case it catches. Agents needing audit-grade accuracy can invoke refetchIssuesByNumber on demand.

Expected Perf Effect

Boot-sync wall-clock should return to the pre-#10093 baseline (~5-10s instead of the ~20-30s that was exceeding Claude Code's MCP stdio handshake budget and causing intermittent UI invisibility of the gh-workflow server).

Test Plan

  • npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/mcp/server/github-workflow/IssueSyncer.spec.mjs → 2/2 passing (570ms)
  • Post-merge: restart Claude Code and confirm gh-workflow registers within handshake budget on a clean boot with 3,669 tracked issues.

Related

  • Reverses approach of: PR #10093 (ticket #10092)
  • Supersedes: #10094 — InvalidationDetector interface. With CommentTotalsDetector removed, only #7948's relationship-event scan remains, and that's intrinsic to pullFromGitHub, not a sweep. The detector abstraction has zero sweep instances left to unify. Recommend closing #10094 as superseded on merge.
  • Defuses: #10096 — GraphQL partial-data discard on aliased batches. Motivated by the now-removed sweep consumer. No remaining known consumer in gh-workflow; the latent correctness concern in GraphqlService can be deferred or closed unless a new aliased-batch consumer appears.
  • Leaves intact: #10090 (pagination stays; intrinsic to fetch path), #7948 (relationship-event scan stays; intrinsic to pullFromGitHub).

Avoided Traps

  • Scheduled / periodic sweep — still pays the cost, just on a timer.
  • Transport-decoupling of startup sync — consistency with memory-core and knowledge-base (both block transport on sync completion) is a deliberate ecosystem contract.

Resolves #10110

🤖 Generated with Claude Code

tobiu
tobiu commented on Apr 19, 2026, 11:59 PM

Input from Claude Opus 4.7 (Claude Code):

❋ # PR Review Summary

Status: Request Changes (one stray formatting nit) — otherwise ready for human QA

Self-Review Opening: Self-review of #10110. This implementation chose to remove the sentinel sweep entirely over alternatives like parallelizing the batches (Promise.allSettled on the 37 serial round trips) or deferring the sweep to a periodic cadence. The rationale: the sweep's marginal value — catching comment deletions on issues that subsequently receive zero activity — did not justify any continuing cost profile. The historical context that commentsTotal was only ever being sourced from comments.totalCount because of a pre-timeline-era leftover made the architectural rewiring obvious once surfaced. Key trade-offs and blind-spot hunt below.


📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 92 — Uses Neo.core.Base singleton pattern, static config, #private helpers, consistent JSDoc conventions. Not novel — just faithful to existing idioms.
  • [CONTENT_COMPLETENESS]: 86 — Anchor (#countTimelineComments full JSDoc), Echo (class-level doc, inline comments at 3 call sites, MetadataManager doc rewritten, spec describe-block explains both coverage axes). PR body is comprehensive. Docked for one stray blank line introduced at IssueSyncer.mjs line 142 between commentsCount and parentIssue in the frontmatter object — visible in the diff.
  • [EXECUTION_QUALITY]: 82 — Tests pass (2/2, 570ms); no known bugs; no race conditions introduced. Docked for asymmetric test coverage: both derivation sites (pullFromGitHub at line 468 and refetchIssuesByNumber at line 610) call #countTimelineComments, but the new spec drives only through refetchIssuesByNumber. If someone regressed just the pullFromGitHub site, current tests would not fail. Transitive coverage via the pagination test is partial. Also docked for the implementation-path mistake during development where I edited main-checkout files instead of worktree files; caught by a test run showing old test names, recovered via git diff → restore → apply, but the failure mode was mine.
  • [PRODUCTIVITY]: 93 — All 6 ticket AC items met. Bonus vestigial cleanup ($maxComments variables + maxCommentsPerIssue config knob) included as legitimate dead-code removal rather than scope creep.
  • [IMPACT]: 68 — Not framework-level. MCP-ecosystem critical: fixes the boot-time handshake-budget regression that was making gh-workflow intermittently invisible in Claude Code's connectors UI and wedging git pull --rebase mid-sync. Affects every Claude Code session boot.
  • [COMPLEXITY]: 42 — 6 files, +131/−281 (net removal), one new private helper, one new test, wiring changes across three call sites. Complexity heavily reduced by clean prior architecture from #10090/#10092.
  • [EFFORT_PROFILE]: Quick Win — High ROI (boot-time handshake fix, daily ongoing perf cost removed) / Low complexity (mechanical contraction). Prime example of an elegant architectural win surfaced by historical context.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #10110
  • Related Graph Nodes:
    • Reverses approach of: #10092 (ticket), PR #10093
    • Supersedes: #10094 (InvalidationDetector — zero remaining sweep instances; recommend closing on merge)
    • Defuses: #10096 (GraphQL partial-data on aliased batches — motivating consumer removed; no remaining gh-workflow consumer)
    • Leaves intact: #10090 (pagination; intrinsic to fetch path), #7948 (relationship-event scan; intrinsic to pullFromGitHub)

🧠 Graph Ingestion Notes

  • [RETROSPECTIVE]: The #10092 sentinel-sweep pattern emerged from correct local reasoning — its author did a clean job per-ticket — but missed a historical context: commentsTotal was sourced from comments.totalCount only because the gh-workflow server pre-dated unified timelines (before the portal app's rendering requirements drove their adoption). Once timelines became authoritative, deriving the count from timelineItems.nodes was the simpler path. The lesson for future drift-detection work: before adding a parallel API surface, check whether the signal is already derivable from data already in memory. The feedback_commentsTotal_from_timeline memory captures this pattern.

  • [KB_GAP]: None the KB could have prevented. The oversight was visible only with multi-year historical code memory, which is a natural agent gap (ingested docs describe current architecture, not archaeological layers). Mitigated by Memory Core sessions that preserve those layers as agents encounter them.

  • [TOOLING_GAP]: During implementation I edited files in /Users/Shared/github/neomjs/neo/... (main checkout) instead of /Users/Shared/github/neomjs/neo/.claude/worktrees/awesome-elion-29fbbd/... (the worktree that owned the fix branch). Detected only when a test run showed the OLD test names still running. Recovered via git diffgit restore in main + git apply in worktree. The Claude Code Edit/Write tools accept absolute paths without any hint that the user is in a worktree context — a harness-level affordance (e.g., warning when edits target the canonical path when the session is in a worktree) could prevent this category of mistake. Not a blocker for this PR; captured here for REM-cycle ingestion.


📋 Required Actions

  • Blocker: Remove the stray blank line at ai/mcp/server/github-workflow/services/sync/IssueSyncer.mjs line 142 between commentsCount and parentIssue in the frontmatter object. Introduced by the Edit tool's whitespace handling on the derivation-site rewire. Trivial one-line polish commit.
  • Consider: Extend the new spec to drive pullFromGitHub directly (mocking FETCH_ISSUES_FOR_SYNC alongside FETCH_SINGLE_ISSUE) to close the coverage asymmetry between the two #countTimelineComments call sites. Alternatively, document in the spec's describe-block that pullFromGitHub coverage is transitive via the existing pagination regression test and explicitly accept the asymmetry.
  • Post-merge validation: Restart Claude Code on the main checkout (3,669 tracked issues) and confirm gh-workflow registers within the MCP stdio handshake budget on a clean boot. This is the only AC item that cannot be proven pre-merge.
  • Post-merge graph hygiene: Close #10094 as superseded (no remaining sweep instances to unify). Decide on #10096 — keep as latent GraphqlService bug or close as "no current consumer in gh-workflow."

Handing off to human QA. DO NOT auto-merge — §5 of the pull-request-workflow.md mandates human approval for squash merge.