Skip to content

fix(db): preserve linked-issue claim on a sparse PR sync payload - #3636

Merged
JSONbored merged 1 commit into
mainfrom
fix/linked-issue-sparse-payload-preserve
Jul 5, 2026
Merged

fix(db): preserve linked-issue claim on a sparse PR sync payload#3636
JSONbored merged 1 commit into
mainfrom
fix/linked-issue-sparse-payload-preserve

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Fixes a real production incident on PR feat(review): render a deterministic changed-files summary in the unified comment #3604: the review gate reported "No linked issue detected" as a concern even though the PR body clearly said "Closes Changed-files summary table in the unified review comment #1957" and the panel's own "Linked issue" row in the same comment showed "✅ Linked Changed-files summary table in the unified review comment #1957" — a direct contradiction within one evaluation pass. Confirmed via direct DB inspection that pull_requests.linked_issues_json was correctly [1957] from the moment the PR was created, and that this repo's linkedIssueGateMode is advisory (so the live open-reference fetch path never runs) — ruling out both a live-fetch bug and a genuinely-empty body as the cause.
  • Root cause: upsertPullRequestFromGitHub's onConflictDoUpdate unconditionally re-derives linkedIssuesJson/linkedIssueClaimedAt from the current payload's body on every sync. GitHubPullRequestPayload.body is typed string | null specifically because some callers legitimately don't have it — a narrower webhook event's embedded pull_request sub-object can omit body entirely (undefined), distinct from a genuinely empty description (null/""). Re-deriving from an absent body silently wiped an already-correctly-claimed linked issue (and reset its claim timestamp, since resolveLinkedIssueClaimedAt treats an empty linkedIssues array as "nothing claimed").
  • Fix: when pr.body === undefined and a prior row already exists, fall back to the already-stored linkedIssuesJson instead of re-deriving from the absent body. An explicitly empty body (null/"") — a contributor genuinely clearing their PR description — still clears the claim exactly as before; this only guards the "field is absent" case the type system already models.

No linked issue for this one — it's a narrowly-scoped correctness fix for a confirmed production incident (evidence: the incident report, the DB query results, and the type signature all cited above); see the Notes section for the full investigation trail.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • No issue link — this is a small, well-evidenced bug fix; see the Summary for the concrete incident and root-cause trail in place of a tracked issue.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — 100% line + branch coverage on every changed line, verified via a targeted v8 coverage pass (both branches of the new pr.body === undefined && existingClaimRows[0] condition, in both its linkedIssues and linkedIssuesJson uses) in addition to the full unsharded run.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries: three new tests in test/unit/db-parsers.test.ts — (1) a sparse re-sync (body absent) preserves an already-claimed linked issue and its claim timestamp, including the function's own return value; (2) an explicit empty body (null) still clears the claim, proving the fix doesn't over-broaden; (3) a sparse sync on a brand-new PR (nothing to preserve) falls through to the pre-existing empty-default behavior. Also re-ran the full existing test suite affected by this widely-used function (npm run test:changed, 6000+ tests, 235+ files) with zero regressions.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session/CORS surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP schema change.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no apps/gittensory-ui changes.)
  • Visible UI changes include a UI Evidence section below with screenshots. (N/A — no visible UI change; this is an internal DB-write correctness fix.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A — internal bug fix, no user-facing docs to update.)

Notes

Investigation trail (for reviewer context, not part of the code change):

  • Ruled out the live open-reference GitHub fetch (resolveLinkedIssueHasOpenReference/fetchLinkedIssueFacts) as the cause: JSONbored/gittensory's own linkedIssueGateMode is advisory, so that fetch path is never invoked (resolveLinkedIssueAdvisoryContext short-circuits to Promise.resolve(true)).
  • Ruled out the GitHub response cache (src/github/client.ts): /issues/{number} isn't classified into any cached class (githubCacheClassForUrl returns null for it), so it can't be served stale from Redis; the volatile single-flight coalescer that DOES apply cleans up its map entry synchronously on settle, so it can't replay a 19-minute-old result either.
  • Confirmed via a direct query against the production D1 pull_requests table that linked_issues_json was "[1957]" essentially from the same instant as created_at — the correct value was captured and persisted immediately; something later wiped the in-memory read used for one evaluation pass, which is what led to the upsertPullRequestFromGitHub onConflictDoUpdate overwrite path above.

A narrower webhook event's embedded pull_request sub-object can omit
body entirely (undefined) rather than reporting it as explicitly
empty (null/""). Re-deriving linked issues from an absent body was
silently wiping an already-correctly-claimed linked issue -- and
resetting its claim timestamp -- on any such upsert, which could
make the gate report "no linked issue" on a PR that clearly cited
one in a prior, fuller sync.

Falls back to the existing stored value only when body is genuinely
absent; an explicit empty body still clears the claim as before.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 5, 2026
@loopover-orb

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-05 20:51:29 UTC

2 files · 1 AI reviewer · 1 blocker · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

Review summary
This patches a real production bug: upsertPullRequestFromGitHub previously re-derived linkedIssuesJson from pr.body on every sync, and a sparse webhook payload with body===undefined would wipe an already-claimed linked issue. The fix falls back to the existing stored linkedIssuesJson only when body is undefined AND a prior row exists, leaving the explicit-empty-body-clears-claim behavior intact, and the fix is reflected in both the DB write and the function's return value (record.linkedIssues override). Three new tests cover the sparse-preserve case, the explicit-null-clears case, and the sparse-on-new-PR fallthrough, giving solid branch coverage of the new condition.

Nits — 6 non-blocking
  • src/db/repositories.ts: the condition `pr.body === undefined && existingClaimRows[0]` is repeated for both `linkedIssues` and `linkedIssuesJson`; extracting a `const preserveExisting = pr.body === undefined && existingClaimRows[0] != null` would avoid the duplication and make the branch intent clearer.
  • src/db/repositories.ts: relies on a `parseLinkedIssuesJson` helper not shown in this diff — worth confirming it safely handles a null/empty stored `linkedIssuesJson` (e.g. a row that was written before this column existed) rather than throwing.
  • The inline comment above the new logic is quite long (7+ lines); consider trimming it to the core rationale (sparse payload vs explicit-empty) since the PR description and tests already carry the full context.
  • test/unit/db-parsers.test.ts: the new tests are appropriately targeted, but consider also asserting `resynced.linkedIssuesJson`/stored JSON string directly (not just the parsed array) to catch any accidental double-encoding regression in the `existingClaimRows[0].linkedIssuesJson` passthrough path.
  • Extract the shared `pr.body === undefined && existingClaimRows[0]` predicate into a single named boolean to reduce duplication and ease future edits (src/db/repositories.ts).
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #1957
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 55 registered-repo PR(s), 46 merged, 480 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 55 PR(s), 480 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 55 PR(s), 480 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.23%. Comparing base (87043af) to head (82efe42).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3636   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         314      314           
  Lines       31952    31953    +1     
  Branches    11698    11700    +2     
=======================================
+ Hits        29789    29790    +1     
  Misses       1517     1517           
  Partials      646      646           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.51% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 1986a0c into main Jul 5, 2026
10 checks passed
@JSONbored
JSONbored deleted the fix/linked-issue-sparse-payload-preserve branch July 5, 2026 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

Changed-files summary table in the unified review comment

1 participant