fix(review): fail closed on incoherent AI verdicts

[JSONbored]

Motivation

• The AI prompt instructs the model to emit a sentinel assessment when the diff is incoherent, but downstream parsing/combining treated that sentinel as a valid (clean) review, causing intended fail-closed behavior to fail open. This creates a risk that a malicious PR author could craft title/body to bypass AI block-mode reviews and reach auto-merge.

Description

• Introduce a single reusable sentinel INCOHERENT_DIFF_ASSESSMENT and reference it in the system prompt via the existing prompt assembly (src/services/ai-review.ts).
• Update parseModelReview to treat the sentinel assessment as unparseable (return null), so downstream combining logic will treat the reviewer slot as missing and produce inconclusive (hold) per existing fail-closed rules.
• Add a regression unit test that asserts a model response containing only the sentinel parses to null and that combineReviews(..., { strategy: "consensus" }) yields inconclusive: true.

Testing

• Ran the unit suite for the AI review helpers: npx vitest run test/unit/ai-review.test.ts, and the tests passed (including the new sentinel regression test).
• Ran the focused test npx vitest run test/unit/ai-review.test.ts -t incoherent, and it passed (the sentinel path is exercised).
• Attempted coverage run npm run test:coverage -- test/unit/ai-review.test.ts; the tests passed but local coverage remapping failed with TypeError: jsTokens is not a function in the coverage remapper (local toolchain issue, not related to the logic change).
• npm run typecheck surfaced pre-existing unrelated TypeScript errors in src/selfhost/sentry.ts (missing @sentry/node types), and npm run test:ci encountered environment/tooling failures (offline actionlint setup and label check) and did not fully complete; these are unrelated to the fix itself but block a full CI run in this environment.

---

Codex Task

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.45%. Comparing base (495450f) to head (64f4aab).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1581   +/-   ##
=======================================
  Coverage   95.45%   95.45%           
=======================================
  Files         202      202           
  Lines       21781    21783    +2     
  Branches     7866     7867    +1     
=======================================
+ Hits        20791    20793    +2     
  Misses        414      414           
  Partials      576      576           

Files with missing lines	Coverage Δ
src/services/ai-review.ts	98.58% <100.00%> (+0.01%)	⬆️

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[loopover-orb]

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review — held for maintainer review

2 files · 1 AI reviewers · no blockers · readiness 48/100 · CI green · unknown

⏸️ Held for maintainer review — Touches a guarded path — held for manual review

Review summary
This PR correctly closes a real security-relevant fail-open: the AI was already instructed to emit a sentinel assessment on incoherent diffs, but `parseModelReview` only short-circuited on a completely empty assessment, so a sentinel response (non-empty `assessment`, empty arrays) parsed as a valid clean review and reached auto-merge. Extracting the sentinel into `INCOHERENT_DIFF_ASSESSMENT`, interpolating it into the system prompt, and inserting an exact-equality guard before the existing empty-check at `ai-review.ts:390` closes the gap cleanly. The DRY promotion means prompt and parser can never silently diverge on the sentinel string, which was the root of the bug.

Signal	Result	Evidence
Code review	✅ No blockers	1 reviewers, synthesized
Linked issue	⚠️ Missing	No linked issue or no-issue rationale found.
Related work	⚠️ 3 scoped overlaps	Top overlaps are listed below; lower-confidence bulk is hidden.
Review load	❌ 8/20	Readiness component derived from cached public PR metadata and labels; size label size:XS.
Validation evidence	❌ 5/25	Cached preflight status is hold.
Open PR queue	❌ 3/10	26 open PR(s), 9 likely reviewable, 17 unlinked.
Contributor context	✅ Confirmed Gittensor contributor	JSONbored; Gittensor profile; 80 PR(s), 312 issue(s).
Gate result	⚠️ Not blocking	Advisory; not blocking this PR.

Nits — 6 non-blocking

• test/unit/ai-review.test.ts: the test hardcodes `const sentinel = "Cannot review — ..."` as a literal instead of importing `INCOHERENT_DIFF_ASSESSMENT`; a future rename of the constant would require a two-place update, and until the test is updated it would silently test a stale string — change the test to `import { INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review.ts"` and drop the local `sentinel` variable.
• ai-review.ts:390: the sentinel check uses strict equality, so a model response with a stray trailing space or Unicode near-lookalike won't trigger it — consider `assessment.trim() === INCOHERENT_DIFF_ASSESSMENT` to be resilient against minor model formatting noise, or add a comment that the system prompt mandates exact output.
• test/unit/ai-review.test.ts: `combineReviews([parsed, parsed], ...)` passes `[null, null]` — this exercises the all-null case but not the mixed case (one sentinel reviewer, one real reviewer returning a defect); a second assertion covering the mixed case would give stronger confidence that a sentinel reviewer doesn't accidentally cancel a real defect finding.
• Import `INCOHERENT_DIFF_ASSESSMENT` directly in the test rather than re-declaring the literal — `import { INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review.ts"` — so the regression test is structurally coupled to the constant, not its current string value.
• Add `.trim()` before the equality check in `parseModelReview` (`if (assessment.trim() === INCOHERENT_DIFF_ASSESSMENT) return null;`) to guard against insignificant whitespace variation in model output.
• Touches a guarded path — held for manual review — A maintainer must review and merge this change.

Review context

• Author: JSONbored
• Role context: owner (maintainer lane)
• Public audience mode: oss maintainer
• Lane context: Repository registration is not available in the local Gittensory cache.
• Public profile languages: not available
• Official Gittensor activity: 80 PR(s), 312 issue(s).
• Related work: Titles/paths share 5 meaningful terms. (PR #1587)
• Related work: Titles/paths share 5 meaningful terms. (PR #1582, PR #1583)
• Related work: Titles/paths share 4 meaningful terms. (PR #1583, PR #1586)

Contributor next steps

• Treat this as maintainer-lane context rather than normal contributor-lane activity.
• Explain no-issue PR.
• Review top overlaps.
• Add scope summary.
• Fix blocker.
• Expect slower review.
• Refresh registry data or choose a registered active repo.
• Link the issue being solved, or explicitly explain why this is a no-issue PR.
• Check active issues and PRs before submitting.

Signal definitions

• Related work = same linked issue, overlapping active PRs, or title/path similarity.
• Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
• Open PR queue = repo-wide review pressure; it is not a PR quality failure.
• Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

This PR correctly closes a real security-relevant fail-open: the AI was already instructed to emit a sentinel assessment on incoherent diffs, but `parseModelReview` only short-circuited on a completely empty assessment, so a sentinel response (non-empty `assessment`, empty arrays) parsed as a valid clean review and reached auto-merge. Extracting the sentinel into `INCOHERENT_DIFF_ASSESSMENT`, interpolating it into the system prompt, and inserting an exact-equality guard before the existing empty-check at `ai-review.ts:390` closes the gap cleanly. The DRY promotion means prompt and parser can never silently diverge on the sentinel string, which was the root of the bug.

Nits (5)

• test/unit/ai-review.test.ts: the test hardcodes `const sentinel = "Cannot review — ..."` as a literal instead of importing `INCOHERENT_DIFF_ASSESSMENT`; a future rename of the constant would require a two-place update, and until the test is updated it would silently test a stale string — change the test to `import { INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review.ts"` and drop the local `sentinel` variable.
• ai-review.ts:390: the sentinel check uses strict equality, so a model response with a stray trailing space or Unicode near-lookalike won't trigger it — consider `assessment.trim() === INCOHERENT_DIFF_ASSESSMENT` to be resilient against minor model formatting noise, or add a comment that the system prompt mandates exact output.
• test/unit/ai-review.test.ts: `combineReviews([parsed, parsed], ...)` passes `[null, null]` — this exercises the all-null case but not the mixed case (one sentinel reviewer, one real reviewer returning a defect); a second assertion covering the mixed case would give stronger confidence that a sentinel reviewer doesn't accidentally cancel a real defect finding.
• Import `INCOHERENT_DIFF_ASSESSMENT` directly in the test rather than re-declaring the literal — `import { INCOHERENT_DIFF_ASSESSMENT } from "../../src/services/ai-review.ts"` — so the regression test is structurally coupled to the constant, not its current string value.
• Add `.trim()` before the equality check in `parseModelReview` (`if (assessment.trim() === INCOHERENT_DIFF_ASSESSMENT) return null;`) to guard against insignificant whitespace variation in model output.

_{🟩 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.

• Re-run Gittensory review