feat(review): add an ordinal improvement/value judgment to the AI review schema - #4754
Conversation
…iew schema
Adds a `valueAssessment` field to `ModelReview` (`src/services/ai-review.ts`),
populated by the SAME LLM call the reviewer already makes -- no new call, no
new provider/model-selection surface. It is a genuinely different axis from
`confidence`: `confidence` is calibrated defect-certainty ("how sure am I my
own blockers are real"); `valueAssessment` instead asks "does this change
plausibly move the codebase forward, given the diff and its stated intent."
It is also not a risk judgment -- that stays the deterministic slop.ts tier,
which this call never touches.
The band is a small fixed ordinal (unclear/minor/moderate/significant),
matching this repo's existing SlopBand convention, never a percentage. The
system prompt only asks for it -- and the parser only looks for it -- when
the caller resolves `input.improvementSignal` on, mirroring how
inlineFindings/findingCategories/securityFocus are already caller-resolved
in this file, so the disabled path spends zero extra prompt or output
tokens and this file carries no new dependency on the `ConvergedFeatureKey`
union.
Sanitizer safety: the prompt explicitly steers the model toward
"improvement/value/gain" wording and away from "score" and its sibling
forbidden terms, since a sanitizer hit on AI-authored text drops the whole
note rather than redacting the offending phrase. A rationale that still
fails the check is dropped the same way (never surfaced, never partially
redacted). New tests assert a representative set of rationale strings pass
every independent public-comment sanitizer this repo relies on
(isPublicSafeText, and both same-named sanitizePublicComment
implementations), plus a negative control proving those assertions are
meaningful.
Dual-review combination: when both reviewers emit a valueAssessment, the
more conservative (lower) of the two bands is surfaced, carrying that
opinion's own rationale -- overclaiming a change's value is the riskier
direction to err toward advisory-only.
Depends on #4738 (adding `improvementSignal` as a real `ConvergedFeatureKey`)
before any caller can resolve and pass this flag in production; that has not
merged yet, so this PR uses a caller-resolved boolean field rather than an
internal resolveConvergedFeature call, keeping it independently compilable
and mergeable regardless of #4738's landing order.
Implements #4743 (sub-issue of epic #4737)
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4754 +/- ##
=======================================
Coverage 94.21% 94.22%
=======================================
Files 440 440
Lines 38712 38736 +24
Branches 14101 14112 +11
=======================================
+ Hits 36474 36498 +24
Misses 1577 1577
Partials 661 661
🚀 New features to boost your workflow:
|
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-10 23:19:03 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 2 non-blocking
Linked issue satisfactionAddressed 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.
|
Summary
valueAssessmentfield toModelReview(src/services/ai-review.ts) — an ordinalunclear/minor/moderate/significantband plus a one-sentencerationale— populated by thesame AI-review call the engine already makes. No new LLM call, no new provider/model-selection
surface.
confidence:confidenceis calibrateddefect-certainty ("how sure am I my own blockers are real");
valueAssessmentinstead asks "doesthis change plausibly move the codebase forward, given the diff and its stated intent." It is also
not a risk judgment — that stays the separate deterministic
signals/slop.tstier, untouched here.input.improvementSignalboolean, mirroring howinlineFindings/findingCategories/securityFocusalready work in this file: off by default, zeroextra prompt/output tokens spent when off.
the two reviewers' bands, carrying that reviewer's own rationale (documented in
composeImprovementSignal's doc comment).away from "score" and its sibling forbidden terms; new tests assert a representative set of
rationale strings pass every independent public-comment sanitizer this repo relies on.
Closes #4743. Part of epic #4737 (advances it; the epic itself stays open until its remaining
sub-issues land).
Scope
type(scope): short summaryConventional Commit format.changes (2 files: the service module and its test file).
CONTRIBUTING.mdand does not touch GitHub Pages/VitePress/site//CNAME.Closes #4743) — Extend ModelReview with an ordinal improvement/value judgment #4743 is a complete,independently-resolved sub-issue of epic Epic: PR improvement signal — quality-delta scoring as the positive-axis counterpart to slop-risk #4737; the epic itself (Epic: PR improvement signal — quality-delta scoring as the positive-axis counterpart to slop-risk #4737) stays open separately
since it has 8 other sub-issues still pending.
Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally (unsharded) — every line and branch this PR changed is covered(verified with a v8 coverage-map cross-reference against the actual diff hunks, not just the
aggregate repo percentage)
npm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:check(unaffected — no OpenAPI/schema surface changed)npm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate— 0 vulnerabilitiesboundaries
Additionally ran the full
npm run test:cicomposite gate end-to-end (not just its piecesindividually): exit 0, 692 test files / 13,742 tests passed, 2/12 pre-existing skips, zero
failures.
Safety
rankings, or private maintainer evidence anywhere in the diff (confirmed by reading the diff).
"score" and its sibling forbidden terms toward "improvement/value/gain" wording; new tests
assert a representative set of rationale strings pass
isPublicSafeText(
src/signals/redaction.ts) and both independently-implementedsanitizePublicCommentfunctions (
src/queue-intelligence.tsandsrc/github/commands.ts), plus a negative-controltest proving those assertions are meaningful (a rationale using forbidden vocabulary does trip
all three).
UI Evidence
N/A — no visible UI/frontend/docs surface in this PR; it only changes
src/services/ai-review.ts(internal schema + prompt) and its test file.Notes
improvementSignalas a realConvergedFeatureKey) before any callercan actually resolve and flip this flag on in production. Config-as-code foundation: improvementSignal as a converged feature (global + per-repo) #4738 had not merged to
mainas of thisPR. This PR does not block on that landing order: rather than an internal
resolveConvergedFeature/convergedFeatureActivecall (which would require"improvementSignal"toalready be a valid
ConvergedFeatureKeyto compile),input.improvementSignalis a plaincaller-resolved boolean — the exact same pattern this file already uses for
inlineFindings/findingCategories/securityFocus. That keeps this PR independently compilable,testable, and mergeable regardless of when Config-as-code foundation: improvementSignal as a converged feature (global + per-repo) #4738 lands. A follow-up change at whichever call site
constructs
GittensoryAiReviewInput(out of scope for this issue, which is scoped tosrc/services/ai-review.tsonly) will setimprovementSignal: await resolveConvergedFeature(env, manifest, "improvementSignal", repoFullName)once Config-as-code foundation: improvementSignal as a converged feature (global + per-repo) #4738 merges.
the deterministic risk tier (
slop.ts) is untouched by this change.fetch/API call, no new*_AI_MODEL/*_AI_PROVIDER-style envvar, no new provider-selection config. This rides whatever
AI_REVIEW_PLAN/AI_PROVIDERthe repoalready has configured (including a self-hosted
ollamaentry).