Skip to content

fix(review): persist culture profile total count - #3957

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-culture-profile-cache-validation-issue
Jul 7, 2026
Merged

fix(review): persist culture profile total count#3957
JSONbored merged 1 commit into
mainfrom
codex/fix-culture-profile-cache-validation-issue

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • A cache-invalidaton bug caused repo culture-profile snapshots to record the capped sample size (from listRecentMergedPullRequests() which limits to 200 rows) while cache validation compared against the uncapped total (countRecentMergedPullRequests()), making caches permanently stale for repos with >200 merged PRs.
  • The stale behavior causes repeated re-derivation and unbounded signal_snapshots writes under normal PR activity when the feature is enabled, increasing cost/storage pressure.

Description

  • Persist the uncapped merged-PR total when writing a culture-profile snapshot by computing sampleCountAtGeneration = await countRecentMergedPullRequests(env, repoFullName) and passing it to persistCultureProfile in src/review/repo-culture-profile.ts so the writer and validator use the same basis for invalidation.
  • Add a regression test in test/unit/repo-culture-profile.test.ts that seeds 201 merged PR records, verifies the derived profile uses the 200-row sample for statistics, and confirms a subsequent extraction reuses the cached snapshot (asserting only one signal_snapshots row was created).
  • This change preserves existing behavior for repos with ≤200 merged PRs and fails safe to the insufficient-data branch on storage errors.

Testing

  • npm run typecheck completed successfully with no type errors.
  • npx vitest run test/unit/repo-culture-profile.test.ts passed and the new regression test (201-PR case) succeeded.
  • Targeted run with the test name (npx vitest -t "reuses a fresh culture-profile cache") also passed.
  • Attempted coverage reporting (npm run test:coverage -- --run test/unit/repo-culture-profile.test.ts) executed the tests but coverage remapping failed with TypeError: jsTokens is not a function due to a local dependency resolution mismatch in ast-v8-to-istanbul, and npm audit --audit-level=moderate could not complete in this environment (registry/auth error).

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.66%. Comparing base (1ce4364) to head (310fa53).
⚠️ Report is 16 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3957   +/-   ##
=======================================
  Coverage   93.66%   93.66%           
=======================================
  Files         372      372           
  Lines       34856    34857    +1     
  Branches    12743    12743           
=======================================
+ Hits        32649    32650    +1     
  Misses       1588     1588           
  Partials      619      619           
Files with missing lines Coverage Δ
src/review/repo-culture-profile.ts 100.00% <100.00%> (ø)
🚀 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 7, 2026
@loopover-orb

loopover-orb Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-07 07:57:43 UTC

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

⏸️ 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 fixes a real cache-invalidation bug: previously `persistCultureProfile` was given `prs.length` (capped at 200 by `listRecentMergedPullRequests`) as the invalidation basis, while `readCachedCultureProfile` compared against the uncapped `countRecentMergedPullRequests`, so any repo with >200 merged PRs would never see `currentCount === sampleCountAtGeneration` and would re-derive/write a new snapshot on every call. The diff now computes `sampleCountAtGeneration` via `countRecentMergedPullRequests` and threads it into `persistCultureProfile`, aligning the writer and validator on the same basis. The new regression test seeds 201 merged PRs and asserts both the 200-row-capped `sampleSize` and that a second extraction reuses the cache (only one `signal_snapshots` row), which correctly exercises the previously-broken path.

Nits — 3 non-blocking
  • src/review/repo-culture-profile.ts:288 — the miss path now issues an extra `countRecentMergedPullRequests` COUNT query on every cold/stale derive (in addition to the one already done in `readCachedCultureProfile`'s cache check); worth a one-line comment noting this is an intentional cheap COUNT and not a regression, since a future reader might try to "optimize" it away and reintroduce the original bug.
  • test/unit/repo-culture-profile.test.ts — the new 201-PR test doesn't cover the drift-invalidation case (a 202nd PR added after the snapshot) for the >200 branch, though the existing `MIN_SAMPLE_PULL_REQUESTS`-based drift test already exercises that logic path generically.
  • Consider a short inline comment at the `countRecentMergedPullRequests` call in `extractRepoCultureProfile` (src/review/repo-culture-profile.ts:288) referencing the invalidation-basis invariant, so it's harder to accidentally revert to `prs.length` in a future refactor.

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: 51 registered-repo PR(s), 43 merged, 343 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 343 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: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 343 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 7, 2026
@JSONbored
JSONbored merged commit 638a704 into main Jul 7, 2026
10 checks passed
@JSONbored
JSONbored deleted the codex/fix-culture-profile-cache-validation-issue branch July 7, 2026 08:01
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

Development

Successfully merging this pull request may close these issues.

1 participant