fix(review): default reviewEvasionProtection to close, not off (#4011)#4038
Conversation
A repo that never discovers and explicitly sets reviewEvasionProtection got ZERO self-close/draft-dodge/repeated-cycling protection under the old "off" default -- a real, already-exploited gaming vector (see the draft-cycling incident this session root-caused and fixed for our own 3 repos via explicit private-config overrides). The fix is NOT a migration changing the schema.ts column default, which would have zero effect: upsertRepositorySettings is the only writer and always resolves an explicit value through normalizeReviewEvasionProtection, so the raw SQLite column-level default is never reached by any live write path (the identical lesson migration 0102 already documented for linked_issue_gate_mode -- SQLite has no ALTER COLUMN SET DEFAULT, and rebuilding the table for a default nothing reads isn't worth the risk). The actually-reachable default lives in three places, all flipped together for consistency: - normalizeReviewEvasionProtection (src/db/repositories.ts): the shared read+write normalizer both getRepositorySettings and upsertRepositorySettings resolve through. Only the explicit opt-out "off" is now honored; anything else (including undefined/garbage) resolves to "close". - getRepositorySettings' own "no row exists yet" fallback object, used before a repo's first-ever settings write. - Three independent `?? "off"` fallbacks in src/queue/processors.ts (maybeCloseReviewEvasionSelfClose, maybeCloseReviewEvasionDraftConversion, the repeated-draft-cycling guard) that defended against an undefined settings value with the SAME old default, entirely independent of the DB layer -- fixing only the DB side would have left these three still silently unprotected against a genuinely undefined value. Left the schema.ts column-level DEFAULT untouched (with a comment explaining why, linking to migration 0102's precedent) rather than risk a full table-rebuild migration for a value no live path reads.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-07 12:45:35 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4038 +/- ##
=======================================
Coverage 93.58% 93.58%
=======================================
Files 381 381
Lines 35504 35504
Branches 13018 13018
=======================================
Hits 33225 33225
Misses 1617 1617
Partials 662 662
🚀 New features to boost your workflow:
|
Summary
Closes #4011.
reviewEvasionProtectiongot ZERO self-close/draft-dodge/repeated-cycling protection under the old"off"default — a real, already-exploited gaming vector.schema.tscolumn default) would have zero effect:upsertRepositorySettingsis the only writer and always resolves an explicit value throughnormalizeReviewEvasionProtection, so the raw SQLite column-level default is never reached by any live write path — the identical lesson migration0102already documented for a sibling field (linked_issue_gate_mode). SQLite has noALTER COLUMN SET DEFAULT, and rebuilding the table for a default nothing reads isn't worth the operational risk.normalizeReviewEvasionProtection(src/db/repositories.ts) — the shared read+write normalizer. Only the explicit opt-out"off"is now honored; anything else resolves to"close".getRepositorySettings's "no row exists yet" fallback object.?? "off"fallbacks insrc/queue/processors.tsthat defended against an undefined settings value with the same old default, entirely independent of the DB layer.schema.tscolumn-levelDEFAULTuntouched with an explanatory comment (linking to migration 0102's precedent) so a future reader doesn't independently "fix" it for zero effect.Our own 3 repos are unaffected either way — they already have
reviewEvasionProtection: closeexplicitly set in the private VPS config, which wins over any DB value regardless of this change. This protects every other repo/deployment (and any future repo we register) that hasn't discovered this field.Scope
wantedPaths(src/,test/)Validation
npm run typecheck— cleannpx vitest run test/unit/moderation-config-db.test.ts test/unit/queue.test.ts test/unit/focus-manifest.test.ts test/unit/agent-action-executor.test.ts— 1372/1372 passedSafety
site/,CNAME,lovable/, or the changelog