Skip to content

feat(review): enforce an aggregate token budget across the AI review prompt - #3941

Merged
JSONbored merged 1 commit into
mainfrom
feat/aggregate-review-prompt-token-budget
Jul 7, 2026
Merged

feat(review): enforce an aggregate token budget across the AI review prompt#3941
JSONbored merged 1 commit into
mainfrom
feat/aggregate-review-prompt-token-budget

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • buildUserPrompt concatenates six independently-capped context sections (grounding, RAG, impact-map, enrichment, culture-profile, test-evidence). Every section enforces its own cap (FILE_CONTENT_BUDGET, MAX_CONTEXT_CHARS, MAX_PROMPT_CHARS, MAX_ENRICHMENT_PROMPT_SECTION_CHARS...), but nothing bounded the combined total: with every convergence feature enabled on one repo, the worst-case assembled prompt exceeds 200,000 characters before the system prompt is even added.
  • The only place the combined length was consulted at all was estimateNeurons(system.length + user.length, ...) — a cost-quota estimate for billing accounting, with zero effect on what's actually sent to the model.
  • Added selectContextSectionsWithinBudget: a priority-ordered hard cutoff. Sections are evaluated highest-priority-first (grounding > RAG > impact-map > enrichment > culture-profile > test-evidence); once one would push the running total over the ceiling, that section AND every lower-priority one after it are dropped. This is a predictable priority cutoff, not a bin-packing optimization that could skip a large blocked section to squeeze in a smaller, lower-priority one.
  • AGGREGATE_CONTEXT_BUDGET_CHARS = 200_000 sits comfortably above diff+description+grounding's own worst case (~182k chars: 120k diff + 2k description + 60k grounding), so grounding is effectively never trimmed. Normal single/few-feature repos stay byte-identical since their combined total never approaches the ceiling — this only changes behavior for the genuine "every feature enabled together" case.

Found via a fresh performance/scalability/accuracy hardening audit of the self-host ORB stack. Tracked under #1667.

Scope

  • src/services/ai-review.tsselectContextSectionsWithinBudget + wiring into buildUserPrompt
  • test/unit/ai-review.test.ts — unit tests for the cutoff helper (inclusion, hard-cutoff-not-bin-packing, absent-section handling, exact boundary) + integration tests via buildUserPrompt (byte-identical when under budget, correct priority-ordered trimming when over, grounding never trimmed at its own real-world max)

Validation

  • npm run typecheck
  • npx vitest run test/unit/ai-review.test.ts test/unit/ai-review-advisory.test.ts test/unit/ai-review-cache.test.ts test/unit/ai-review-cache-input.test.ts --coverage — 248/248 passing; changed lines fully covered (remaining uncovered lines are pre-existing and untouched by this diff)
  • npm run test:coverage (full unsharded) — 560 files / 11115 tests passed
  • git diff --check clean

Safety

  • No behavior change for the common case (single/few features enabled — never approaches the ceiling). No secrets. Purely a prompt-assembly change; the gate/consensus logic downstream is unaffected.

Closes #3900

…prompt

buildUserPrompt concatenated six independently-capped context
sections (grounding, RAG, impact-map, enrichment, culture-profile,
test-evidence) with no combined limit. Every section enforces its own
cap, but with every convergence feature enabled on one repo the
worst-case assembled prompt exceeds 200,000 characters before the
system prompt is even added -- degrading signal-to-noise on exactly
the large/complex PRs that most need focused attention. The only
place the combined length was consulted was the neuron-budget cost
estimate, which affects billing accounting, not what's actually sent.

Add a priority-ordered cutoff (grounding > RAG > impact-map >
enrichment > culture-profile > test-evidence): walk sections
highest-priority-first, including each while it fits, and stop
entirely once one would overflow -- a predictable priority cutoff,
not a bin-packing optimization. The 200,000-char ceiling sits
comfortably above diff+description+grounding's own worst case (~182k)
so grounding is effectively never trimmed; normal single/few-feature
cases stay byte-identical since they never approach the ceiling.

Closes #3900
@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 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:37:55 UTC

2 files · 1 AI reviewer · no blockers · readiness 100/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

Review summary
This adds a priority-ordered hard cutoff (selectContextSectionsWithinBudget) that caps the combined length of six independently-capped optional prompt sections at 200,000 chars, wired into buildUserPrompt so lower-priority sections (test-evidence, culture-profile, enrichment, impact-map) are dropped in order once the running total would overflow, while grounding/RAG (highest priority) survive even in the worst-case all-features-enabled scenario. The arithmetic is correct (the +2-char accounting for `lines.push("", text)` matches how the array-join actually grows), and the new tests genuinely exercise the real `buildUserPrompt` path with realistic section sizes (60k grounding, 120k diff) rather than fabricating impossible states. The one inconsistency worth a fix is that the priority order used to decide inclusion (grounding, rag, impactMap, enrichment, cultureProfile, testEvidence) doesn't match the physical push order in buildUserPrompt (grounding, rag, impactMap, cultureProfile, enrichment, testEvidence) — harmless to the budget total since summation is order-independent, but a maintenance trap if someone assumes the two orders are the same.

Nits — 6 non-blocking
  • src/services/ai-review.ts: the section evaluation order passed to selectContextSectionsWithinBudget (enrichment before cultureProfile) doesn't match the physical `lines.push` order in buildUserPrompt (cultureProfile before enrichment) — doesn't break the budget math but is confusing given the comment claims a single documented priority order; consider driving both from one shared ordered array to prevent future drift.
  • src/services/ai-review.ts: `usedChars` is computed via `lines.join("\n").length` before evaluating sections, which is fine today but re-joins the whole `lines` array again at the end of the function — negligible cost given the array is small and diff is pre-sliced, but worth a one-line comment noting the double-join is intentional.
  • external brief flags the file's overall line count (2235 lines); not something this diff need fix, but if this pattern of appending yet another optional section keeps growing, ai-review.ts is due for a split into a dedicated prompt-assembly module.
  • Extract the six `{key, text}` priority entries into a single named `const CONTEXT_SECTION_ORDER = [...]` array reused for both the `selectContextSectionsWithinBudget` call and the sequence of `lines.push` calls, so the budget-evaluation order and the render order can never diverge (src/services/ai-review.ts:~721-760).
  • Consider asserting in a test that the render order in the final prompt string matches the declared priority order (grounding text appears before rag text, etc.) — this would have caught the enrichment/cultureProfile swap.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3900
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: 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 ✅ Passing No configured 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.
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

@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.68%. Comparing base (70c4396) to head (b01b6a7).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3941   +/-   ##
=======================================
  Coverage   93.68%   93.68%           
=======================================
  Files         372      372           
  Lines       34891    34902   +11     
  Branches    12767    12769    +2     
=======================================
+ Hits        32688    32699   +11     
  Misses       1584     1584           
  Partials      619      619           
Files with missing lines Coverage Δ
src/services/ai-review.ts 97.12% <100.00%> (+0.05%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit 5628d2e into main Jul 7, 2026
10 checks passed
@JSONbored
JSONbored deleted the feat/aggregate-review-prompt-token-budget branch July 7, 2026 07:57
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.

Development

Successfully merging this pull request may close these issues.

feat(review): enforce an aggregate token budget across buildUserPrompt's context sections

1 participant