fix(review): explain validation and guardrail holds#3304
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review result - approve/merge recommendedReview updated: 2026-07-05 05:21:20 UTC
✅ Suggested Action - Approve/Merge
Review summary Nits — 5 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 #3304 +/- ##
==========================================
+ Coverage 93.88% 93.90% +0.01%
==========================================
Files 282 282
Lines 30570 30588 +18
Branches 11136 11143 +7
==========================================
+ Hits 28702 28724 +22
+ Misses 1211 1208 -3
+ Partials 657 656 -1
🚀 New features to boost your workflow:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | 3be6bfa | Commit Preview URL Branch Preview URL |
Jul 05 2026, 02:36 AM |
d8bf71d to
fa3efae
Compare
fa3efae to
0ebfd63
Compare
… gaps hasValidationNote matched any mention of testing regardless of polarity, so a PR body like "No tests run" or "Tests not run" satisfied a configured manifest test expectation and let manifest_missing_tests disappear on a reachable live gate path. Add a negation guard covering both word orders before falling through to the existing affirmative match. Thread the same helper into the pre-submission predictor so it stops predicting stricter than the live gate on this exact finding, and close the codecov/patch gap: cover the truncated guardrail-path list, the all-unsafe test-expectations case, and a null PR body through the live webhook path. Simplify the redundant guardrailReason fallback in agent-actions.ts to a single non-nullable default instead of three unreachable `?? fallback` arms.
…hrases The first fix enumerated literal negation phrases keyed on the noun forms "test"/"tests", which missed "Not tested locally." -- the verb form "tested" only matched the affirmative branch, so a body stating tests were NOT run could still satisfy a configured manifest test expectation. Redesign hasValidationNote around a shared test/validation stem definition and a word-proximity negation check (a negation word within a bounded window of the stem, in either order) instead of enumerating more literal templates, so the same class of miss cannot recur for a different tense/form. The window is clause-bounded (stops at ,.!?;) so an unrelated negation earlier in the body cannot suppress a later, real validation note.
… body
hasValidationNote ran its negation checks against the entire PR body and
returned false on the first match, so a genuine negated clause ("No tests
run locally.") vetoed a separate, later clause with real affirmative
evidence ("Validated with npm run test:ci."). Split on clause-boundary
punctuation and require only one clause to be an affirmative, non-negated
mention.
0ebfd63 to
abed057
Compare
Summary
Clarifies configured validation-evidence failures and guardrail manual-hold reasons in ORB review output.
What changed
Why
The previous review text could say “Maintainer test expectations unmet” even when the PR body included validation evidence, and guardrail hold reasons did not identify the touched path.
Validation
npx vitest run test/unit/test-evidence.test.ts test/unit/focus-manifest.test.ts test/unit/public-safe-manifest-finding.test.ts test/unit/agent-actions.test.ts test/unit/gate-check-policy.test.ts test/unit/queue.test.tsnpm run typecheckgit diff --check