Skip to content

fix(review): make listPullRequestFiles order deterministic to stabilize content fingerprints - #4481

Merged
JSONbored merged 2 commits into
mainfrom
claude/fix-pull-request-files-order-nondeterminism
Jul 9, 2026
Merged

fix(review): make listPullRequestFiles order deterministic to stabilize content fingerprints#4481
JSONbored merged 2 commits into
mainfrom
claude/fix-pull-request-files-order-nondeterminism

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • listPullRequestFiles (src/db/repositories.ts) had no ORDER BY, so row order was whatever the query planner returned — not guaranteed stable across repeat calls for the same unchanged PR.
  • buildUnifiedReviewDiff only fully orders files by (priority bucket, added-line count); files tied on both (common — e.g. multiple source files with 0 net additions) fall through to this function's own (previously undefined) order.
  • That untied order flows straight into content-hash fingerprints downstream (linkedIssueSatisfactionCacheInputFingerprint and similar), so a silent reorder alone changes the hash and defeats caches even though the diff's actual content never changed.
  • Confirmed live: JSONbored/metagraphed#4532 re-ran its linked-issue-satisfaction LLM call 12 times across 7 hours on one unchanged head SHA, despite a matching cache row already existing (496 rows total in that cache table, 0 recorded hits ever) — the PR's own title/body and its linked issue's title/body were both unedited during that window (verified via the GitHub timeline API), ruling out a legitimate content change as the cause.
  • Fix: add an explicit .orderBy(pullRequestFiles.path). path is unique per (repoFullName, pullNumber) per the table's own unique index, so it alone is a total, stable order — no secondary tie-break needed.

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.
  • Owner PR, no linked issue required — a live-production caching bug found and root-caused during tonight's post-incident audit.

Validation

  • npm run typecheck
  • New regression test added and verified to FAIL without the fix (temporarily reverted it locally, confirmed the test catches the exact insertion-order-vs-alphabetical mismatch) and PASS with it.
  • npx vitest run test/unit/backfill.test.ts test/unit/backfill-file-hydration-scoping.test.ts test/unit/data-spine.test.ts test/unit/queue.test.ts test/unit/repository-settings-linked-issue-satisfaction.test.ts (1002 passed)
  • Full npm run test:coverage not re-run unsharded locally for this diff; relying on CI's full gate given the change is a single-line, well-covered, low-surface-area fix (one new .orderBy() call).

If any required check was skipped, explain why:

  • See above — targeted suite run instead of the full unsharded suite, given the change's narrow blast radius and the lateness of an already-long incident-response session; CI runs the full gate.

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.
  • N/A — no auth/cookie/CORS/GitHub App/Cloudflare/session changes.
  • N/A — no API/OpenAPI/MCP behavior changed.
  • N/A — no UI changes.

Notes

  • This same instability plausibly also slightly undermines ai_review/ai_slop's own cache hit rates (both build diffs through the same buildAiReviewDiff/buildUnifiedReviewDiff path), though those two also have upstream freeze/pause gates as a primary defense, so the practical impact there is smaller. linked_issue_satisfaction has no such upstream gate today and relies entirely on this fingerprint cache, which is why it was fully exposed to this bug.

…ze content fingerprints

listPullRequestFiles had no ORDER BY, so row order was whatever the query
planner returned -- not guaranteed stable across repeat calls for the same
unchanged PR. buildUnifiedReviewDiff only fully orders files by (priority
bucket, added-line count); files tied on both fall through to this
function's own order, so an unstable order silently reorders the diff
string and flips any hash built from it. Confirmed live:
JSONbored/metagraphed#4532 re-ran its linked-issue-satisfaction LLM call 12
times across 7 hours on one unchanged head SHA, despite a matching cache
row already existing, because the fingerprint (which includes the diff
text) never matched between passes. Adds an explicit ORDER BY path (unique
per repo+pull per the table's own index, so no secondary tie-break is
needed) and a regression test pinning the deterministic order.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.03%. Comparing base (8fafbdd) to head (57f4589).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4481   +/-   ##
=======================================
  Coverage   94.03%   94.03%           
=======================================
  Files         420      420           
  Lines       37521    37521           
  Branches    13701    13701           
=======================================
  Hits        35282    35282           
  Misses       1583     1583           
  Partials      656      656           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.72% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

loopover-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-09 21:53:15 UTC

3 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This is a narrow, well-targeted fix: `listPullRequestFiles` (src/db/repositories.ts) gains an explicit `.orderBy(pullRequestFiles.path)`, replacing an undefined query-planner order that could silently flip and defeat the downstream content-fingerprint cache described in the PR (confirmed live via the metagraphed#4532 incident: 12 redundant LLM calls on one unchanged head SHA). The accompanying regression test inserts three files out of path order and asserts both `listPullRequestFiles` calls return the same alphabetical order, which is a real assertion against the actual code path, not a fabricated one. The fix's correctness rests on the claim that `path` is unique per `(repoFullName, pullNumber)` via a table-level unique index, which isn't visible in the provided context, but is used only as a tie-break key here and the change is a strict improvement over the previous unordered query regardless.

Nits — 5 non-blocking
  • Could not verify from the provided context that `pullRequestFiles` actually has a unique index on `(repoFullName, pullNumber, path)` as the PR claims — worth a schema.ts/migrations spot-check to confirm `path` alone is truly a total order and no secondary tie-break is needed.
  • The 12-line explanatory comment on `listPullRequestFiles` (src/db/repositories.ts) is dense for a one-line `.orderBy()` addition; matches this repo's existing verbose-comment convention (see backfill.ts) so not out of place, but could be trimmed to the load-bearing claim plus the incident reference.
  • The new regression test lives under `test/unit/backfill-file-hydration-scoping.test.ts`, a file otherwise scoped to hydration-scoping tests rather than ordering/repository-layer behavior — consider whether a repositories-focused test file is a better home.
  • Confirm the unique index backing the path-uniqueness claim exists in schema.ts, and cite it by name/line in the comment for future readers.
  • Consider whether `buildUnifiedReviewDiff`'s own tie-break (priority bucket, added-line count) should also document that it now relies on `listPullRequestFiles`'s stable order as its final tie-break, so the invariant isn't only documented on one side.

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.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
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 (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 49 registered-repo PR(s), 41 merged, 377 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 49 PR(s), 377 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 is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 49 PR(s), 377 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 9, 2026
… file-listing fix

listPullRequestFiles now returns files in path-ascending order (previous
commit). This test's fixture seeded files as [src/a.ts, README.md] and
asserted that exact (insertion) order, which only happened to match the
old undefined query order by coincidence -- README.md sorts before
src/a.ts alphabetically. Full unsharded suite confirmed this was the only
test relying on the old order (12844 passed).
@JSONbored
JSONbored merged commit 37bce00 into main Jul 9, 2026
11 checks passed
@JSONbored
JSONbored deleted the claude/fix-pull-request-files-order-nondeterminism branch July 9, 2026 21:53
loopover-orb Bot pushed a commit that referenced this pull request Jul 10, 2026
…on queries (#4501) (#4560)

Two queries in repositories.ts ordered by a timestamp column alone with no
secondary tiebreak, the same bug class already fixed in listPullRequestFiles
(#4481): same-millisecond ties under concurrent writes make row order (and
therefore which rows survive a cap) query-plan-dependent instead of
deterministic. Adds an id tiebreak to the product-usage daily rollup's main
and retention scans, and to review-suppression's eviction and read queries,
matching the established desc(id) convention elsewhere in this file.
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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant