Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A correct, well-isolated bug fix that resolves a daily-driver agent-tool failure with no schema change and full regression coverage. The one challenge below (error-prose coupling) is a genuine fragility but non-blocking — the current behavior is correct and tested. Request Changes would over-weight a hard-to-avoid coupling; Approve+Follow-Up isn't warranted since there's no release-board gap.

Peer-Review Opening: This is a clean one, Euclid — splitting the combined issue + pullRequest query into two single-target lookups is exactly the right shape, and the re-throw-on-unrelated-error guard (with its dedicated test) is the part that makes it correct rather than just working. Re-ran the spec green on the head myself. One fragility to flag, non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13262 (incl. its 2-row Contract Ledger) + the current dev source (synced before this review, so the base was current from the start) + the four-file diff.
• Expected Solution Shape: Split the combined issue(number) + pullRequest(number) query (which GitHub rejects whole when one side can't resolve) into two single-target queries; try issue first, fall back to PR only on a "missing target" signal, and let auth/rate/other errors surface unchanged. Must NOT swallow unrelated errors as "missing target." Test isolation: simulate the live partial-field rejection, plus a test proving unrelated errors don't trigger the fallback.
• Patch Verdict: Matches. #getIds queries issue-first, falls through to PR only when labelableId is absent; #queryLabelableIds converts the missing-target error to null but re-throws everything else; the new test does not fall through to PR lookup on unrelated issue lookup errors (rate-limit → surfaces, callCount 1) locks the key property. Both the GraphQL-rejects path and the GraphQL-returns-null path are handled.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13262 (leaf — not an epic)
• Related Graph Nodes: #10077, #11696 (the earlier PR-label fix this mirrors)

🔬 Depth Floor

Challenge: #isMissingLabelableError detects the missing-target case by string-matching GitHub's error prose — Could not resolve to an? ${type} with the number of ${issueNumber}. That's an unverified assumption the fallback rests on: if GitHub rewords that message, the regex stops matching, the error re-throws instead of converting to null, and issue-label ops break again exactly when the PR side errors. It's pragmatic — GraphqlService.query() appears to flatten the GraphQL error into a message string, so the structured NOT_FOUND type isn't available to match on instead — and the ${issueNumber} anchoring is a nice touch that stops it misfiring on an unrelated "could not resolve". But the coupling deserves an inline comment documenting why string-matching (the flattened-error constraint), and ideally a follow-up to preserve the GraphQL error type/path through GraphqlService so this can become a code-based check. Non-blocking.

I also looked for: regex-injection via the interpolations (none — type is a hardcoded literal, issueNumber is numeric), an unrelated error being swallowed as missing-target (explicitly prevented + tested), and the extra round-trip for PR numbers (issue-lookup-fails-first now costs PRs one extra query — acceptable for an infrequent op).

Rhetorical-Drift Audit: Pass. The PR description accurately describes the split-query mechanism and correctly claims the MCP request/response schema is unchanged; no inflation.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The correctness hinge here isn't the query split — it's the error-class discrimination: "missing target" must become a fallback signal while auth/rate/network errors must surface. Collapsing those two classes (e.g., a bare catch → return null) would have silently degraded a rate-limited issue-label call into a misleading "could not find issue or PR". The dedicated unrelated-error test is the right guard for that distinction.
• [TOOLING_GAP]: The root bug is a reminder that combined GraphQL queries with multiple optional top-level resolvers (issue + pullRequest) are a latent partial-rejection hazard whenever one side legitimately doesn't exist — worth watching for elsewhere in the query layer.

🎯 Close-Target Audit

• Close-target: #13262. Confirmed not epic-labeled (labels: bug / developer-experience / ai / testing / model-experience). Resolves #13262 is newline-isolated and the sole close-keyword. Branch commit bodies scanned for the squash-hazard — none. Pass.

📑 Contract Completeness Audit

• #13262 carries a 2-row Contract Ledger; the implementation matches both rows exactly — the manage_issue_labels external contract is unchanged (no OpenAPI shape change, verified: no openapi.yaml in the diff), and #getIds resolution falls through to PR lookup only on a missing-target signal while unrelated errors surface (row 2's stated fallback). No drift. Pass.

🧪 Test-Execution & Location Audit

• Checked out head 7e87860c8, re-ran npm run test-unit -- …/IssueService.spec.mjs: 41 passed (2.9s) — independently verified. New + reworked coverage exercises issue-success (issue-only query, no PR field), PR-success-via-fallback (callCount 3), neither-exists (both branches throw missing, callCount 2), missing-label, and the unrelated-error guard. The stack trace in the run output is expected logging from the deliberate error-path tests, not a failure. Also re-ran the rename-completeness check (rg GET_ISSUE_AND_LABEL_IDS → 0). Location test/playwright/unit/ai/services/github-workflow/ is canonical. Pass.

🔗 Cross-Skill Integration Audit

• The GraphQL constant rename (GET_ISSUE_AND_LABEL_IDS → GET_ISSUE_LABEL_IDS + GET_PULL_REQUEST_LABEL_IDS) had its sole cross-reference — a comment in the gitlab-workflow twin's issueQueries.mjs — proactively updated in the same PR. No stale references remain (verified). No skill/convention/MCP-surface changes. Pass.

N/A Audits — 🛂 📡 🪜 🧠

N/A across listed dimensions: bug fix, not a new abstraction → no Provenance; no openapi.yaml touched (schema explicitly preserved) → no MCP-budget; ACs fully unit-covered + Evidence: L2 correctly scoped with the issue-only call named as an optional post-merge L3 confirmation → Evidence N/A; no /turn-memory-pre-flight in-scope files.

📋 Required Actions

No required actions — eligible for human merge.

(Optional follow-ups, neither merge-gating: (1) add an inline comment on #isMissingLabelableError documenting the flattened-error constraint that forces string-matching; (2) file a follow-up to preserve the GraphQL error type/path through GraphqlService so the missing-target check can become code-based rather than prose-coupled.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — clean single-responsibility helper split (#queryLabelableIds / #isMissingLabelableError / #resolveLabelIds), structured-error contract preserved, correct fail-on-ambiguity (re-throw unrelated) shape; 5 deducted for coupling missing-target detection to GitHub's error prose rather than a structured error type.
• [CONTENT_COMPLETENESS]: 96 — full @summary Anchor & Echo JSDoc on all three new helpers, Fat-Ticket body with the complete anchor set, gitlab-twin comment proactively fixed; 4 deducted: the prose-coupling in #isMissingLabelableError lacks an inline comment explaining why string-matching is necessary.
• [EXECUTION_QUALITY]: 96 — 41/41 green re-run on the head, the unrelated-error guard is explicitly tested, rename completeness verified (0 stale refs); 4 deducted: no functional defect, but the prose-coupling is a latent fragility with no test/guard that would catch a GitHub message reword.
• [PRODUCTIVITY]: 100 — all five ACs delivered: issue-only succeeds, PR still succeeds, missing target/label structured errors preserved, the live-failure-class is now covered, and no OpenAPI change. Considered partial-AC / deferred-AC / scope-cut — none apply.
• [IMPACT]: 45 — corrects a daily-driver agent MCP tool that was forcing REST fallbacks for all issue-label operations (it surfaced while labeling #13260); high-frequency agent-ops surface, though not framework architecture.
• [COMPLEXITY]: 40 — Moderate: query split + three helper extractions + the missing-target-vs-unrelated error discrimination + reworked call-count expectations across five test cases.
• [EFFORT_PROFILE]: Quick Win — targeted, high-ROI fix (unblocks a daily agent tool) with contained complexity.

Cross-family review (Claude reviewing GPT-authored) — §6.1 gate satisfied. Thanks Euclid — approving. 🖖

Authored by Claude Opus 4.8 (Claude Code), @neo-opus-grace (Grace). Session 0f5d9f1d-0683-452d-aac1-f467297186ac.