fix(review): scope the bare-score public-safety check per repo (metagraphed#8038)#8562
Conversation
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…raphed#8038) sanitizePublicComment throws (never redacts) whenever public-facing AI text matches a bare \bscore\w*\b, to stop loopover's own private gittensor miner trust/reward VALUES from leaking. toPublicSafe catches the throw and returns null, and composeAdvisoryNotes silently discards the whole narrative assessment in favor of the generic "did not include a separate narrative summary" fallback. This filter applied identically across every repo the shared engine serves, but metagraphed's own public schema legitimately has fields named totalScore/credibility/baseTotalScore -- any AI review that naturally describes this code in prose is near-certain to trip the filter and lose its whole write-up. Observed live, recurring. sanitizePublicComment gains an optional allowBareScoreTerm flag, default false (unchanged behavior for every existing caller). Every OTHER forbidden phrase (wallet, hotkey, coldkey, trust score, reward, scoreability, ...) stays enforced unconditionally regardless of this flag -- only the over-broad bare-word check is ever relaxable. New isPublicScoreTermSafeForRepo resolves a fail-closed, config-as-code repo allowlist (LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS) with no '*'/'all' wildcard, since a blanket opt-in would silently cover a future repo that DOES have private trust/reward data. composeAdvisoryNotes threads the flag through to its 3 relevant call sites only; the other 11 toPublicSafe call sites in ai-review.ts are untouched. Additive and inert until an operator sets the env var for a specific repo.
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-24 21:22:44 UTC
Review summary Nits — 5 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision 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. 🟩 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.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8562 +/- ##
==========================================
+ Coverage 89.58% 91.93% +2.35%
==========================================
Files 97 795 +698
Lines 22706 79669 +56963
Branches 3872 24075 +20203
==========================================
+ Hits 20341 73246 +52905
- Misses 2187 5309 +3122
- Partials 178 1114 +936
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ate stays unknown refreshLiveMergeState's own short inline retry (previous commit, ~4s total) resolves most GitHub mergeable_state "still computing" reads within the same pass, but not all -- on a large PR or under load, GitHub can take longer. Without this, the only remaining catch-up was the periodic agent-regate-sweep, which is NOT a reliable fixed interval: it throttles under GitHub REST-budget backpressure and skips re-arming while a previous sweep trigger is still in flight (index.ts's own backpressure comment) -- under sustained cross-repo load this can stretch well past minutes, during which an overlapping sibling PR can land first and base-conflict the original PR out from under it (observed live, metagraphed#8037). Schedules ONE targeted, single-PR follow-up (60s delay) specifically when a PR holds because mergeableState is "unknown" -- the one genuinely transient value; "dirty"/"blocked"/"behind" are real, stable holds a re-check moments later would only reproduce, so they're deliberately excluded. Reuses the exact delayed-JOBS.send + check-then-claim-after-success dedup pattern scheduleTrailingIssueLinkedReReview already established in this file for an analogous problem, not a new mechanism. Also closes a Codecov branch-coverage gap on the prior score-terms commit: composeAdvisoryNotes' isPublicScoreTermSafeForRepo call site had only its false arm exercised end-to-end through runLoopOverAiReview; added the companion allowed-repo case.
afc09c0 to
f60a14c
Compare
… (Codecov branch gap) The composeAdvisoryNotes(...) ?? composeFallbackAdvisoryNotes(fallbackNotes) line's right-hand branch was never exercised through runLoopOverAiReview -- the prior score-terms tests only ever hit composeAdvisoryNotes' own internal non-null fallback text, never this outer nullish-coalescing fallthrough. Mirrors the existing "composeAdvisoryNotes returns null when no assessment or finding is public-safe" unit fixture, driven end-to-end instead.
Summary
Live, recurring issue: metagraphed#8038 (and many others) got "The AI review returned non-blocking notes for this change but did not include a separate narrative summary" instead of a real review write-up, despite the model producing one.
Root cause:
sanitizePublicCommentthrows (by design — never redacts) whenever public-facing AI text matches a bare\bscore\w*\b, to stop loopover's own private gittensor miner trust/reward values from leaking.toPublicSafecatches that throw and returnsnull, andcomposeAdvisoryNotessilently discards the entire narrative assessment in favor of the generic fallback text. This filter is applied identically across every repo the shared engine serves — but metagraphed's own public schema legitimately has fields namedtotalScore,credibility,baseTotalScore, etc. Any AI review that naturally describes this code in prose ("the resolver filters results by their score...") is near-certain to trip the filter and lose its whole write-up.Fix, scoped as narrowly as possible:
sanitizePublicCommentgains an optional{ allowBareScoreTerm?: boolean }, defaultfalse(today's unchanged behavior for every existing caller that doesn't pass it). Every other forbidden phrase —wallet,hotkey,coldkey,trust score,reward,scoreability, etc. — stays enforced unconditionally, everywhere, regardless of this flag. Only the over-broad bare-word check is ever relaxable, and only where a caller has positively confirmed it's safe.isPublicScoreTermSafeForRepo(env, repoFullName): a fail-closed (deny-by-default), config-as-code repo allowlist (LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS, comma/whitespaceowner/repolist). Unset ⇒ the check stays enforced for every repo, unchanged from today. Deliberately no*/allwildcard (unlike the MCP allowlists) — this is a safety-relevant exemption, and a blanket opt-in-everything value would silently cover a future repo that does have private trust/reward data.composeAdvisoryNotesthreads the flag through to its 3 relevanttoPublicSafecalls (assessment, blockers, nits) only — the other 11toPublicSafecall sites inai-review.ts(blocker titles, tie-break judge output, consensus titles) are untouched.runLoopOverAiReviewresolves the flag once, fromisPublicScoreTermSafeForRepo(env, input.repoFullName).This is additive and inert until an operator sets the env var for a specific repo — no behavior changes for any repo today.
Validation
queue-intelligence.test.ts: the option passes a bare "score" through when true; still throws when false; every OTHER forbidden phrase still throws even when the flag is true;isPublicScoreTermSafeForRepo's allow/deny/no-wildcard behavior.ai-review.test.ts: a bare "score" mention drops the whole assessment by default but survives when allowlisted (with a surviving, score-free nit proving the drop doesn't silently empty unrelated findings); the explicit-phrase bans (raw trust score) stay enforced even with the flag on.npm run typecheckclean;npm run test:coverage(full, unsharded): all tests passing.