LearnNewsExamplesServices
Frontmatter
titlefix(github-workflow): propagate GraphQL errors from LabelService (#10112)
authortobiu
stateMerged
createdAtApr 20, 2026, 1:02 AM
updatedAtApr 20, 2026, 1:13 AM
closedAtApr 20, 2026, 1:13 AM
mergedAtApr 20, 2026, 1:13 AM
branchesdev ← fix/10112-labelservice-propagate-errors
urlhttps://github.com/neomjs/neo/pull/10114
Merged
tobiu
tobiu commented on Apr 20, 2026, 1:02 AM

Summary

Removes the swallow-and-wrap pattern from LabelService.listLabels() so GraphQL exceptions propagate with their original status + message instead of being reduced to a generic {error, message, code} object that hides diagnostics. Restores observability on the data-sync-pipeline CI workflow — the next intermittent failure will print the actual underlying error (rate-limit 429, 5xx, network) in the CI log instead of the useless "Invalid response from GH_LabelService" wrapper.

Architectural Impact

Before

async listLabels() {
    try {
        // ... paginated query ...
        return { count, labels };
    } catch (error) {
        logger.error('Error fetching labels via GraphQL:', error);
        return { error: 'GraphQL API request failed', message: error.message, code: 'GRAPHQL_API_ERROR' };
    }
}
  • Union-typed return: {count, labels} OR {error, message, code}
  • Callers forced to shape-check; labels.mjs:58 did !response || !response.labels → threw its own 'Invalid response from GH_LabelService', burying the real error twice
  • logger.error writes to stderr but the original Error object's status code / stack is already swallowed by string interpolation in error.message

After

async listLabels() {
    // ... paginated query ...
    return { count, labels };
    // No catch — GraphqlService exceptions propagate unmodified
}
  • Single return shape; exceptions are exceptions
  • labels.mjs no longer shape-checks; its existing outer try/catch logs the real error
  • MCP tool boundary at Server.mjs:150-222 already catches thrown exceptions and converts to structured MCP error payloads — the in-service wrap was redundant with protocol-level handling

Why Not a Broader Refactor?

Same swallow-and-wrap exists at 11 call sites across 4 services (IssueService × 6, DiscussionService × 2, PullRequestService × 2, LabelService × 1). Deliberately scoped narrow to LabelService because:

  • It is the only service exposed through a CI-visible build script (labels.mjs) — its failures are user-visible
  • The other 10 sites route through the MCP tool boundary which already produces usable structured errors for protocol responses
  • A cross-service cleanup would be its own architectural ticket; bundling here would scope-creep and risk semantic drift

Leaving breadcrumbs for a future cleanup if pressure arises, but no commitment.

Diagnostic Payoff

The prevailing hypothesis for why the pipeline fails intermittently: the DevIndex Spider 3x-loop consumes GraphQL rate-limit budget → subsequent GraphQL calls in the same workflow (labels, release, tickets index generation) hit 429. This PR cannot prove that by itself — but once it lands, the next CI failure will print the actual HTTP status + GraphQL error body, and the hypothesis can be empirically confirmed or refuted. Companion ticket #10113 addresses the suspected root cause (spider loop reduction).

Test Coverage

New file: test/playwright/unit/ai/mcp/server/github-workflow/LabelService.spec.mjs with 3 cases:

  1. Happy path + pagination: multi-page query returns aggregated {count, labels} with correct ordering
  2. Error propagation (#10112 core): when GraphqlService.query throws, the exact thrown value propagates unmodified — no catch, no wrap, no synthetic replacement
  3. Non-Error throwables: string / non-Error payloads also propagate unchanged (guards against regressing to catch-all wrapping)

All 3 pass; the existing 2 IssueSyncer cases from #10110 still pass — full gh-workflow spec suite 5/5 green (570ms).

Test Plan

  • npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/mcp/server/github-workflow/ → 5/5 passing
  • Post-merge: next data-sync-pipeline CI run exposes the actual underlying error when/if it fails

Related

  • Companion: #10113 — Reduce DevIndex Spider CI loop from 3x→1x (addresses suspected rate-limit root cause)
  • Future candidate: cross-service error-propagation cleanup (10 remaining swallow sites in IssueService, DiscussionService, PullRequestService) — not scoped here; harmless via MCP tool boundary which already catches
  • Context: builds on #10110 (merged in this session) — same session's earlier PR also refined the gh-workflow sync layer

Avoided Traps

  • Retry logic inside LabelService — wrong layer; retry belongs in GraphqlService as a cross-cutting primitive
  • Wrapping MCP tool boundary to "normalize" shapes — unnecessary, that boundary already produces structured MCP error payloads from thrown exceptions
  • Scope expansion to the 10 sibling swallow sites — tempting but violates the "one ticket → one discrete problem" rule

Resolves #10112

🤖 Generated with Claude Code

tobiu
tobiu commented on Apr 20, 2026, 1:11 AM

Input from Claude Opus 4.7 (Claude Code):

❋ # PR Review Summary

Status: Approved — ready for human QA

Self-Review Opening: Self-review of #10112. This implementation chose to remove LabelService's swallow-and-wrap locally rather than refactor all 11 sibling sites across 4 services. Rationale: LabelService is the only one exposed through a CI-visible build script, so its diagnostic loss is user-observable; the other 10 sites route through the MCP tool boundary which already converts thrown exceptions into structured error payloads, making their wraps functionally redundant but not user-visible. A cross-service cleanup can be filed later if warranted. Key trade-offs, gaps, and one process miss below.

📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 90 — Neo singleton, standard static config, clean removal of the now-unused logger import, JSDoc @summary + @throws contract. Faithful to framework idioms.
  • [CONTENT_COMPLETENESS]: 88 — Anchor (full @summary + @throws on listLabels, explicit reference to Server.mjs:150-222 MCP boundary), Echo (inline comment at labels.mjs:55-57 anchoring to #10112, spec describe-block explaining the pre-#10112 anti-pattern). Docked for the initial PR body claiming "MCP tool boundary already catches" without tracing Server.mjs line numbers or verifying text-format equivalence — corrected during review via user challenge, but the evidence should have been in the PR body from the start.
  • [EXECUTION_QUALITY]: 78 — All 3 new tests pass; existing 2 IssueSyncer cases from #10110 still pass (5/5 green). Tests cover happy-path pagination, Error-typed throw propagation, and non-Error throwable propagation. Docked for a test-layer gap: no integration test drives the full callTool('list_labels', ...) path through Server.mjs:151-222 with a mocked GraphQL failure. That would empirically lock the architectural claim ("exceptions propagate cleanly into MCP error payloads") at the execution level. Mitigated via grep-sweep confirmation that zero existing tests depend on the old shape — but a positive lock is stronger than a negative audit.
  • [PRODUCTIVITY]: 95 — All 4 AC items met pre-merge. The "Next CI failure prints the original GraphQL error" item is only verifiable post-merge.
  • [IMPACT]: 55 — Scope is narrow (one service, one CLI consumer, moderate diagnostic-recovery value). Not framework-critical. Preconditions for empirical confirmation of the rate-limit hypothesis that companion #10113 attempts to root-cause.
  • [COMPLEXITY]: 28 — 3 files, +152/-33, one service change, one CLI shape-check removal, one new spec file. Changes are mechanical.
  • [EFFORT_PROFILE]: Quick Win — High ROI (restores observability on a critical CI pipeline) / Low complexity (net code reduction on the service side, pure additions on the test side).

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #10112
  • Related Graph Nodes:
    • Companion: #10113 (DevIndex Spider 3x→1x; hypothesized root cause of the rate-limit pressure this PR restores visibility into)
    • Built on: #10110 / PR #10111 (merged earlier in this session; same-area architectural work)
    • Deferred follow-ups: 10 sibling swallow-sites in IssueService / DiscussionService / PullRequestService (not user-visible via MCP boundary; cross-service cleanup can be filed if justified post-observation)

🧠 Graph Ingestion Notes

  • [RETROSPECTIVE]: When asserting "existing infrastructure already handles this" in a PR body, produce a claim→evidence table before including the claim — cite file:line, trace the code path, verify the exact output shape. My initial PR body said "MCP tool boundary already catches thrown exceptions and converts to structured MCP error payloads" without citing Server.mjs:211, tracing the catch-branch response shape, or acknowledging that the error text format differs between the wrap path and the throw path. The user's follow-up question ("does it REALLY not break our MCP tool shapes?") caught this rigor gap; the claim was ultimately correct, but arrived at through their challenge rather than my own pre-publication tracing. Bake this into future PR body drafting: never assert an architectural shortcut without the trace at hand.

  • [TOOLING_GAP]: The pr-review skill's Self-Review Detection step (per §2 of pr-review-guide.md) relies on query_raw_memories matching the ticket number within the current session. The Memory Core Consolidate-Then-Save protocol persists memory at end-of-turn — so a PR authored, opened, and reviewed within a single turn has no persisted #N match yet, and the detection query returns empty. This incorrectly triggers peer-review-mode for genuine self-review cases. For this PR I manually overrode to self-review mode based on factual authorship knowledge. A more robust signal would check (a) git log --author on the PR's commits against the authenticated user, or (b) the active agent's in-flight work track, not just persisted memory. Captured here for REM-cycle ingestion.

  • [KB_GAP]: None significant. The architectural primitives (Server.mjs MCP boundary, GraphqlService.query throwing contract) are well-structured and the pattern being removed is documented as legacy in adjacent tickets (#10110, #10092). No framework-concept gap led to the original anti-pattern — it was clearly a bespoke-per-service idiom that accumulated.

📋 Required Actions

None blocking. Optional items for consideration:

  • Consider (not blocking): Add an integration test that drives callTool('list_labels', ...) through Server.mjs:151-222 with a mocked GraphQL failure and asserts the returned MCP payload has isError: true + a text block containing the underlying error message. Would empirically lock the architectural claim at the execution layer, complementing the grep-sweep's negative audit. Skippable if the grep-sweep is judged sufficient evidence.
  • Post-merge validation (unavoidable): Observe the next data-sync-pipeline CI failure — the log should now show the real GraphQL error (HTTP status + body) rather than the pre-#10112 "Invalid response from GH_LabelService" wrapper. Confirms AC item that can't be validated pre-merge.
  • Post-merge graph hygiene (deferred, not blocking this PR): If companion #10113 lands and rate-limit hypothesis is confirmed post-observation, optionally file a cross-service error-propagation cleanup ticket covering the 10 remaining swallow-sites. No commitment yet.

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