Skip to content

fix(review): bound AI review re-spend and public-surface republish on unchanged heads - #2667

Merged
JSONbored merged 2 commits into
mainfrom
claude/reverent-curran-5f2f61
Jul 3, 2026
Merged

fix(review): bound AI review re-spend and public-surface republish on unchanged heads#2667
JSONbored merged 2 commits into
mainfrom
claude/reverent-curran-5f2f61

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Scheduled re-gate sweeps were repeatedly re-spending a full AI review (and republishing the public surface) on the same PR/head even though nothing had changed, on low-activity repos. Production evidence for one 6h window on one repo: 97 agent.sweep.regate events, 225 github_app.pr_public_surface_published events, 184 ai_review_pr usage events — for "the same few open PRs ... despite virtually no repo activity."

Root cause (confirmed against production Postgres data, not just code inspection): the durable ai_review_cache (keyed on repo+PR+head SHA+mode) is correct and was being consulted, but two things bypassed it entirely with no throttle:

  1. A repo with an active dynamic-context review feature (grounding/RAG/enrichment/reputation) unconditionally bypassed the cache on every single call — by design, since that external context can drift for an unchanged head. For the incident PR, RAG was enabled and this accounted for ~92% of the 281 calls in 24h at one unchanged head.
  2. A genuinely non-cacheable outcome (consensus defect / inconclusive / a lock-contention placeholder) is, correctly, never written to the cache — but nothing then throttled how often it gets retried, so a PR stuck in that state got re-reviewed on every sweep tick, forever.

The prior fix in this repo (#2639, isReviewsCacheUpToDate) addressed a different cache — the GitHub-reviews-data cache, not this AI-review-result cache — so it did not touch this behavior.

Fix

  • ai_review_cache gains a cacheable column (migration 0098). A non-cacheable outcome is now persisted but marked non-durable, reusable only within a bounded 30-minute cooldown (AI_REVIEW_NON_CACHEABLE_RETRY_COOLDOWN_MS) — collapsing a sweep tick's worth of redundant LLM calls into one, while still periodically retrying (an LLM's own non-determinism, or a maintainer fix, may resolve the dispute) and never treating the result as durably trustworthy.
  • The dynamic-context bypass is now bounded the same way instead of unconditional — a repo with grounding/RAG/enrichment/reputation active reuses its last review for the cooldown window, then refreshes, rather than paying for a fresh LLM call on every single pass.
  • A lock-contention placeholder ("another pass is already reviewing this head") is explicitly excluded from persistence at all (persistable: false) — it's a transient scheduling artifact, not a real verdict, and replaying it after the concurrent pass finished would be actively wrong.
  • A cache write failure is now observable: .catch(() => undefined) is replaced with an audit event (github_app.ai_review_cache_write_error) + counter, instead of a silent no-op.
  • New audit events + Prometheus counters for direct operational visibility: ai_review_cache_hit/ai_review_cache_miss/ai_review_cache_write_error/ai_review_non_cacheable, agent.sweep.regate_ai_skipped_current, github_app.public_surface_publish_skipped_current.
  • A narrow, deliberately conservative public-surface no-op guard: for a check-run-only repo (publicSurface: "off"), skip republishing when the head matches the last published surface and an independently-verified completed check run already exists at that exact head and the AI dimension was reused, not fresh. markPullRequestSurfacePublished's own doc comment warns the stored marker alone is "reporting/diagnostic state, not a hard scheduled-sweep skip" (a comment can be stale/partial even when it matches) — so this guard is scoped to the one surface (the check run) GitHub itself gives an authoritative signal for, and any doubt falls through to a full republish. Comment/label republishing is intentionally left untouched.
  • agent-regate-pr jobs carry an optional force flag (unused by any current caller) that bypasses both the cache and the cooldown, so a future manual re-gate trigger has a supported way to force a fresh opinion.

Scope

  • Stayed within wantedPaths (src/, test/, migrations/)
  • No secrets, wallets, hotkeys, trust scores, or reward values anywhere
  • No changes to site/, CNAME, **/lovable/**, or CHANGELOG.md
  • Traced ≥2 existing analogues before writing: getCachedAiReview/putCachedAiReview (src/db/repositories.ts), the github_app.miner_detection_cache_hit/miss audit-event pair, and surfaceRepairPriorityPullNumbers's existing completed-check-run verification pattern

Validation

  • npm run typecheck
  • npm run db:migrations:check
  • npm run test:coverage (unsharded) — 100% line+branch coverage on every changed line in src/db/repositories.ts and src/queue/processors.ts, verified by diffing changed lines against the v8 coverage map directly
  • npm run test:workers
  • npm run ui:lint / ui:test (unaffected, no UI changes)
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • npm run actionlint, cf-typegen:check, selfhost:env-reference:check, build:mcp, test:mcp-pack, ui:openapi:check, ui:openapi:settings-parity, ui:version-audit
  • Cross-checked the fix against live production Postgres (audit_events, ai_usage_events, ai_review_cache) on the self-host VPS before writing the fix, to confirm the actual root cause rather than guessing from code alone

New/updated tests (test/unit/ai-review-cache.test.ts, test/unit/queue.test.ts, tagged #regate-churn):

  • A scheduled sweep does not call AI twice for a non-cacheable outcome at an unchanged head (reproduces the production incident shape)
  • A cache write failure is observable via audit_events and counters, not silently swallowed
  • The public surface is not republished when already current at the head; falls through when the surface marker matches but no completed check run backs it up (the documented partial-publish edge case); fails open on a failed check-run read
  • A changed head still triggers a fresh AI review even within the cooldown window
  • A manual force re-gate bypasses the cache and cooldown
  • A low-activity repo's PR does not generate a repeated AI review on every one of many sweep ticks, at both loose (beyond-cooldown) and tight (within-cooldown) tick spacing
  • The dynamic-context (grounding/RAG) test that previously asserted an unconditional, unbounded re-run is now updated to assert the new bounded-cooldown behavior instead

Safety

  • No secrets/wallets/hotkeys/trust-scores/reward values in code, tests, or this description
  • The public-surface guard fails open (falls through to a full republish) on any ambiguity — verified with a dedicated "no completed check run" test and a "check-run read fails" test
  • Manual re-gate / webhook-triggered reviews on a real state change (new head, changed review-input fingerprint) still bypass the cooldown immediately, regardless of age — verified with a dedicated test

… unchanged heads

Scheduled re-gate sweeps were re-spending a full AI review on every pass for a
PR whose outcome landed in a non-cacheable state (consensus defect /
inconclusive / a dynamic-context repo with grounding or RAG enabled) — the
durable ai_review_cache correctly never stores those outcomes, so nothing
throttled the retry. Root-caused in production: one PR generated 281 AI
review calls in 24h at an unchanged head, ~92% of it from the RAG-active
unconditional-bypass path.

- ai_review_cache gains a `cacheable` column; a non-cacheable outcome (and a
  dynamic-context result, now bounded rather than unconditionally bypassing
  the cache) is still persisted for a 30-minute cooldown reuse, never as a
  durable hit. A lock-contention placeholder is still never persisted at all.
- New audit events + counters: ai_review_cache_hit/miss/write_error,
  ai_review_non_cacheable, agent.sweep.regate_ai_skipped_current,
  github_app.public_surface_publish_skipped_current. A cache write failure is
  now observable instead of a silent catch.
- A narrow public-surface no-op guard skips republishing a check-run-only
  repo's completed check when nothing provably changed since the last pass
  (head match + a live-verified completed check run + no pending refresh
  signal), falling through to a full republish on any doubt.
- `agent-regate-pr` jobs carry an optional `force` flag that bypasses both
  the cache and the cooldown for an explicit manual re-gate.

Validated against production Postgres audit_events/ai_usage_events data for
the incident repo/PR before and during the fix.
@dosubot dosubot Bot added the size:L label Jul 3, 2026
@loopover-orb

loopover-orb Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 02:39:20 UTC

7 files · 1 AI reviewer · 1 blocker · readiness 93/100 · CI green · blocked

⏸️ Suggested Action - Manual Review

  • Possible leaked secret in the diff (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

Review summary
The change correctly adds a `cacheable` bit to `ai_review_cache`, persists non-durable AI outcomes for bounded reuse, threads `force` through re-gate jobs, and adds a narrow public-surface no-op guard for check-run-only repos. The schema and migration are in parity, the migration is D1-safe, and the tests cover the key reachable paths: non-cacheable cooldown reuse, force bypass, dynamic-context bounded reuse, write-error observability, and public-surface skip/fail-open behavior. I do not see a visible correctness blocker in the changed hunks.

Nits — 5 non-blocking
  • nit: `src/queue/processors.ts:5850` says no current caller sets `forceAiReview`, but this diff now threads `options.force` into that field, so tighten the comment to say no production/manual-trigger producer currently enqueues it if that is the intended distinction.
  • nit: `src/queue/processors.ts:6631` records forced cache bypasses under `gittensory_ai_review_cache_miss_total`, which makes the metric less precise; consider a separate forced-bypass counter or metadata field so real misses and intentional bypasses are distinguishable.
  • nit: `test/unit/queue.test.ts:2248` adds a lot of repeated repo setup, fetch stubs, and fingerprint construction; extracting local helpers would make the new regression cases easier to audit and less brittle.
  • In `src/queue/processors.ts:6657`, split cache miss telemetry from explicit `forceAiReview` bypass telemetry so incident dashboards can tell whether the cache failed to serve or was deliberately skipped.
  • In `src/queue/processors.ts:5850`, update the stale `forceAiReview` comment to match the new `reReviewStoredPullRequest(..., { force })` wiring.

Concerns raised — review before merging

  • Possible leaked secret in the diff (generic_secret_assignment) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.
Signal Result Evidence
Code review ❌ 1 blocker 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 (size label size:L; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 61 registered-repo PR(s), 52 merged, 506 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 61 PR(s), 506 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: not available
  • Official Gittensor activity: 61 PR(s), 506 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.
  • No action.
  • 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 gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 3, 2026
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.11%. Comparing base (cfbdc9f) to head (44acb15).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2667   +/-   ##
=======================================
  Coverage   96.10%   96.11%           
=======================================
  Files         237      237           
  Lines       26540    26582   +42     
  Branches     9625     9640   +15     
=======================================
+ Hits        25507    25549   +42     
  Misses        424      424           
  Partials      609      609           
Files with missing lines Coverage Δ
src/db/repositories.ts 96.60% <100.00%> (+0.01%) ⬆️
src/db/schema.ts 69.46% <ø> (ø)
src/queue/processors.ts 92.70% <100.00%> (+0.12%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 3, 2026
…ypass telemetry

The new test fixtures reused the pre-existing "installation-token" literal
verbatim; since this is the first time those specific lines appear as new
diff content, the scanner flags it the same way #2639 already worked around
this exact false positive — rename the new occurrences to the established
"fake-installation-token" convention.

Also: a forced re-gate bypass was being counted under the cache-miss
metric/audit, conflating "the cache had nothing to serve" with "a caller
explicitly opted out" — split it into its own
gittensory_ai_review_force_bypass_total counter and
github_app.ai_review_force_bypass audit event, and tighten the stale
forceAiReview comment.
@JSONbored
JSONbored merged commit c44dd33 into main Jul 3, 2026
12 checks passed
@JSONbored
JSONbored deleted the claude/reverent-curran-5f2f61 branch July 3, 2026 05:12
JSONbored added a commit that referenced this pull request Jul 3, 2026
…boot (#2686)

* fix(review): add the missing securityFocus field to 5 AI-review-cache test fixtures

#2675 (feat(review): add a security-focused review profile toggle) added
securityFocus as a required AiReviewCacheInput field after these fixtures
(from #2667) were written, breaking npm run typecheck on main for anyone
branching fresh off it.

* fix(selfhost): reject known-placeholder and weak critical secrets at boot

.env.selfhost.example shipped ENABLED (not commented-out) placeholder values
for GITHUB_WEBHOOK_SECRET, GITTENSORY_API_TOKEN, GITTENSORY_MCP_TOKEN,
INTERNAL_JOB_TOKEN, and SELFHOST_SETUP_TOKEN. An operator who copies the
starter to .env per the quickstart docs and misses "fill in the placeholders"
runs an instance with a PUBLICLY KNOWN webhook HMAC secret (forgeable
signatures) and PUBLICLY KNOWN static bearer tokens -- GITTENSORY_API_TOKEN
authenticates as the server-to-server actor and bypasses per-repo write
checks, INTERNAL_JOB_TOKEN gates internal routes -- silently, with no error
at boot or runtime.

- The boot-time preflight check (already gates server.ts's main(), throwing
  before the process starts serving) now rejects any of the five critical
  secrets that is set to the exact known-placeholder string, or that is
  merely too short to be a real generated secret, or that duplicates another
  critical secret's value. Presence is still each secret's own concern (most
  are feature-gating, not universally required) -- this only judges
  STRENGTH whenever one is actually set, so it can never be silently bypassed
  by leaving the file's placeholder in place.
- .env.selfhost.example now ships these five lines commented out, with
  explicit per-secret generation guidance, instead of enabled placeholders.
- The quickstart doc callout it directed users through now explicitly warns
  about generating distinct random values for each secret.

Defense in depth: the docs + example file guide an operator toward doing the
right thing, and the preflight check makes doing the wrong thing impossible
rather than merely discouraged.
JSONbored added a commit that referenced this pull request Jul 3, 2026
… test fixtures (#2684)

#2675 (feat(review): add a security-focused review profile toggle) added
securityFocus as a required AiReviewCacheInput field after these fixtures
(from #2667) were written, breaking npm run typecheck on main for anyone
branching fresh off it.
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

Development

Successfully merging this pull request may close these issues.

1 participant