fix(selfhost): warn at boot when the public-score-terms allowlist is unset - #9437
Conversation
…unset The #public-score-terms-scoping fix is config-dependent: the code path shipped and is correctly wired, but stays inert until an operator sets LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS. Unset (the shipped default, and the state found in production) the bare-`score` public-comment check is enforced for every repo, so any AI review narrative using the ordinary word "score"/"scoring" has its WHOLE assessment discarded in favour of the generic no-narrative placeholder -- exactly the metagraphed#8038 behaviour the exemption was written to fix, with no signal anywhere. No unit test can catch this class: tests verify the mechanism works WHEN enabled, never that a production env var was actually set. Shout once at boot instead, mirroring shouldWarnRagEmbedUnavailable (pure predicate here, the console.error in server.ts). Warn-only, not a hard boot failure: an empty allowlist is the correct configuration for a deployment whose repos genuinely carry private trust/reward data -- the operator just needs to make that an informed choice rather than an unnoticed default.
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-27 19:07:51 UTC
Review summary Nits — 1 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9437 +/- ##
==========================================
- Coverage 89.53% 88.63% -0.91%
==========================================
Files 843 843
Lines 109959 109960 +1
Branches 26184 26184
==========================================
- Hits 98457 97467 -990
- Misses 10239 11521 +1282
+ Partials 1263 972 -291
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
Closes part of #9433.
The
#public-score-terms-scopingfix is config-dependent: the code path shipped and is correctly wired atai-review.ts(allowBareScoreTerm: isPublicScoreTermSafeForRepo(env, input.repoFullName)), but it stays inert until an operator setsLOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS.Unset is the shipped default, and it was the state found in production. In that state the allowlist fails closed, so
BARE_SCORE_TERM_PATTERN = /\bscore\w*\b/iis enforced for every repo.sanitizePublicCommentthrows on a match andtoPublicSafecatches it and returnsnull— all-or-nothing — so any AI review narrative containing score / scores / scoring / scored had its entire assessment discarded in favour of the generic "did not include a separate narrative summary" placeholder. Nits survived, because those are filtered individually.That is exactly the behaviour
composeAdvisoryNotes' own doc comment already documents:So the fix shipped, was considered done, and then sat inert with no signal anywhere.
Why a warning rather than a test
No unit test can catch this class. The existing tests verify the mechanism works when enabled; they cannot assert that a production env var was actually set. The failure mode is a correct code path plus an unset variable, which is invisible to CI by construction. Shouting once at boot is the only place this is observable.
Design
Mirrors the existing
shouldWarnRagEmbedUnavailableprecedent exactly: a pure predicate inqueue-intelligence.ts(next to the exemption it guards, so the two can never disagree about what "configured" means), and a single structuredconsole.errorat the boot site inserver.ts.Warn-only, deliberately not a hard boot failure. An empty allowlist is the correct configuration for a deployment whose repos genuinely carry private trust/reward data — the operator just needs to make that an informed choice rather than an unnoticed default.
Test plan
isPublicScoreTermSafeForRepoagree on what counts as configured — if they drift, the shout either fires for a configured deployment (noise) or stays silent for an unconfigured one (the failure it exists to catch)"*"case: it parses as a non-empty entry so the warning is silent, but it grants no repo the exemption (no wildcard escape hatch)npm run typecheckcleanNote on scope
An earlier draft of this branch also changed
hasPublicReviewAssessmentto reject the persisted no-narrative placeholder. That was reverted: it made the orchestration hold PRs for manual review and discard the model's nits, which two existing tests correctly pin as unwanted. See the correction on #9432.