From d16061aa7ec0d35afe766c2c4aac35ffc3f1fd94 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 19:44:42 -0700 Subject: [PATCH 1/2] feat(review): surface the improvement signal in the PR panel Combines the deterministic structural-improvement tier (#4742) and the LLM tier's composed value judgment (#4743) into a new advisory panel row, gated behind the improvementSignal converged feature (#4738). Re-checks both inputs against the public-safety filter as defense in depth, and fixes containsPrivatePublicTerm's regex (it had "hotkey" but was missing its "coldkey" sibling). Closes #4744 Part of epic #4737 --- .gittensory.yml.example | 5 +- .../gittensory-engine/src/focus-manifest.ts | 7 +- src/config/gittensory-repo-focus-manifest.ts | 2 +- src/queue/processors.ts | 65 +++++++ src/signals/engine.ts | 115 +++++++++++- test/unit/ai-review-advisory.test.ts | 45 +++++ test/unit/queue-4.test.ts | 176 ++++++++++++++++++ test/unit/signals-coverage.test.ts | 161 ++++++++++++++++ 8 files changed, 568 insertions(+), 8 deletions(-) diff --git a/.gittensory.yml.example b/.gittensory.yml.example index f4304f1233..c64f781690 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -1075,7 +1075,10 @@ settings: # text: "Reviewed by the Acme maintainer bot." # Custom lead line. String or null. Default: null. # note: "Run the test suite before requesting review." # Short intro line shown above the panel. String or null. # # Per-row show/hide toggles for the panel. Keys: linkedIssue | relatedWork | reviewLoad | -# # validationEvidence | openPrQueue | contributorContext | gateResult. Default: all shown (true). +# # validationEvidence | openPrQueue | contributorContext | gateResult | improvementSignal. Default: all +# # shown (true). improvementSignal (#4744) only ever renders content when the `improvementSignal` converged +# # feature (see `features:` below) is ALSO active for this repo -- this toggle just hides that row/section +# # like any other; it never turns the feature itself on. # fields: # relatedWork: false # openPrQueue: false diff --git a/packages/gittensory-engine/src/focus-manifest.ts b/packages/gittensory-engine/src/focus-manifest.ts index 793cd6dbab..2d40087233 100644 --- a/packages/gittensory-engine/src/focus-manifest.ts +++ b/packages/gittensory-engine/src/focus-manifest.ts @@ -434,8 +434,11 @@ export type FocusManifestSettings = Partial< advisoryAiRouting?: Partial | undefined; }; -/** Field keys for the public review-panel rows a maintainer can show/hide via `review.fields`. */ -export const REVIEW_FIELD_KEYS = ["linkedIssue", "relatedWork", "reviewLoad", "validationEvidence", "openPrQueue", "contributorContext", "gateResult"] as const; +/** Field keys for the public review-panel rows a maintainer can show/hide via `review.fields`. `improvementSignal` + * (#4744) is the newest: the optional row combining the deterministic structural-improvement tier (#4742) and, + * where also active, the LLM tier's composed value judgment (#4743) -- default-shown, like every sibling here, + * when the `improvementSignal` converged feature itself is active for the repo. */ +export const REVIEW_FIELD_KEYS = ["linkedIssue", "relatedWork", "reviewLoad", "validationEvidence", "openPrQueue", "contributorContext", "gateResult", "improvementSignal"] as const; export type ReviewFieldKey = (typeof REVIEW_FIELD_KEYS)[number]; // `review.profile` (#review-profile): how nitpicky the AI maintainer review is. `chill` = surface only blocking diff --git a/src/config/gittensory-repo-focus-manifest.ts b/src/config/gittensory-repo-focus-manifest.ts index 30fd6caa1f..88c1b872e2 100644 --- a/src/config/gittensory-repo-focus-manifest.ts +++ b/src/config/gittensory-repo-focus-manifest.ts @@ -57,7 +57,7 @@ gate: # note: "Run the test suite before requesting review." # short intro line shown above the panel # fields: # show/hide rows (default: all shown). Stable keys: # relatedWork: false # linkedIssue | relatedWork | reviewLoad (Change scope) | -# openPrQueue: false # validationEvidence (Validation posture) | openPrQueue (Contributor workload) | contributorContext | gateResult +# openPrQueue: false # validationEvidence (Validation posture) | openPrQueue (Contributor workload) | contributorContext | gateResult | improvementSignal # AI-review eligibility filters (#3999): a draft PR previously re-triggered a full AI review on every # push, letting a contributor iterate for free while tokens kept burning — skip_drafts stops that. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 51cc6d1bf1..0ef36d0ed2 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -419,6 +419,7 @@ import { buildSlopAssessment, type SlopBand, } from "../signals/slop"; +import { buildStructuralImprovementAssessment } from "../signals/improvement"; import { runGittensoryLinkedIssueSatisfaction } from "../services/linked-issue-satisfaction-run"; import { decidePublicSurface } from "../signals/settings-preview"; import { @@ -468,6 +469,7 @@ import { runGittensoryAiReview, utcDayStartIso, type AiReviewActualUsage, + type ImprovementMagnitude, type InlineFinding, } from "../services/ai-review"; import { @@ -7395,6 +7397,16 @@ export async function runAiReviewForAdvisory( // quality-culture reference block (typical merged-PR size + common labels) to the reviewer prompt. Absent/ // false ⇒ byte-identical (no section, no extra D1 read). reviewCultureProfile?: boolean | undefined; + // `.gittensory.yml` `features.improvementSignal` (#4744, first real caller of #4738's activation wiring), + // resolved by the caller via `convergedFeatureActive`/`resolveConvergedFeature` -- NOT resolved internally + // here (unlike reputation/rag/grounding above), mirroring reviewProfile/reviewImpactMap/reviewCultureProfile + // above, which are ALL caller-resolved rather than looked up internally (see `ModelReview.valueAssessment`'s + // own doc comment in services/ai-review.ts for why `improvementSignal` -- a read-only advisory signal, not a + // security control -- follows that majority pattern rather than `safety`'s internal-resolution exception). + // Threaded straight into runGittensoryAiReview's own `improvementSignal` gate (#4743) for the LLM tier's + // value-assessment prompt addition. Absent/false ⇒ the prompt is byte-identical (no valueAssessment + // requested) -- the only reachable value until this PR started resolving the feature. + improvementSignal?: boolean | undefined; // The inbound webhook delivery id that triggered this review (#codex-timeout-fields) — forwarded to a // self-host provider's failure log purely for operator correlation; never read by any review logic. Absent // (e.g. a sweep/repair fan-out with no single originating delivery, or a unit test) ⇒ the log line omits it. @@ -7437,6 +7449,13 @@ export async function runAiReviewForAdvisory( // within the bounded cooldown could replay "another pass is running" long after that pass finished. // Defaults to true (persistable) for every other outcome, cacheable or not. persistable?: boolean | undefined; + // The LLM tier's composed improvement/value judgment (#4743/#4744) -- present ONLY on a FRESH review + // (cache miss) with `improvementSignal` requested and at least one reviewer emitting a usable, public-safe + // judgment. Absent on a cache hit: exactly like `inlineFindings`/`impactMap` above, `ai_review_cache` + // never persists this field (getCachedAiReview/putCachedAiReview, db/repositories.ts, have no column for + // it), so a re-served cached review has no LLM-tier judgment to show on that particular render. The + // deterministic tier is unaffected -- it is computed fresh every pass, never cached. + valueAssessment?: { magnitude: ImprovementMagnitude; rationale: string } | undefined; } | undefined > { @@ -7726,6 +7745,9 @@ export async function runAiReviewForAdvisory( ), repoInstructions: args.reviewInstructions ?? null, changedFiles: files, + // improvementSignal (#4744): ask the model for the ordinal value/improvement judgment (#4743) only when + // the caller resolved the feature on for this repo. Absent/false ⇒ byte-identical prompt. + improvementSignal: args.improvementSignal === true, }); if (result.status !== "ok") return undefined; const findings: AdvisoryFinding[] = []; @@ -7818,6 +7840,7 @@ export async function runAiReviewForAdvisory( findings, metadata: metadataFor(result.advisoryNotes, []), cacheable: false, + valueAssessment: result.valueAssessment ?? undefined, }; } if (hasPublicReviewAssessment(result.advisoryNotes)) { @@ -7828,6 +7851,7 @@ export async function runAiReviewForAdvisory( impactMap: impactMapEntries, findings, metadata: metadataFor(result.advisoryNotes, result.inlineFindings), + valueAssessment: result.valueAssessment ?? undefined, }; } if (result.inconclusive) { @@ -9038,6 +9062,18 @@ async function maybePublishPrPublicSurface( repoFullName, "unifiedComment", ); + // improvementSignal (#4744): the first real caller of #4738's activation wiring (epic #4737's config-as-code + // foundation) -- nothing resolved this feature before this PR (see signals/improvement.ts's own header + // comment). Resolved once, independent of unifiedCommentAllowed above: it gates BOTH the deterministic + // tier's own computation further below (which has no AI dependency at all -- a paused repo, a non-reviewable + // author, or aiReviewMode: "off" still gets it) and, threaded into runAiReviewForAdvisory, the LLM tier's + // prompt addition (#4743). loadRepoFocusManifest is cached, so this second manifest resolution costs no + // extra fetch in the common case where something else already resolved it this pass. + const improvementSignalAllowed = await convergedFeatureActive( + env, + repoFullName, + "improvementSignal", + ); // `settings` is the EFFECTIVE config (`.gittensory.yml` > DB > defaults), resolved by the caller via // resolveRepositorySettings — so gate on/off and every blocker mode already reflect the repo's config // file. The gate verdict is the same for every author; confirmedContributor feeds only on-chain scoring. @@ -9414,6 +9450,8 @@ async function maybePublishPrPublicSurface( // inlineFindings is present ONLY on a FRESH review (cache miss) with inline comments enabled; the AI cache // round-trips notes + reviewerCount + the gate findings (so a cache hit replays consensus/split/inconclusive // blockers — see below), but NOT inlineFindings, so a cache hit never re-posts inline comments (#inline-comments). + // valueAssessment (#4743/#4744) follows the exact same cache-miss-only shape as inlineFindings/impactMap -- + // see runAiReviewForAdvisory's own return-type doc comment. let aiReview: | { notes: string; @@ -9424,6 +9462,7 @@ async function maybePublishPrPublicSurface( metadata?: Record | undefined; cacheable?: boolean | undefined; persistable?: boolean | undefined; + valueAssessment?: { magnitude: ImprovementMagnitude; rationale: string } | undefined; } | undefined; let inlineCommentsEnabledForReview = false; @@ -10523,6 +10562,10 @@ async function maybePublishPrPublicSurface( reviewSelfHostAiModel, reviewImpactMap, reviewCultureProfile, + // improvementSignal (#4744): resolved once above (independent of unifiedCommentAllowed), reused + // here so the LLM tier's value-assessment prompt addition (#4743) only fires when this repo has + // actually opted in. + improvementSignal: improvementSignalAllowed, // #regate-dup-prep: this call's own advisory lock is already claimed (by aiReviewCacheReadDecideAndRun's // caller, above) — pass it through so runAiReviewForAdvisory trusts it instead of re-claiming (and // losing) against itself, and does not release it before the cache write below runs. @@ -11196,6 +11239,25 @@ async function maybePublishPrPublicSurface( // winner's hard-duplicate block is suppressed (they recompute the winner from their own open-only sibling // list). Flag-OFF (default) ⇒ false ⇒ the panels are byte-identical to today. const duplicateWinnerEnabled = env.GITTENSORY_DUPLICATE_WINNER === "true"; + // improvementSignal deterministic tier (#4742/#4744): pure/sync, no AI dependency, so it is computed + // independent of aiReview's own eligibility gates above (a paused repo, non-reviewable author, or + // aiReviewMode: "off" still gets this tier -- the two tiers are deliberately independent, epic #4737). + // Only computed when the feature resolves on for this repo, matching "nothing at all when the feature is + // off" (#4744) and avoiding the extra file resolve on the default (until an operator opts in) path where + // it's off. changedFiles reuses the SAME memoized getReviewFiles() resolver every other gate/panel input + // already calls, so this costs no extra fetch when something else already resolved it this pass. + // complexityDeltas/duplicationDeltas/patchCoverageDeltaPercent have no caller yet (see improvement.ts's own + // header comment) -- only the changedFiles-based axes (test-evidence) can fire today; that is expected, + // not a bug in this PR, and the assessment degrades cleanly ("insufficient-signal"/"none") when they don't. + const structuralImprovementAssessment = improvementSignalAllowed + ? buildStructuralImprovementAssessment({ + changedFiles: (await getReviewFiles()).map((file) => ({ + path: file.path, + additions: file.additions, + deletions: file.deletions, + })), + }) + : undefined; const commentArgs = { repo, pr, @@ -11208,6 +11270,7 @@ async function maybePublishPrPublicSurface( gate: gateEvaluation, review: reviewConfig, aiReview, + improvementSignal: structuralImprovementAssessment, duplicateWinnerEnabled, env, }; @@ -11339,6 +11402,8 @@ async function maybePublishPrPublicSurface( settings, gate: commentGate, duplicateWinnerEnabled, + improvementSignal: structuralImprovementAssessment, + valueAssessment: aiReview?.valueAssessment, }); // Visual before/after capture (visual-capture port). Fires ONLY when (1) the "screenshots" converged // feature resolves active for this repo (resolveConvergedFeature — the global flag AND (a per-repo diff --git a/src/signals/engine.ts b/src/signals/engine.ts index 37f18fe23f..ca1a2ab770 100644 --- a/src/signals/engine.ts +++ b/src/signals/engine.ts @@ -37,6 +37,8 @@ import { splitAiReviewNits } from "../review/ai-notes"; import { GITTENSORY_GATE_CHECK_NAME, shouldPublishReviewCheck } from "../review/check-names"; import { isAgentConfigured } from "../settings/autonomy"; import { diffFilePriority } from "../review/review-diff"; +import type { ImprovementBand, StructuralImprovementAssessment } from "./improvement"; +import type { ImprovementMagnitude } from "../services/ai-review"; export type ParticipationLane = "direct_pr" | "issue_discovery" | "split" | "inactive" | "unknown"; export type SignalFinding = AdvisoryFinding; @@ -4288,12 +4290,21 @@ export function buildPublicPrIntelligenceComment(args: { settings: RepositorySettings; gate?: PublicPrPanelGateEvaluation | undefined; review?: FocusManifestReviewConfig | undefined; - /** Optional AI maintainer-review notes (already public-safe). Rendered as an advisory section. */ - aiReview?: { notes: string } | undefined; + /** Optional AI maintainer-review notes (already public-safe). Rendered as an advisory section. `valueAssessment` + * (#4743/#4744) is the same tier's composed improvement/value judgment, already run through + * `composeImprovementSignal`'s own `toPublicSafe` pass upstream (services/ai-review.ts) -- re-checked against + * `containsPrivatePublicTerm` again here (defense in depth) before it can reach the improvement row below. */ + aiReview?: { notes: string; valueAssessment?: { magnitude: ImprovementMagnitude; rationale: string } | undefined } | undefined; /** Duplicate-winner adjudication (#dup-winner). When true AND this PR is the earliest observed linked-issue * claimant among `linkedDuplicatePrs`, the hard-duplicate panel block is suppressed so the winner's panel * does not show a blocking duplicate. Default/false ⇒ byte-identical to today. */ duplicateWinnerEnabled?: boolean | undefined; + /** Deterministic structural-improvement tier (#4742/#4744), pre-computed by the caller via + * `buildStructuralImprovementAssessment` and passed through exactly like `gate`/`aiReview` above are + * pre-computed results, not raw inputs. Absent ⇒ the improvement row renders nothing, matching + * `resolveConvergedFeature(env, manifest, "improvementSignal", repoFullName)` resolving false for the repo, + * or a caller that hasn't wired this yet. */ + improvementSignal?: StructuralImprovementAssessment | undefined; /** Resolved by the caller from `env.PUBLIC_SITE_ORIGIN` so a self-hoster's own domain reaches the * always-on footer's attribution link instead of `GITTENSORY_SITE_URL` (#4613). */ env: GittensoryFooterEnv; @@ -4415,6 +4426,11 @@ export function buildPublicPrIntelligenceComment(args: { { key: "contributorContext", cells: ["Contributor context", contributorContext.result, contributorContext.evidence, contributorContext.action] }, { key: "gateResult", cells: ["Gate result", gateStatus(gateEnabled, gateConclusion), gateEnabled ? gateAction(gateConclusion) : "Advisory only.", gateEnabled ? gateNextAction(gateConclusion) : "No action."] }, ]; + // Improvement row (#4744): combines the deterministic tier (#4742) + LLM tier (#4743). `improvementRow` is + // null (row omitted entirely) when the caller passes no `improvementSignal` -- see buildImprovementSignalRow's + // own doc comment for why that's what keeps this byte-identical to today for every existing caller. + const improvementRow = buildImprovementSignalRow(args.improvementSignal, args.aiReview?.valueAssessment); + if (improvementRow) allRows.push(improvementRow); const reviewFields = args.review?.fields; const rows: Array<[string, string, string, string]> = allRows.filter((row) => reviewFields?.[row.key] !== false).map((row) => row.cells); const overlapDetails = relatedWorkDetails(args.pr, scopedOverlapClusters); @@ -4558,6 +4574,18 @@ export function buildPublicPrPanelSignalRows(args: { * claimant among `linkedDuplicatePrs`, the hard-duplicate block is suppressed. Default/false ⇒ byte-identical * to today. Matches `buildPublicPrIntelligenceComment` so both panels agree. */ duplicateWinnerEnabled?: boolean | undefined; + /** Deterministic structural-improvement tier (#4742/#4744), pre-computed by the caller via + * `buildStructuralImprovementAssessment` and passed through exactly like `gate` above is a pre-computed + * result, not a raw input -- keeps this render layer pure/sync. Absent (every existing caller today) ⇒ the + * row is omitted entirely and `rows` stays byte-identical to today, matching + * `resolveConvergedFeature(env, manifest, "improvementSignal", repoFullName)` resolving false for the repo, + * or a caller that hasn't wired this yet. */ + improvementSignal?: StructuralImprovementAssessment | undefined; + /** The LLM tier's composed improvement/value judgment (#4743), already run through `composeImprovementSignal`'s + * own `toPublicSafe` pass upstream. Re-checked against `containsPrivatePublicTerm` here anyway (defense in + * depth, #4744) before it can reach the row. Absent ⇒ the row (when `improvementSignal` above is present) + * shows the deterministic tier only. */ + valueAssessment?: { magnitude: ImprovementMagnitude; rationale: string } | undefined; }): { rows: PublicPrPanelSignalRow[]; readinessTotal: number } { const relatedWork = buildDuplicateWinnerRelatedWorkView({ pr: args.pr, @@ -4600,7 +4628,83 @@ export function buildPublicPrPanelSignalRows(args: { { key: "contributorContext", cells: ["Contributor context", contributorContext.result, contributorContext.evidence, contributorContext.action] }, { key: "gateResult", cells: ["Gate result", gateStatus(gateEnabled, gateConclusion), gateEnabled ? gateAction(gateConclusion) : "Advisory only.", gateEnabled ? gateNextAction(gateConclusion) : "No action."] }, ]; - return { rows, readinessTotal: readiness.total }; + const improvementRow = buildImprovementSignalRow(args.improvementSignal, args.valueAssessment); + return { rows: improvementRow ? [...rows, improvementRow] : rows, readinessTotal: readiness.total }; +} + +// ── Improvement-signal row (#4744) ─────────────────────────────────────────────────────────────────── +// +// Combines the deterministic tier (#4742, `buildStructuralImprovementAssessment`) and, when also active, the +// LLM tier's composed judgment (#4743, `composeImprovementSignal`) into the optional 8th panel row. Shared by +// `allRows` (legacy) and `buildPublicPrPanelSignalRows` (unified-comment bridge) so the two never diverge, the +// same way the other seven rows are already hand-mirrored between the two functions. Advisory only -- this +// row is never a gate input (epic #4737 design constraint 2), and it never renders anything when the caller +// omits `improvementSignal` (the `improvementSignal` converged feature resolving false for the repo, or the +// deterministic tier having nothing to report today isn't possible -- `buildStructuralImprovementAssessment` +// always returns a band, even "insufficient-signal"). + +/** Static template labels (#4744), one per {@link ImprovementBand} -- never runtime-interpolated free text, so + * this bypasses the public-comment sanitizer safely, mirroring how `"**Readiness score: ${total}/100**"` + * (buildPublicPrIntelligenceComment) is a hardcoded template rather than sanitizer-filtered AI prose. Advisory + * icons only (✅/ℹ️, never ⚠️/❌): every band here is informational, never a reason to flag the PR. */ +const IMPROVEMENT_BAND_LABELS: Record = { + "insufficient-signal": "ℹ️ Insufficient signal", + none: "ℹ️ None detected", + minor: "✅ Minor", + moderate: "✅ Moderate", + significant: "✅ Significant", +}; + +/** The Evidence cell: a short, safe-by-construction summary of the deterministic findings (already filtered + * through `containsPrivatePublicTerm` by the caller), plus the LLM tier's magnitude + rationale when present + * (also already filtered). Caps at 2 inline finding sentences -- mirrors the "Nits"-style convention of not + * dumping every finding inline; with more than 2, the remainder is summarized by count rather than omitted + * (see the module comment above for why a full collapsible isn't wired up in this PR: at most one + * deterministic finding can fire today, since REES's complexity/duplication analyzers and a parsed Codecov + * number have no caller yet -- see `signals/improvement.ts`'s own header comment). */ +function improvementEvidenceText( + band: ImprovementBand, + safeFindings: SignalFinding[], + safeValueAssessment: { magnitude: ImprovementMagnitude; rationale: string } | undefined, +): string { + const findingSentences = safeFindings.map((finding) => finding.publicText ?? finding.detail); + const deterministicPart = + band === "insufficient-signal" + ? "Nothing measurable for the structural-improvement analyzers on this PR (e.g. no code files changed)." + : findingSentences.length > 0 + ? findingSentences.slice(0, 2).join(" ") + (findingSentences.length > 2 ? ` (+${findingSentences.length - 2} more.)` : "") + : "No structural-improvement signals were detected for this PR."; + const valuePart = safeValueAssessment ? ` LLM value judgment: ${safeValueAssessment.magnitude} — ${safeValueAssessment.rationale}` : ""; + return `${deterministicPart}${valuePart}`; +} + +/** Builds the optional "Improvement" row, or `null` when the caller has no improvement data to show. `null` + * here (rather than a placeholder row) is what keeps `allRows`/`buildPublicPrPanelSignalRows`'s `rows` + * byte-identical to today for every existing caller that doesn't pass `improvementSignal` -- see the + * `KEYS`/`toHaveLength(7)` assertions in signals-coverage.test.ts, which assume a fixed 7-row table. Defense + * in depth (#4744 requirement, epic #4737): both the deterministic findings and the LLM rationale are + * re-checked against `containsPrivatePublicTerm` here even though `improvement.ts`'s findings are safe by + * construction (integers interpolated into a fixed template) and the LLM rationale already passed + * `composeImprovementSignal`'s own `toPublicSafe` check upstream (services/ai-review.ts) -- this row must + * never leak forbidden vocabulary regardless of what feeds it, not merely trust that upstream composition. */ +function buildImprovementSignalRow( + assessment: StructuralImprovementAssessment | undefined, + valueAssessment: { magnitude: ImprovementMagnitude; rationale: string } | undefined, +): PublicPrPanelSignalRow | null { + if (!assessment) return null; + const safeFindings = assessment.findings.filter( + (finding) => !containsPrivatePublicTerm([finding.title, finding.detail, finding.publicText].filter(Boolean).join(" ")), + ); + const safeValueAssessment = valueAssessment && !containsPrivatePublicTerm(valueAssessment.rationale) ? valueAssessment : undefined; + return { + key: "improvementSignal", + cells: [ + "Improvement", + IMPROVEMENT_BAND_LABELS[assessment.band], + improvementEvidenceText(assessment.band, safeFindings, safeValueAssessment), + "Advisory only — never blocks merge.", + ], + }; } function isOfficialContributorDetection(detection: ContributorDetection): boolean { @@ -5140,7 +5244,10 @@ function isPrivateBountyLifecycleFinding(code: string): boolean { } function containsPrivatePublicTerm(value: string): boolean { - return /\b(reward|payout|farming|wallet|hotkey|trust score|raw trust|estimated score|scoreability|likely_duplicate|reviewability\s*\d)\b/i.test(value); + // "coldkey" added alongside its existing "hotkey" sibling (#4744) -- the improvement-signal row below is the + // first caller that re-checks an already-composed LLM sentence against this backstop, and the wallet-key pair + // is otherwise incomplete (a bare "coldkey" mention previously slipped through untouched). + return /\b(reward|payout|farming|wallet|hotkey|coldkey|trust score|raw trust|estimated score|scoreability|likely_duplicate|reviewability\s*\d)\b/i.test(value); } function sanitizePanelText(value: string): string { diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index 70df48b0ea..3db7da3ee4 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -62,6 +62,17 @@ function notesOnlyJson() { function nitsWithoutAssessmentJson() { return JSON.stringify({ assessment: "", blockers: [], nits: ["Add a test."], suggestions: ["Add a test."] }); } +// #4744: a model response that ALSO includes the #4743 valueAssessment field, for exercising +// runAiReviewForAdvisory's improvementSignal pass-through end to end. +function notesWithValueAssessmentJson() { + return JSON.stringify({ + assessment: "Looks fine.", + blockers: [], + nits: ["Add a test."], + suggestions: ["Add a test."], + valueAssessment: { magnitude: "moderate", rationale: "This meaningfully simplifies the retry logic." }, + }); +} function aiEnv(run: () => Promise, flags = true) { return createTestEnv({ @@ -592,6 +603,40 @@ describe("runAiReviewForAdvisory", () => { expect(result?.notes).toContain("Add a test."); }); + it("#4744: threads improvementSignal into the model call and surfaces the composed valueAssessment when requested", async () => { + const adv = advisory(); + const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: notesWithValueAssessmentJson() })), { + mode: "live", + settings: { aiReviewMode: "advisory" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + improvementSignal: true, + }); + expect(result?.valueAssessment).toEqual({ + magnitude: "moderate", + rationale: "This meaningfully simplifies the retry logic.", + }); + }); + + it("#4744: omits valueAssessment when the caller does not resolve improvementSignal on, even if the model returned one", async () => { + const adv = advisory(); + const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: notesWithValueAssessmentJson() })), { + mode: "live", + settings: { aiReviewMode: "advisory" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + // improvementSignal omitted (the default, and every caller until this PR) -- the prompt never asked for + // the field, so runGittensoryAiReview never composes it regardless of what the raw model JSON contains. + }); + expect(result?.valueAssessment).toBeUndefined(); + }); + it("returns undefined (no notes, no finding) when AI is disabled", async () => { const adv = advisory(); const result = await runAiReviewForAdvisory(aiEnv(async () => ({ response: defectJson() }), false), { diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 5342eb8d77..caa35eeab4 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2854,6 +2854,182 @@ describe("queue processors", () => { } }); + // #4744 (improvement-signal panel row, epic #4737): same unified-comment scaffold as the test above, but with + // the `improvementSignal` converged feature ALSO resolved on (env kill-switch + allowlist) — this is the only + // way, anywhere in the existing suite, that `maybePublishPrPublicSurface`'s `improvementSignalAllowed` ternary + // takes its TRUE arm: every other existing test leaves the feature off (the default), which already covers the + // FALSE arm thousands of times over. Proves the deterministic tier threads end to end into a real posted + // comment, not just in the isolated `buildPublicPrPanelSignalRows`/`buildStructuralImprovementAssessment` unit + // tests (signals-coverage.test.ts). + it("#4744: threads the improvement-signal row into the unified comment when the converged feature resolves on", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", + GITTENSORY_REVIEW_IMPROVEMENT_SIGNAL: "true", + GITTENSORY_REVIEW_REPOS: "JSONbored/gittensory", + }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", reviewCheckMode: "required", + backfillEnabled: true, + autonomy: { update_branch: "auto" }, + }); + let postedBody = ""; + const calls = { comments: 0, gateChecks: 0 }; + let gateFinalized = false; + let failedPostGateMint = false; + const liveCiSpy = vi + .spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl") + .mockRejectedValueOnce(new Error("transient CI read failed")) + .mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + hasMissingRequiredContext: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") { + return Response.json([ + { + uid: 7, + githubUsername: "oktofeesh1", + githubId: "123", + totalPrs: 4, + totalMergedPrs: 3, + totalOpenPrs: 1, + totalClosedPrs: 0, + totalOpenIssues: 0, + totalClosedIssues: 0, + totalSolvedIssues: 0, + totalValidSolvedIssues: 0, + isEligible: true, + credibility: 1, + eligibleRepoCount: 1, + hotkey: "must-not-leak", + }, + ]); + } + if (url === "https://api.gittensor.io/miners/123") { + return Response.json({ + repositories: [ + { + repositoryFullName: "JSONbored/gittensory", + totalPrs: "4", + totalMergedPrs: "3", + totalOpenPrs: "1", + totalClosedPrs: "0", + totalOpenIssues: "0", + totalClosedIssues: "0", + isEligible: true, + credibility: "1.000000", + }, + ], + }); + } + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]); + if (url.includes("/access_tokens")) { + if (gateFinalized && !failedPostGateMint) { + failedPostGateMint = true; + return new Response("mint failed", { status: 500 }); + } + return Response.json({ token: "installation-token", expires_at: "2026-05-28T00:04:00.000Z" }); + } + // PR files — also what the improvement-signal deterministic tier reads via getReviewFiles() for its + // test-evidence axis (#4742); one plain code file with no accompanying test evidence resolves to band "none". + if (url.includes("/pulls/4/files")) return Response.json([{ filename: "src/cache.ts", additions: 5, deletions: 1, status: "modified" }]); + if (/\/pulls\/4(?:\?|$)/.test(url)) return Response.json({ number: 4, mergeable_state: "clean" }); + if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") { + calls.gateChecks += 1; + const body = JSON.parse(String(init?.body ?? "{}")) as { status?: string; conclusion?: string }; + if (body.status !== "in_progress" || body.conclusion) { + gateFinalized = true; + clearInstallationTokenCacheForTest(); + } + return Response.json({ id: 901 }, { status: 201 }); + } + if (url.includes("/check-runs/901") && method === "PATCH") { + calls.gateChecks += 1; + gateFinalized = true; + clearInstallationTokenCacheForTest(); + return Response.json({ id: 901 }); + } + if (url.includes("/issues/4/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/4/comments") && method === "POST") { + calls.comments += 1; + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "pr-improvement-signal-unified", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 4, + title: "Cache invalidation cleanup", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "improvement123" }, + labels: [{ name: "bug" }], + body: "Fixes #1", + }, + }, + }); + + expect(calls.comments).toBeGreaterThan(0); + expect(postedBody).toContain(""); + // The improvement-signal row (#4744) — proves improvementSignalAllowed resolved TRUE and + // buildStructuralImprovementAssessment's result threaded all the way into the posted comment, not just + // computed and discarded. No test evidence was provided for the one changed code file, so this resolves to + // band "none" (measured, found nothing) rather than "insufficient-signal" (nothing to measure at all). The + // unified renderer's table only surfaces the first 3 of each row's 4 cells (Label/Result/Evidence, not + // Action) — same as the adjacent "Gate result" row, which also never shows its own 4th cell here — so this + // asserts against the 3 columns this renderer actually prints, not the row's full cells array. + expect(postedBody).toContain("| Improvement | ⚠️ ℹ️ None detected | No structural-improvement signals were detected for this PR. |"); + // Public-safe regardless: no internal trust/economics fields leak through this new row either. + expect(postedBody).not.toMatch(/wallet|hotkey|coldkey|reward|trust score/i); + } finally { + liveCiSpy.mockRestore(); + } + }); + it("INVARIANT (#4498): the disposition planner reuses the public surface's own live mergeable_state/CI read instead of re-fetching a third time", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); await persistRegistrySnapshot( diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 75fbaad8f8..8b3af5b61c 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -40,6 +40,7 @@ import { buildRepoRewardRisk, } from "../../src/signals/reward-risk"; import { PREFLIGHT_LIMITS } from "../../src/signals/preflight-limits"; +import type { FocusManifestReviewConfig } from "../../src/signals/focus-manifest"; import type { GittensorContributorSnapshot } from "../../src/gittensor/api"; import type { ContributorRepoStatRecord, @@ -881,6 +882,166 @@ describe("signal coverage edge cases", () => { expect(noAutonomyGateRow.cells[3]).toBe("No action."); }); + describe("#4744: improvement-signal panel row (deterministic tier #4742 + LLM tier #4743)", () => { + const improvementRepo = repo("owner/improvement"); + const improvementPr = pr(improvementRepo.fullName, 120, "Simplify retry logic", { authorLogin: "miner", linkedIssues: [7], body: "Fixes #7" }); + const improvementProfile = buildContributorProfile("miner", { login: "miner", topLanguages: ["TypeScript"], source: "github" }, [], []); + const improvementDetection = { detected: true, source: "official_gittensor_api" as const, reason: "Confirmed.", priorPullRequests: 1, priorMergedPullRequests: 0, priorIssues: 0 }; + const improvementCollisions = buildCollisionReport(improvementRepo.fullName, [], []); + const improvementQueueHealth = buildQueueHealth(improvementRepo, [], [], improvementCollisions); + const improvementPreflight = buildPreflightResult( + { repoFullName: improvementRepo.fullName, title: improvementPr.title, body: improvementPr.body ?? undefined, linkedIssues: improvementPr.linkedIssues, changedFiles: ["src/retry.ts"] }, + improvementRepo, + [], + [], + ); + const improvementSettings = repoSettings(improvementRepo.fullName); + const improvementBaseArgs = { + repo: improvementRepo, + pr: improvementPr, + profile: improvementProfile, + detection: improvementDetection, + queueHealth: improvementQueueHealth, + collisions: improvementCollisions, + preflight: improvementPreflight, + settings: improvementSettings, + }; + const minorAssessment = { + improvementScore: 10, + band: "minor" as const, + findings: [ + { + code: "added_test_evidence", + title: "Change carries test evidence", + severity: "info" as const, + detail: "Code changes are accompanied by test evidence.", + action: "No action needed — this is a positive signal.", + publicText: "Code changes are accompanied by test evidence.", + }, + ], + }; + + it("omits the row entirely when the caller passes no improvementSignal (feature off, or not wired yet)", () => { + const comment = buildPublicPrIntelligenceComment({ ...improvementBaseArgs, env: {} }); + expect(comment).not.toContain("| Improvement |"); + const panel = buildPublicPrPanelSignalRows(improvementBaseArgs); + expect(panel.rows.find((r) => r.key === "improvementSignal")).toBeUndefined(); + expect(panel.rows).toHaveLength(7); + }); + + it("renders the deterministic band as a static template label when only the deterministic tier is available", () => { + const comment = buildPublicPrIntelligenceComment({ ...improvementBaseArgs, improvementSignal: minorAssessment, env: {} }); + expect(comment).toContain("| Improvement | ✅ Minor |"); + expect(comment).toContain("Code changes are accompanied by test evidence."); + expect(comment).not.toContain("Value judgment"); + expect(comment).not.toContain("LLM value judgment"); + + const panel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: minorAssessment }); + expect(panel.rows).toHaveLength(8); + const row = panel.rows.find((r) => r.key === "improvementSignal")!; + expect(row.cells).toEqual([ + "Improvement", + "✅ Minor", + "Code changes are accompanied by test evidence.", + "Advisory only — never blocks merge.", + ]); + }); + + it("renders the LLM tier's magnitude + rationale alongside the deterministic band when both tiers are available", () => { + const noneAssessment = { improvementScore: 0, band: "none" as const, findings: [] }; + const valueAssessment = { magnitude: "significant" as const, rationale: "This removes a whole class of retry bugs." }; + const comment = buildPublicPrIntelligenceComment({ + ...improvementBaseArgs, + improvementSignal: noneAssessment, + aiReview: { notes: "Looks fine.", valueAssessment }, + env: {}, + }); + // The Result cell reflects the DETERMINISTIC band ("none"), never the LLM magnitude -- the two tiers are + // deliberately never blended into one number/label (epic #4737 design constraint 1). + expect(comment).toContain("| Improvement | ℹ️ None detected |"); + expect(comment).toContain("LLM value judgment: significant — This removes a whole class of retry bugs."); + + const panel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: noneAssessment, valueAssessment }); + const row = panel.rows.find((r) => r.key === "improvementSignal")!; + expect(row.cells[1]).toBe("ℹ️ None detected"); + expect(row.cells[2]).toBe("No structural-improvement signals were detected for this PR. LLM value judgment: significant — This removes a whole class of retry bugs."); + }); + + it("renders the insufficient-signal band and caps inline findings at 2 with a '+N more' summary beyond that", () => { + const insufficientAssessment = { improvementScore: 0, band: "insufficient-signal" as const, findings: [] }; + const insufficientPanel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: insufficientAssessment }); + const insufficientRow = insufficientPanel.rows.find((r) => r.key === "improvementSignal")!; + expect(insufficientRow.cells[1]).toBe("ℹ️ Insufficient signal"); + expect(insufficientRow.cells[2]).toContain("Nothing measurable"); + + const manyFindingsAssessment = { + improvementScore: 100, + band: "significant" as const, + findings: [ + { code: "reduced_complexity", title: "Complexity went down", severity: "info" as const, detail: "2 function(s) have lower cyclomatic complexity after this pull request." }, + { code: "resolved_duplication", title: "Duplication went down", severity: "info" as const, detail: "1 previously-duplicated code block(s) were consolidated or removed by this pull request." }, + { code: "increased_patch_coverage", title: "Patch coverage went up", severity: "info" as const, detail: "Patch coverage increased by 5 percentage point(s) compared to the base branch." }, + { code: "added_test_evidence", title: "Change carries test evidence", severity: "info" as const, detail: "Code changes are accompanied by test evidence." }, + ], + }; + const manyFindingsPanel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: manyFindingsAssessment }); + const manyFindingsRow = manyFindingsPanel.rows.find((r) => r.key === "improvementSignal")!; + // Only the first two finding sentences render inline (none of the four fixtures above set `publicText`, so + // this also exercises the `finding.publicText ?? finding.detail` fallback); the remaining two are + // summarized by count rather than dumped inline (mirrors the "Nits"-style non-inline-dump convention). + expect(manyFindingsRow.cells[2]).toBe( + "2 function(s) have lower cyclomatic complexity after this pull request. 1 previously-duplicated code block(s) were consolidated or removed by this pull request. (+2 more.)", + ); + }); + + it("hides the row via review.fields.improvementSignal: false, exactly like its seven siblings", () => { + const comment = buildPublicPrIntelligenceComment({ + ...improvementBaseArgs, + improvementSignal: minorAssessment, + review: { present: true, fields: { improvementSignal: false } } as unknown as FocusManifestReviewConfig, + env: {}, + }); + expect(comment).not.toContain("| Improvement |"); + }); + + it("REGRESSION (#4744): never leaks forbidden vocabulary regardless of what findings/rationale feed the row (mirrors the repo's existing public-safety invariant tests)", () => { + const unsafeAssessment = { + improvementScore: 40, + band: "moderate" as const, + findings: [ + { + code: "reduced_complexity", + title: "Reward wallet payout", + severity: "info" as const, + detail: "wallet hotkey coldkey trust score reward payout scoreability reviewability farming", + publicText: "wallet hotkey coldkey trust score reward payout scoreability reviewability farming", + }, + ], + }; + const unsafeValueAssessment = { + magnitude: "significant" as const, + rationale: "This raises the wallet hotkey coldkey trust score reward payout scoreability reviewability farming.", + }; + const forbidden = /wallet|hotkey|coldkey|trust score|reward|payout|scoreability|reviewability|farming/i; + + const comment = buildPublicPrIntelligenceComment({ + ...improvementBaseArgs, + improvementSignal: unsafeAssessment, + aiReview: { notes: "Looks fine.", valueAssessment: unsafeValueAssessment }, + env: {}, + }); + expect(comment).toContain("| Improvement | ✅ Moderate |"); // the static band label itself still renders + expect(comment).not.toMatch(forbidden); + + const panel = buildPublicPrPanelSignalRows({ ...improvementBaseArgs, improvementSignal: unsafeAssessment, valueAssessment: unsafeValueAssessment }); + const row = panel.rows.find((r) => r.key === "improvementSignal")!; + expect(JSON.stringify(row)).not.toMatch(forbidden); + // With the one (unsafe) finding filtered out and the (unsafe) valueAssessment dropped, the deterministic + // "no signals" fallback text is what's left -- never an empty cell. + expect(row.cells[2]).toBe("No structural-improvement signals were detected for this PR."); + }); + }); + it("#dup-winner: panel hard-duplicate block is suppressed for the winner, kept for the loser, byte-identical when flag OFF", () => { const directRepo = repo("owner/dupwin"); const dupIssue = issue(directRepo.fullName, 42, "Cache invalidation race"); From 0258ec5c4503d585cc5fb81dee83bff3c28aa1eb Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 19:55:12 -0700 Subject: [PATCH 2/2] fix(review): mirror the improvementSignal fields doc into the other 2 config copies .gittensory.yml.example and src/config/gittensory-repo-focus-manifest.ts's bundled copy already documented the new fields.improvementSignal toggle; .gittensory.yml and config/examples/gittensory.full.yml are two more copies of the same block that config-templates.test.ts and gittensory-focus-manifest.test.ts require to stay aligned. --- .gittensory.yml | 2 +- config/examples/gittensory.full.yml | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.gittensory.yml b/.gittensory.yml index 149046a4c0..3e74dafafe 100644 --- a/.gittensory.yml +++ b/.gittensory.yml @@ -53,7 +53,7 @@ gate: # note: "Run the test suite before requesting review." # short intro line shown above the panel # fields: # show/hide rows (default: all shown). Stable keys: # relatedWork: false # linkedIssue | relatedWork | reviewLoad (Change scope) | -# openPrQueue: false # validationEvidence (Validation posture) | openPrQueue (Contributor workload) | contributorContext | gateResult +# openPrQueue: false # validationEvidence (Validation posture) | openPrQueue (Contributor workload) | contributorContext | gateResult | improvementSignal # AI-review eligibility filters (#3999): a draft PR previously re-triggered a full AI review on every # push, letting a contributor iterate for free while tokens kept burning — skip_drafts stops that. diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index d2296f3be1..95e8fee051 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -1088,7 +1088,10 @@ settings: # text: "Reviewed by the Acme maintainer bot." # Custom lead line. String or null. Default: null. # note: "Run the test suite before requesting review." # Short intro line shown above the panel. String or null. # # Per-row show/hide toggles for the panel. Keys: linkedIssue | relatedWork | reviewLoad | -# # validationEvidence | openPrQueue | contributorContext | gateResult. Default: all shown (true). +# # validationEvidence | openPrQueue | contributorContext | gateResult | improvementSignal. Default: all +# # shown (true). improvementSignal (#4744) only ever renders content when the `improvementSignal` converged +# # feature (see `features:` below) is ALSO active for this repo -- this toggle just hides that row/section +# # like any other; it never turns the feature itself on. # fields: # relatedWork: false # openPrQueue: false