Skip to content

fix(db): fold seven Drizzle-schema identity tables into renameRepositoryIdentity - #8416

Closed
kai392 wants to merge 1 commit into
JSONbored:mainfrom
kai392:fix/critical-issue-rename-identity-drizzle-tables
Closed

fix(db): fold seven Drizzle-schema identity tables into renameRepositoryIdentity#8416
kai392 wants to merge 1 commit into
JSONbored:mainfrom
kai392:fix/critical-issue-rename-identity-drizzle-tables

Conversation

@kai392

@kai392 kai392 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

The Drizzle half of the rename-orphan gap (companion to the raw-SQL half, #8380/#8409). Seven src/db/schema.ts tables carry a repo_full_name identity column with live writers but were never covered by renameRepositoryIdentity, so a repo-rename webhook permanently disconnected a repo's BYOK provider key, Linear key, open bounties, review-suppression rules, pending agent approvals, issue-watch subscriptions, and command-feedback votes.

Each uses the fold strategy its real constraint requires, verified in schema.ts, and reuses a shape already present in the module:

Table Constraint Strategy
repositoryAiKeys, repositoryLinearKeys repo_full_name PRIMARY KEY fold-then-rename anchor, same as repositories
bounties UNIQUE (repo_full_name, issue_number) fold on issue_number, same as pullRequests/issues
reviewSuppression UNIQUE (repo_full_name, category, path_glob, pattern_hash) per-tuple fold, same as checkSummaries
agentPendingActions UNIQUE (repo_full_name, pull_number, action_class) per-tuple fold, same as pullRequestFiles
issueWatchSubscriptions UNIQUE (login, repo_full_name) single-column fold on login, same as contributorRepoStats
githubAgentCommandFeedback only unique index is (answer_id, actor_hash) plain rename, same as repoSnapshots

Two details worth flagging, both verified rather than assumed:

  • No id rewrites. Unlike pullRequests/issues/contributorRepoStats, none of these ids embed the repo name — five are crypto.randomUUID() at their writer, and bounties.id is an external id (String(issue.id) from the Gitt snapshot, src/bounties/ingest.ts). Rewriting them would corrupt an external key.
  • No isNull branching in the two per-tuple folds: unlike checkSummaries.headSha, every fold column here is notNull in the schema, so a plain eq() per column is correct.

Closes #8379

Test plan

  • Six new describe blocks in test/unit/repo-identity-rename.test.ts, each asserting the fold keeps the old row's real data (key ciphertext, bounty status, the maintainer's accepted pending-action) rather than the post-rename fragment, and that near-miss rows survive untouched — a suppression rule differing only by pattern_hash, another differing only by category, a pending action differing only by action_class, another by pull_number, a different watcher's subscription, a different repo's feedback
  • github_agent_command_feedback asserts both rows survive the rename (no unique index on that column, so nothing may fold)
  • Suite green (67/67, all pre-existing assertions unmodified)
  • Local coverage on src/db/repo-identity-rename.ts: 132/132 lines, 37/38 branches — the single partial branch is the pre-existing repoParts ternary at line 67, outside this diff (lines 366-445)
  • npm run typecheck clean
  • CI validate

@kai392
kai392 requested a review from JSONbored as a code owner July 24, 2026 12:22
@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 24, 2026
@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-24 12:35:24 UTC

2 files · 1 AI reviewer · no blockers · CI pending · dirty

⏸️ Suggested Action - Manual Review

Review summary
This PR extends renameRepositoryIdentity with seven more tables, each folded per its actual constraint as verified against schema.ts (PK-anchor fold for repositoryAiKeys/repositoryLinearKeys, issue_number fold for bounties, per-tuple folds for reviewSuppression/agentPendingActions, single-column fold for issueWatchSubscriptions, plain rename for githubAgentCommandFeedback). The strategies match the existing patterns already used elsewhere in the same function (repositories, checkSummaries, contributorRepoStats, repoSnapshots), and each new block is paired with a targeted regression test that seeds a colliding row and asserts the fold keeps the pre-existing row's data. I traced the bounties external-id claim (src/bounties/ingest.ts per description) and the notNull claim for reviewSuppression/agentPendingActions cannot be independently re-verified since schema.ts isn't in the provided context, but the per-tuple fold logic itself is internally consistent with the isNull-branching pattern used for checkSummaries where nullability does exist.

Nits — 4 non-blocking
  • src/db/repo-identity-rename.ts is now ~448 lines in one function; each table block is independently reasoned so this is a stylistic size concern, not a maintainability blocker given the file's own stated convention of one explicit block per table.
  • The new githubAgentCommandFeedback plain-rename block (repo-identity-rename.ts) trusts that repo_full_name never appears in a unique index; worth a one-line comment cross-reference to the actual migration file (like the review_audit/contributor_gate_history blocks do) for future auditability.
  • Consider extracting the repeated 'collect distinct fold keys → delete colliding rows → update' shape (used now 8+ times) into a small typed helper to cut duplication, though the file's own header comment explains why per-table explicitness was chosen over a shared abstraction.
  • Double check that reviewSuppression.category/pathGlob/patternHash are indeed NOT NULL in schema.ts as claimed, since a NULL there would make the eq() fold silently miss the collision (same class of bug the checkSummaries isNull branch exists to prevent).

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8379
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: 117 registered-repo PR(s), 62 merged, 4 issue(s).
Contributor context ✅ Confirmed Gittensor contributor kai392; Gittensor profile; 117 PR(s), 4 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Addressed
The diff adds all seven listed tables to renameRepositoryIdentity with the exact fold strategies specified (fold-then-rename PK anchors for repositoryAiKeys/repositoryLinearKeys, fold-on-issue_number for bounties, per-tuple folds for reviewSuppression and agentPendingActions, single-column fold for issueWatchSubscriptions, and plain rename for githubAgentCommandFeedback), matching each deliverable

Review context
  • Author: kai392
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 117 PR(s), 4 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (conflicts with the base branch — resolve and open a fresh PR). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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.

Projects

None yet

2 participants