Skip to content

fix(review): reputation signal re-queried twice per pass with zero memoization #4507

Description

@JSONbored

Part of #4496. P10 — low severity (bounded duplicate DB reads, not unbounded/LLM re-spend).

Context

shouldSkipAiForReputation(project, submitter) is invoked with IDENTICAL arguments twice in the same webhook-processing pass: once by shouldStartAiReviewForAdvisory (src/queue/processors.ts:6828, to compute aiReviewWillRun at line 9475) and again inside runAiReviewForAdvisory itself (processors.ts:7167-7175), with no memoization between the two calls even though nothing about the submitter or repo can have changed in the interim.

resolveConvergedFeature (src/review/feature-activation.ts:48-63) shows reputationActive defaults to the SAME isConvergenceRepoAllowed(env, repoFullName) allowlist used by the first call when no per-repo manifest override exists, so both gates evaluate true/false in lockstep for the common (no-override) case — guaranteeing the underlying D1 read (getSubmitterReputation, which scans up to REPUTATION_WINDOW_ROW_CAP = 500 review_targets rows per call, src/review/submitter-reputation.ts:71, 223-258) executes twice per pass with no caching layer at all — reputation-wire.ts has no memoization/cache export.

This is a genuine, verified duplication, but bounded (two extra reads, not unbounded reprocessing or LLM spend) — hence low severity. The codebase already has a precedent for exactly this caller/callee duplication problem: preAcquiredAiReviewLock, used elsewhere in this same function.

Requirements

  • Thread the already-computed boolean (or the SubmitterStats itself) from shouldStartAiReviewForAdvisory's predicate into runAiReviewForAdvisory instead of re-deriving it, mirroring the preAcquiredAiReviewLock pattern.
  • Confirm the divergent-config edge case (a per-repo manifest override disagreeing with the allowlist, where only one call site fires today) still behaves correctly once threaded — i.e. the fix must not force a skip when the two checks would have legitimately disagreed under manifest override.
  • Invariant + regression tests (non-negotiable): an invariant test asserting getSubmitterReputation/shouldSkipAiForReputation is called exactly ONCE per pass in the common (no-override) case; a regression test reproducing the manifest-override divergent-config edge case, confirming the threaded value doesn't silently override the callee's own more-specific check when they'd legitimately disagree.

Deliverables

  • Reputation check result threaded from caller to callee instead of re-derived
  • Divergent-config edge case still behaves correctly
  • Invariant test: single call per pass in the common case
  • Regression test: manifest-override divergence still resolves correctly

Expected outcome

One getSubmitterReputation D1 read per pass instead of two, for every reputation-enabled, allowlisted repo — a small but real reduction in redundant bounded DB reads.

References

  • src/queue/processors.ts:6815-6829, 7142-7175, 9471-9483 (both call sites + the shared aiReviewWillRun gate)
  • src/review/feature-activation.ts:48-63 (resolveConvergedFeature)
  • src/review/reputation-wire.ts:62-69 (shouldSkipAiForReputation, no memoization)
  • src/review/submitter-reputation.ts:71, 223-258 (getSubmitterReputation, the underlying scan)

Effort

XS

Metadata

Metadata

Assignees

Labels

gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.maintainer-onlyOwner-only work — yields no Gittensor points.orbGittensory Orb related - maintainer self-hosting analytics.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions