fix(review): make the safety feature override force-on-only - #2386
Conversation
resolveConvergedFeature's precedence let a per-repo features.safety override force the prompt-injection defense OFF even when the operator enabled it globally. .gittensory.yml lives in the repo itself, writable by a lower-trust actor than the operator, so a repo owner (or anyone with push access) could silently defeat a security control the operator believed was instance-wide, with no warning. Special-case safety in resolveConvergedFeature: an override can only tighten (force-on), never loosen. false is now treated as "no opinion" and falls through to the allowlist default instead of forcing the feature off. rag/reputation/unifiedComment are unaffected — their override still wins in both directions.
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-01 19:21:25 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 3 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 #2386 +/- ##
=======================================
Coverage 95.71% 95.71%
=======================================
Files 222 222
Lines 24661 24662 +1
Branches 8949 8950 +1
=======================================
+ Hits 23605 23606 +1
Misses 433 433
Partials 623 623
🚀 New features to boost your workflow:
|
What
resolveConvergedFeature's precedence for thesafetyconverged feature (prompt-injection defanging viadefangReviewInput) was: global env kill-switch → per-repofeatures.safetyoverride (either direction) → allowlist default. Since.gittensory.ymllives in the repo itself — writable by a lower-trust actor than the self-host operator — a repo owner (or anyone with push access to a repo) could setfeatures.safety: falseand silently defeat a security control the operator explicitly turned on instance-wide, with no warning surfaced anywhere. This is distinct fromsecret_leak, which is a separate, already-unconditional mechanism.Fix
resolveConvergedFeature(src/review/feature-activation.ts) now special-casessafety: an override can only tighten (force it on), never loosen it.features.safety: falseis treated as "no opinion" and falls through to theGITTENSORY_REVIEW_REPOSallowlist default instead of forcing the feature off. The master env kill-switch is checked first as before, so atrueoverride still can't turn safety on when the operator hasn't enabled it globally.rag,reputation, andunifiedCommentare untouched — their per-repo override still wins in both directions, since they aren't security-critical and the issue's acceptance criteria explicitly calls for their existing flexibility to be preserved.Went with the primary recommended fix (precedence inversion) rather than the issue's fallback (an
audit_eventswarning on opt-out):resolveConvergedFeatureis a deliberately pure, synchronous, no-I/O function (per its own doc comment) reused across many hot-path call sites, so bolting an async DB write onto it would be a real architecture change for a feature this narrow. Inverting the precedence closes the gap completely rather than just making it visible.Tests
resolveConvergedFeature: afalseoverride is ignored (falls through to the allowlist default, both allowlisted and not), atrueoverride still force-activates even when not allowlisted, and the master kill-switch still wins over atrueoverride.convergedFeatureActiveDB-backed path, confirming the fix holds through the full stack.feature-activation.test.tstests (which exerciseragspecifically, plus an all-features sweep that never happens to hitsafetyas the "other feature should stay off" case) pass unchanged, confirming the other three features are unaffected.npx tsc --noEmitclean.feature-activation.test.ts— 12 passed.safety-wiring.test.ts,safety.test.ts,ai-review.test.ts,ai-review-advisory.test.ts,gate-check-policy.test.ts(the tests most likely to exercise this path) — 214 passed.src/review/feature-activation.ts: fully covered.npm run test:coverage: 5604 passed, 4 skipped (pre-existing/unrelated), 0 failed.npm audit --audit-level=moderate: 0 vulnerabilities.Advances #1936. Closes #2269.