diff --git a/.gittensory.yml.example b/.gittensory.yml.example index d93713c7b8..2ae050901a 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -748,6 +748,10 @@ settings: # # deterministic "Changed files" summary: one row per file category, with counts and +/- totals. Bool # # or null. Default: null/false. # changed_files_summary: false +# # When true, the unified review comment (only rendered when the unifiedComment feature is on) gains a +# # compact "review effort: N/5 (~M min)" chip -- a deterministic, no-AI complexity/time estimate from the +# # changed files' added-line volume and file-type mix. Bool or null. Default: null/false. +# effort_score: false # # When true, an inline finding is ALSO tagged with a category (security/correctness/performance/ # # maintainability/tests/style) -- the AI reviewer self-categorizes, with a deterministic path/keyword # # fallback for whatever it omits. Only takes effect when inline_comments is already on. Bool or null. diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index f9188652f6..e899fd5893 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -761,6 +761,10 @@ settings: # # deterministic "Changed files" summary: one row per file category, with counts and +/- totals. Bool # # or null. Default: null/false. # changed_files_summary: false +# # When true, the unified review comment (only rendered when the unifiedComment feature is on) gains a +# # compact "review effort: N/5 (~M min)" chip -- a deterministic, no-AI complexity/time estimate from the +# # changed files' added-line volume and file-type mix. Bool or null. Default: null/false. +# effort_score: false # # When true, an inline finding is ALSO tagged with a category (security/correctness/performance/ # # maintainability/tests/style) -- the AI reviewer self-categorizes, with a deterministic path/keyword # # fallback for whatever it omits. Only takes effect when inline_comments is already on. Bool or null. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d2ca9c7291..5d9c7bb569 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -335,6 +335,7 @@ import { } from "../signals/engine"; import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner"; import { buildUnifiedReviewDiff } from "../review/review-diff"; +import { estimateReviewEffort } from "../review/review-effort"; import { buildUnifiedCommentBody } from "../review/unified-comment-bridge"; import { randomUUID } from "node:crypto"; import { isRetryableJobError, RetryableJobError } from "./retryable"; @@ -7663,6 +7664,7 @@ async function maybePublishPrPublicSurface( let inlineCommentsEnabledForReview = false; let suggestionsEnabledForReview = false; let changedFilesSummaryEnabledForReview = false; + let effortScoreEnabledForReview = false; let findingCategoriesEnabledForReview = false; let aiReviewExpected = false; let aiReviewWasReused = false; @@ -7804,6 +7806,26 @@ async function maybePublishPrPublicSurface( }).catch(() => undefined); return gateEvaluation; } + // review-effort minutes (#1955): a deterministic, no-AI per-PR estimate persisted onto the SAME published + // event public-stats.ts already reads (github_app.pr_public_surface_published) -- so the public "time saved" + // stat can average a REAL per-PR figure instead of only the flat MINUTES_SAVED_PER_PR fallback constant. + // Computed unconditionally (independent of review.effort_score, which only gates the unified-comment CHIP) + // because public-stats is a cross-repo aggregate with no manifest of its own. Reuses the SAME memoized + // getReviewFiles() accessor the gate/comment pipeline already resolved this pass -- no extra fetch when the + // unified comment already ran; exactly one fetch otherwise. Fail-safe: a files-fetch error here must never + // block the publish audit itself, so a throw degrades to `undefined` (public-stats' own COALESCE fallback + // then applies, same as a pre-#1955 historical row). + const reviewEffortMinutesForStats = await getReviewFiles() + .then((files) => + estimateReviewEffort( + files.map((file) => ({ + path: file.path, + patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, + })), + ), + ) + .then((effort) => effort.minutes) + .catch(() => undefined); await recordAuditEvent(env, { eventType: "github_app.pr_public_surface_published", actor: author, @@ -7820,6 +7842,7 @@ async function maybePublishPrPublicSurface( publishedOutputs, failedOutputs, gateCheckFinalized: gateFinalized, + ...(reviewEffortMinutesForStats !== undefined ? { reviewEffortMinutes: reviewEffortMinutesForStats } : {}), }, }); await recordGithubProductUsage(env, "pr_public_surface_published", { @@ -8129,13 +8152,16 @@ async function maybePublishPrPublicSurface( deliveryId: webhook.deliveryId, headSha: advisory.headSha ?? null, })); - // review.changed_files_summary (#1957): deterministic, no-AI — resolve it here, UNCONDITIONALLY, rather than - // inside the aiReviewWillRun-gated closure below. This table must still render whenever the manifest opts - // in even when the AI review itself is skipped this pass (author blacklisted, frozen for manual review, or - // AI review disabled for the repo) — it has nothing to do with the AI pipeline. Captured into the - // outer-scoped `changedFilesSummaryEnabledForReview` (mirroring inlineCommentsEnabledForReview/ - // suggestionsEnabledForReview) so it survives past this try block to the publish step below. - changedFilesSummaryEnabledForReview = resolveReviewPromptOverrides(reviewManifestForAutoReview).changedFilesSummary; + // review.changed_files_summary (#1957) + review.effort_score (#1955): both deterministic, no-AI — resolve + // them here, UNCONDITIONALLY, rather than inside the aiReviewWillRun-gated closure below. These sections + // must still render whenever the manifest opts in even when the AI review itself is skipped this pass + // (author blacklisted, frozen for manual review, or AI review disabled for the repo) — neither has anything + // to do with the AI pipeline. One resolve call feeds both outer-scoped flags (mirroring + // inlineCommentsEnabledForReview/suggestionsEnabledForReview) so they survive past this try block to the + // publish step below. + const deterministicReviewOverrides = resolveReviewPromptOverrides(reviewManifestForAutoReview); + changedFilesSummaryEnabledForReview = deterministicReviewOverrides.changedFilesSummary; + effortScoreEnabledForReview = deterministicReviewOverrides.effortScore; const aiReviewWillRun = !authorBlacklisted && !isFrozenForManualReview && @@ -9346,6 +9372,21 @@ async function maybePublishPrPublicSurface( })), } : {}), + // review.effort_score (#1955): deterministic, no-AI complexity/time estimate — only computed when the + // manifest opts in (effortScoreEnabledForReview, resolved unconditionally above), mirroring + // changedFilesSummaryEnabledForReview immediately above. Reuses the SAME unifiedFiles this pass already + // resolved (no extra fetch); `patch` comes from the file record's raw payload, the same extraction the AI + // review request already uses (reviewFilesForAi.map above). + ...(effortScoreEnabledForReview + ? { + reviewEffort: estimateReviewEffort( + unifiedFiles.map((file) => ({ + path: file.path, + patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, + })), + ), + } + : {}), ...(findingCategoriesEnabledForReview && aiReview?.inlineFindings?.length ? { findingCategories: aiReview.inlineFindings } : {}), diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 5e2bcda0cf..8ccc639fd9 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -14,7 +14,10 @@ // reviewed = merged + closed + commented (every distinct PR a review surface was published for) // filteredPct = (reviewed - merged) / reviewed (share resolved WITHOUT a merge — noise kept off humans) // accuracyPct = 1 - reversed / (merged + closed) (reversed = engine auto-actions a human overturned, live) -// minutesSaved = reviewed * MINUTES_SAVED_PER_PR (estimated maintainer review time saved) +// minutesSaved = reviewed * avgReviewEffortMinutes (estimated maintainer review time saved -- #1955: the +// real per-PR average of `estimateReviewEffort`'s minutes, +// persisted at publish time; MINUTES_SAVED_PER_PR only +// backstops an empty/all-historical ledger) // // PRIVACY: counts only — no PR content, authors, scores, or reward internals. Safe to serve publicly. // @@ -31,8 +34,11 @@ // excludeAccount dedup this call deliberately does not use. import { getOrbGlobalStats } from "../orb/outcomes"; -/** Estimate of maintainer review/triage time saved per reviewed PR. Dial this to taste — it is the single knob - * behind the "time saved" stat (at current volume: 20 min ≈ 38 days saved; 15 min ≈ 28 days). */ +/** FALLBACK estimate of maintainer review/triage time saved per reviewed PR, used ONLY when the real per-PR + * average (`estimateReviewEffort`'s minutes, persisted at publish time — see `reviewEffortMinutes` in the + * `github_app.pr_public_surface_published` audit metadata) is unavailable: an empty allowlist, or a ledger whose + * published rows all predate this feature. (#1955 — previously the ONLY figure behind "time saved"; kept as the + * documented degrade rather than removed, since a historical ledger genuinely has no other number to report.) */ export const MINUTES_SAVED_PER_PR = 20; /** Truthy-string flag check, matching ops-wire / selftune-wire. */ @@ -179,11 +185,12 @@ export async function getPublicStats( // The own-ledger side needs at least one allowlisted project to query; an empty allowlist skips these three // queries entirely (own-ledger totals stay zero) but still lets the Orb aggregate below run. const inList = projects.map(() => "?").join(", "); - const [dispositions, reversalRows, weeklyRows] = projects.length === 0 + const [dispositions, reversalRows, weeklyRows, effortRows] = projects.length === 0 ? await Promise.all([ Promise.resolve([]), Promise.resolve<{ project: string; reversed: number }[]>([]), Promise.resolve<{ reviewed: number; merged: number }[]>([]), + Promise.resolve<{ avgMinutes: number | null }[]>([]), ]) : await Promise.all([ safeAll( @@ -237,6 +244,21 @@ export async function getPublicStats( sinceIso, ...projects, ), + // review-effort minutes (#1955): a deterministic, no-AI per-PR estimate persisted at publish time + // (processors.ts's pr_public_surface_published metadata.reviewEffortMinutes). AVG over every published event + // in the allowlist gives a real per-review time figure; a published row that predates this feature (or a + // files-fetch failure at publish time) simply has no `reviewEffortMinutes` key, so json_extract returns SQL + // NULL for that row and SQLite's AVG silently skips it — an all-historical ledger degrades to a NULL average + // (handled below via `?? MINUTES_SAVED_PER_PR`), never a crash or a skewed zero. + safeAll<{ avgMinutes: number | null }>( + env, + `SELECT AVG(json_extract(metadata_json, '$.reviewEffortMinutes')) AS avgMinutes + FROM audit_events + WHERE event_type = 'github_app.pr_public_surface_published' + AND LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) IN (${inList}) + AND instr(target_key, '#') > 0`, + ...projects, + ), ]); const reversedByProject = new Map( @@ -291,6 +313,11 @@ export async function getPublicStats( const reviewed = reviewedOf(totals); const w = weeklyRows[0] ?? { reviewed: 0, merged: 0 }; + // review-effort minutes (#1955): prefer the REAL average per-review estimate (persisted at publish time from + // estimateReviewEffort); an empty/all-null ledger (no allowlisted project, or every published row predates + // this feature) falls back to the flat MINUTES_SAVED_PER_PR constant, exactly like every other nullish-SUM + // fallback in this module (`?? 0`) — never a crash, never a skewed zero. + const avgReviewEffortMinutes = effortRows[0]?.avgMinutes ?? MINUTES_SAVED_PER_PR; return { generatedAt, updatedAt: generatedAt, @@ -299,7 +326,7 @@ export async function getPublicStats( reviewed, filteredPct: filteredPct(reviewed, totals.merged), accuracyPct: accuracyPct(totals.merged, totals.closed, totals.reversed), - minutesSaved: reviewed * MINUTES_SAVED_PER_PR, + minutesSaved: Math.round(reviewed * avgReviewEffortMinutes), }, weekly: { reviewed: w.reviewed ?? 0, merged: w.merged ?? 0 }, byProject, diff --git a/src/review/stats.ts b/src/review/stats.ts index 04cff5055f..f8346fc5be 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -15,6 +15,20 @@ // are NOT part of this aggregation and are heavily entangled with the gate. They are taken as INJECTED // deps so the core decision/reversal/gate-action aggregation is fully native here. The host wires its own // implementations (or the defaults below, which emit empty/no-signal reports, keeping the payload shape). +// +// SCOPE NOTE (#1955 — deterministic review-effort score): this feed's tables (`review_targets`, `review_audit`, +// and the injected eval/parity engine's own source, `review_audit`'s `gate_decision`/`pr_outcome` rows) are the +// LEGACY reviewbot ledger — nothing writes new rows to `review_targets` since the self-host convergence cutover +// (see public-stats.ts's file header), and every write into `review_audit` (outcomes-wire.ts's `pr_outcome`, +// the gate's `gate_decision`) carries only decision/outcome metadata, never a PR's changed files or patches. +// There is therefore NO live source this module could aggregate a per-PR review-effort estimate FROM today — +// unlike public-stats.ts's `getPublicStats`, which reads the ACTIVE `audit_events` ledger the live review +// pipeline (src/queue/processors.ts) still writes to every publish, and where `estimateReviewEffort`'s minutes +// are now persisted (`reviewEffortMinutes` in `github_app.pr_public_surface_published` metadata) and averaged. +// Wiring a same-shaped aggregate here would only ever read back a permanently-null placeholder — scaffolding +// with no live behavior — so it is deliberately left out of this change rather than faked. A REAL maintainer- +// dashboard effort aggregate needs its own persisted source (e.g. this module reading `audit_events` the way +// public-stats.ts now does), which is a genuine follow-up, not a one-line addition to this file. // ── Inlined report types (ported shapes from reviewbot src/core/{eval,tuning}.ts) ──────────────── diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 272a041e77..e491b1d47f 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -314,6 +314,11 @@ export type UnifiedCommentBridgeArgs = { * passes this only when the manifest opts in — see `resolveReviewPromptOverrides`'s `changedFilesSummary`). * (#1957) */ changedFilesSummary?: ChangedFileSummaryInput[] | undefined; + /** Deterministic per-PR review-effort estimate (review.effort_score port, `src/review/review-effort.ts`). When + * present, a compact `review effort: N/5 (~M min)` chip is appended to the status-chip row (passed straight + * through to `buildUnifiedReviewInput`'s `reviewEffort`). No AI. Default OFF (the processor passes this only + * when the manifest opts in — see `resolveReviewPromptOverrides`'s `effortScore`). (#1955) */ + reviewEffort?: { band: 1 | 2 | 3 | 4 | 5; minutes: number } | undefined; /** Line-anchored AI findings, one entry per inline finding (review.finding_categories port). When present + * non-empty, a "Finding categories" collapsible (a count per security/correctness/performance/maintainability/ * tests/style category) is appended. A finding missing its own `category` falls back to @@ -497,6 +502,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string ...(verdictReason !== undefined ? { verdictReason } : {}), ...(args.mergeReadiness !== undefined ? { readiness: args.mergeReadiness } : {}), ...(args.merged !== undefined ? { merged: args.merged } : {}), + ...(args.reviewEffort !== undefined ? { reviewEffort: args.reviewEffort } : {}), }); // The gate already produced 0/1 reviewer notes from a synthesis of the model pair; reflect the caller's // actual reviewer count (for the chip + the "N reviewers, synthesized" evidence) without re-deriving it. diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index a2e73bb14d..de3632463c 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -166,6 +166,12 @@ export interface UnifiedReviewInput { consensusBlocker?: boolean; /** Reviewers that produced no parseable verdict (a partial review → held, not ready). */ failedCount?: number; + /** Deterministic per-PR review-effort estimate (`estimateReviewEffort`, `src/review/review-effort.ts`) — a + * 1-5 complexity band + a minutes estimate from the changed files' added-line volume and file-type mix. No + * AI. Rendered as a compact `review effort: N/5 (~M min)` chip only when the host passes this (gated by + * `review.effort_score` — see `resolveReviewPromptOverrides`'s `effortScore`); omitted ⇒ no chip + * (byte-identical). (#1955) */ + reviewEffort?: { band: 1 | 2 | 3 | 4 | 5; minutes: number }; } /** One row of the readiness signal table (gittensory side, host-provided; the engine adds Code review). */ @@ -323,6 +329,9 @@ function statusChips(input: UnifiedReviewInput, ctx: UnifiedCommentContext): str chips.push(ci === "passed" ? "`CI green`" : ci === "failed" ? "`CI failing`" : "`CI pending`"); if (input.readiness.mergeStateLabel) chips.push(`\`${escapePublicHtmlAngles(input.readiness.mergeStateLabel)}\``); } + // review.effort_score (#1955): deterministic, no-AI — only rendered when the host resolved + passed it + // (gated by the manifest toggle). Absent ⇒ no chip (byte-identical). + if (input.reviewEffort) chips.push(`\`review effort: ${input.reviewEffort.band}/5 (~${input.reviewEffort.minutes} min)\``); return chips.join(" · "); } @@ -544,6 +553,7 @@ export function buildUnifiedReviewInput(opts: { decision?: Verdict; merged?: boolean; verdictReason?: string; + reviewEffort?: { band: 1 | 2 | 3 | 4 | 5; minutes: number }; }): UnifiedReviewInput { const ex = extractReviewSummary(opts.reviews); const changedFiles = typeof opts.changedFiles === "number" ? opts.changedFiles : opts.changedFiles.length; @@ -560,6 +570,7 @@ export function buildUnifiedReviewInput(opts: { ...(opts.decision !== undefined ? { decision: opts.decision } : {}), ...(opts.merged !== undefined ? { merged: opts.merged } : {}), ...(opts.verdictReason !== undefined ? { verdictReason: opts.verdictReason } : {}), + ...(opts.reviewEffort !== undefined ? { reviewEffort: opts.reviewEffort } : {}), }; } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 62cc8d8a90..aef7af9452 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -329,6 +329,14 @@ export type FocusManifestReviewConfig = { * existing `classifyChangedFile` classifier (`src/review/changed-files-classify.ts`, built for this table * under #2143). null/false (default, absent) = no changed-files section = byte-identical behavior. (#1957) */ changedFilesSummary: boolean | null; + /** `review.effort_score`: when true, the unified review comment (only rendered when the `unifiedComment` + * convergence feature is on) gains a compact "review effort: N/5 (~M min)" chip — a deterministic, no-AI + * complexity/time estimate from `estimateReviewEffort` (`src/review/review-effort.ts`), weighting each + * changed file's added lines by its category (source costs most; generated/vendored/lockfiles cost least) + * plus a fixed per-file overhead. Mirrors `changedFilesSummary` exactly: same table, same deterministic + * source, same display-only (never touches the AI prompt) shape. null/false (default, absent) = no chip = + * byte-identical behavior. (#1955) */ + effortScore: boolean | null; /** `review.finding_categories`: when true, an inline finding is ALSO tagged with a category (security/ * correctness/performance/maintainability/tests/style) — the AI reviewer is asked to self-categorize, with a * deterministic path/keyword fallback (`classifyFindingCategory`) covering whatever it omits. Only takes @@ -642,7 +650,7 @@ const EMPTY_MANIFEST: FocusManifest = { publicNotes: [], gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -672,7 +680,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { ...EMPTY_FEATURES_CONFIG }, contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, repoDocGeneration: { ...EMPTY_REPO_DOC_GENERATION_CONFIG }, @@ -1603,7 +1611,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin * throws; invalid/unsafe values are dropped with warnings. */ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig { - const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; + const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }; if (value === undefined || value === null) return empty; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review" must be a mapping; ignoring it.`); @@ -1642,6 +1650,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const inlineComments = normalizeOptionalBoolean(r.inline_comments, "review.inline_comments", warnings); const suggestions = normalizeOptionalBoolean(r.suggestions, "review.suggestions", warnings); const changedFilesSummary = normalizeOptionalBoolean(r.changed_files_summary, "review.changed_files_summary", warnings); + const effortScore = normalizeOptionalBoolean(r.effort_score, "review.effort_score", warnings); const findingCategories = normalizeOptionalBoolean(r.finding_categories, "review.finding_categories", warnings); const pathInstructions = parseReviewPathInstructions(r.path_instructions, warnings); const instructions = parsePublicSafeText(r.instructions, "review.instructions", warnings); @@ -1663,6 +1672,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo inlineComments !== null || suggestions !== null || changedFilesSummary !== null || + effortScore !== null || findingCategories !== null || pathInstructions.length > 0 || instructions !== null || @@ -1690,6 +1700,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo inlineComments, suggestions, changedFilesSummary, + effortScore, findingCategories, pathInstructions, instructions, @@ -2068,6 +2079,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.inlineComments !== null) out.inline_comments = review.inlineComments; if (review.suggestions !== null) out.suggestions = review.suggestions; if (review.changedFilesSummary !== null) out.changed_files_summary = review.changedFilesSummary; + if (review.effortScore !== null) out.effort_score = review.effortScore; if (review.findingCategories !== null) out.finding_categories = review.findingCategories; if (review.instructions !== null) out.instructions = review.instructions; if (review.pathInstructions.length > 0) out.path_instructions = review.pathInstructions.map((entry) => ({ path: entry.path, instructions: entry.instructions })); @@ -2219,7 +2231,7 @@ export function composeManifestReviewInstructions(instructions: string | null, t * failure). A null manifest yields the byte-identical defaults. Centralized so the AI-review caller threads them * in one place with the null-manifest branch covered here (unit-tested) rather than inline in the processor. * (#review-profile / #review-tone / #review-security-focus / #review-path-instructions / #review-exclude-paths / #2043 / #selfhost-ai-model-override / #1956) */ -export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; changedFilesSummary: boolean; findingCategories: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { +export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { profile: ReviewProfile | null; tone: string | null; securityFocus: boolean; inlineComments: boolean; suggestions: boolean; changedFilesSummary: boolean; effortScore: boolean; findingCategories: boolean; pathInstructions: ReviewPathInstruction[]; instructions: string | null; excludePaths: string[]; pathFilters: string[]; selfHostAiModel: SelfHostAiModelConfig } { // inlineComments resolves to a strict boolean — true ONLY when the manifest explicitly set review.inline_comments: // true; null/false/absent ⇒ false. The caller ANDs this per-repo toggle with the operator flag + cutover allowlist. // securityFocus resolves the same way — true ONLY when the manifest explicitly set review.security_focus: true. @@ -2227,9 +2239,11 @@ export function resolveReviewPromptOverrides(manifest: FocusManifest | null): { // inlineComments gate, since a suggestion has nothing to attach to without an inline comment. // changedFilesSummary resolves the same way (#1957) — independent of inlineComments/suggestions; it only // needs the unified-comment convergence feature itself to be on (the caller's own outer gate). + // effortScore resolves the same way (#1955) — like changedFilesSummary, it is deterministic/display-only + // (never touches the AI prompt) and only needs the unified-comment convergence feature to be on. // findingCategories resolves the same way (#1958) — like suggestions, the caller further ANDs it with the // already-resolved inlineComments gate, since a category has nothing to categorize without an inline finding. - return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, findingCategories: manifest?.review.findingCategories === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; + return { profile: manifest?.review.profile ?? null, tone: manifest?.review.tone ?? null, securityFocus: manifest?.review.securityFocus === true, inlineComments: manifest?.review.inlineComments === true, suggestions: manifest?.review.suggestions === true, changedFilesSummary: manifest?.review.changedFilesSummary === true, effortScore: manifest?.review.effortScore === true, findingCategories: manifest?.review.findingCategories === true, pathInstructions: manifest?.review.pathInstructions ?? [], instructions: manifest?.review.instructions ?? null, excludePaths: manifest?.review.excludePaths ?? [], pathFilters: manifest?.review.pathFilters ?? [], selfHostAiModel: resolveReviewSelfHostAiModel(manifest) }; } /** Resolve `review.pre_merge_checks` from a possibly-null manifest (null = load failure ⇒ no checks). Centralized diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index d1c6ca04ce..f1c99b1678 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -351,6 +351,7 @@ describe(".gittensory.yml.example field-exhaustiveness (#1670)", () => { inlineComments: "inline_comments:", suggestions: "suggestions:", changedFilesSummary: "changed_files_summary:", + effortScore: "effort_score:", findingCategories: "finding_categories:", pathInstructions: "path_instructions:", instructions: "instructions:", @@ -762,7 +763,7 @@ describe("compileFocusManifestPolicy", () => { publicNotes: ["Keep PRs focused.", "Maximize your reward payout"], gate: { present: false, enabled: null, checkMode: null, pack: null, linkedIssue: null, duplicates: null, readinessMode: null, readinessMinScore: null, slopMode: null, slopMinScore: null, slopAiAdvisory: null, sizeMode: null, lockfileIntegrityMode: null, aiReviewMode: null, aiReviewByok: null, aiReviewProvider: null, aiReviewModel: null, aiReviewAllAuthors: null, aiReviewCloseConfidence: null, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, mergeReadiness: null, selfAuthoredLinkedIssue: null, manifestPolicy: null, dryRun: null, firstTimeContributorGrace: null, premergeContentRecheck: null, requireFreshRebaseWindowMinutes: null, claMode: null, claConsentPhrase: null, claCheckRunName: null, claCheckRunAppSlug: null, expectedCiContexts: null }, settings: {}, - review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, + review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, labelingRules: [], aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, repoDocGeneration: { present: false, enabled: false, scope: ["agents"], allowOverwriteExisting: false, refreshIntervalDays: 7 }, @@ -2866,10 +2867,10 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { }); it("resolveReviewPromptOverrides: non-null manifest passes the config through; null manifest → defaults", () => { - const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, suggestions: true, changed_files_summary: true, finding_categories: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); - expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, changedFilesSummary: true, findingCategories: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); - // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + changed-files summary + finding categories + security focus default OFF. - expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, findingCategories: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + const manifest = parseFocusManifest({ review: { profile: "chill", security_focus: true, inline_comments: true, suggestions: true, changed_files_summary: true, effort_score: true, finding_categories: true, path_instructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", exclude_paths: ["**/*.lock"], path_filters: ["src/**", "!src/generated/**"] } }); + expect(resolveReviewPromptOverrides(manifest)).toEqual({ profile: "chill", tone: null, securityFocus: true, inlineComments: true, suggestions: true, changedFilesSummary: true, effortScore: true, findingCategories: true, pathInstructions: [{ path: "src/**", instructions: "be strict" }], instructions: "Follow our async-error conventions.", excludePaths: ["**/*.lock"], pathFilters: ["src/**", "!src/generated/**"], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); + // A null manifest (load failure) yields the byte-identical defaults; inline comments + suggestions + changed-files summary + effort score + finding categories + security focus default OFF. + expect(resolveReviewPromptOverrides(null)).toEqual({ profile: null, tone: null, securityFocus: false, inlineComments: false, suggestions: false, changedFilesSummary: false, effortScore: false, findingCategories: false, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], selfHostAiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG } }); // An explicit false / absent toggle both resolve to the strict-boolean false. expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { inline_comments: false } })).inlineComments).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).inlineComments).toBe(false); @@ -2877,6 +2878,8 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).suggestions).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { changed_files_summary: false } })).changedFilesSummary).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).changedFilesSummary).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { effort_score: false } })).effortScore).toBe(false); + expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).effortScore).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { finding_categories: false } })).findingCategories).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { profile: "chill" } })).findingCategories).toBe(false); expect(resolveReviewPromptOverrides(parseFocusManifest({ review: { security_focus: false } })).securityFocus).toBe(false); @@ -2934,6 +2937,23 @@ describe("resolveReviewPathInstructions (#review-path-instructions)", () => { expect(bad.warnings.some((w) => /review\.changed_files_summary.*must be a boolean/.test(w))).toBe(true); }); + it("parses review.effort_score (default OFF), marks present, round-trips, and warns on a non-boolean (#1955)", () => { + expect(parseFocusManifest({ review: { effort_score: true } }).review.effortScore).toBe(true); + const on = parseFocusManifest({ review: { effort_score: true } }); + expect(on.review.present).toBe(true); // an effort-score-only manifest IS present + expect(parseFocusManifest({ review: reviewConfigToJson(on.review) }).review).toEqual(on.review); // survives round-trip + // Explicit false is retained (and marks present, since the maintainer set it). + const off = parseFocusManifest({ review: { effort_score: false } }); + expect(off.review.effortScore).toBe(false); + expect(off.review.present).toBe(true); + // Absent ⇒ null (the byte-identical default), config not present. + expect(parseFocusManifest({ review: {} }).review.effortScore).toBeNull(); + // A non-boolean is ignored with a warning. + const bad = parseFocusManifest({ review: { effort_score: "yes" } }); + expect(bad.review.effortScore).toBeNull(); + expect(bad.warnings.some((w) => /review\.effort_score.*must be a boolean/.test(w))).toBe(true); + }); + it("parses review.finding_categories (default OFF), marks present, round-trips, and warns on a non-boolean (#1958)", () => { expect(parseFocusManifest({ review: { finding_categories: true } }).review.findingCategories).toBe(true); const on = parseFocusManifest({ review: { finding_categories: true } }); diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index 5c1f127f70..1e5516052f 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -30,9 +30,15 @@ const NOW = Date.parse("2026-06-22T00:00:00Z"); function isWeekly(sql: string): boolean { return sql.includes("first_seen"); } +// The effort-minutes read is the only one that extracts reviewEffortMinutes from metadata_json (#1955). +function isEffort(sql: string): boolean { + return sql.includes("reviewEffortMinutes"); +} function isDispositions(sql: string): boolean { return ( - sql.includes("github_app.pr_public_surface_published") && !isWeekly(sql) + sql.includes("github_app.pr_public_surface_published") && + !isWeekly(sql) && + !isEffort(sql) ); } // The reversal read is the only one that inspects engine auto-actions (close/merge) against pull_requests state. @@ -117,6 +123,32 @@ describe("getPublicStats — live aggregate over the review ledger", () => { expect(out.updatedAt).toBe(out.generatedAt); }); + // #1955: minutesSaved now averages the REAL per-PR estimate (estimateReviewEffort's minutes, persisted at + // publish time) instead of unconditionally multiplying by the flat MINUTES_SAVED_PER_PR constant. Proves the + // new estimate is actually used when the ledger has it — the regression case for the flat-constant replacement. + it("uses the real average review-effort minutes when the ledger has them, instead of the flat constant", async () => { + const withEffort = (sql: string): Row[] => { + if (isEffort(sql)) return [{ avgMinutes: 7.4 }]; + return ledger(sql); + }; + const out = await getPublicStats(stubEnv(withEffort), NOW); + // reviewed = 2742 (same ledger as the base test) * 7.4 = 20290.8 -> rounded. + expect(out.totals.minutesSaved).toBe(Math.round(2742 * 7.4)); + expect(out.totals.minutesSaved).not.toBe(2742 * MINUTES_SAVED_PER_PR); + }); + + // The nullish arm of `effortRows[0]?.avgMinutes ?? MINUTES_SAVED_PER_PR`: a ledger whose published rows all + // predate this feature (or an empty allowlist) yields a NULL average (SQLite's AVG skips missing json_extract + // keys entirely) rather than a row missing outright — both must degrade to the flat constant, not NaN/0. + it("falls back to the flat MINUTES_SAVED_PER_PR constant when the effort average is SQL NULL", async () => { + const nullEffort = (sql: string): Row[] => { + if (isEffort(sql)) return [{ avgMinutes: null }]; + return ledger(sql); + }; + const out = await getPublicStats(stubEnv(nullEffort), NOW); + expect(out.totals.minutesSaved).toBe(2742 * MINUTES_SAVED_PER_PR); + }); + it("breaks byProject ties on project name so equal-reviewed repos keep a deterministic order", async () => { // Two repos share reviewed=10, fed in reverse-alphabetical input order; the busier repo // still leads and the tied pair must come out alphabetically, not in arbitrary SQL order. @@ -290,6 +322,60 @@ describe("getPublicStats — live aggregate over the review ledger", () => { expect(out.totals.accuracyPct).toBe(100); }); + // #1955: end-to-end over REAL D1/SQLite (not the stub) — a published row's `metadata_json.reviewEffortMinutes` + // (the exact shape processors.ts writes at publish time) round-trips through json_extract/AVG into + // minutesSaved, proving the SQL itself (not just the mocked shape) computes the real per-PR average. + it("averages a real reviewEffortMinutes value out of metadata_json via json_extract (real D1)", async () => { + const env = createTestEnv({ GITTENSORY_PUBLIC_STATS_REPOS: "JSONbored/gittensory" }); + const db = env.DB; + + await db + .prepare( + `INSERT INTO pull_requests (id, repo_full_name, number, title, state, merged_at) + VALUES (?, ?, ?, ?, ?, ?), (?, ?, ?, ?, ?, ?)`, + ) + .bind( + "pr-a", + "JSONbored/gittensory", + 10, + "small fix", + "closed", + "2026-06-01T00:00:00.000Z", + "pr-b", + "JSONbored/gittensory", + 11, + "bigger change", + "closed", + "2026-06-01T00:00:00.000Z", + ) + .run(); + await db + .prepare( + `INSERT INTO audit_events (id, event_type, target_key, outcome, metadata_json) + VALUES (?, ?, ?, ?, ?), (?, ?, ?, ?, ?)`, + ) + .bind( + "published-a", + "github_app.pr_public_surface_published", + "JSONbored/gittensory#10", + "completed", + JSON.stringify({ reviewEffortMinutes: 4 }), + "published-b", + "github_app.pr_public_surface_published", + "JSONbored/gittensory#11", + "completed", + JSON.stringify({ reviewEffortMinutes: 96 }), + ) + .run(); + + const out = await getPublicStats(env, NOW); + + // avg(4, 96) = 50; reviewed = 2 -> minutesSaved = 100 (not 2 * MINUTES_SAVED_PER_PR = 40). + expect(out.totals.reviewed).toBe(2); + expect(out.totals.minutesSaved).toBe(100); + expect(out.totals.minutesSaved).not.toBe(2 * MINUTES_SAVED_PER_PR); + }); + it("skips the own-ledger queries but still queries the Orb aggregate when the allowlist is empty", async () => { const env = { DB: { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 36d54ca3dc..afac69578f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -5,6 +5,7 @@ import { PR_PANEL_COMMENT_MARKER } from "../../src/github/comments"; import * as backfillModule from "../../src/github/backfill"; import * as rateLimitModule from "../../src/github/rate-limit"; import * as repositoriesModule from "../../src/db/repositories"; +import * as reviewEffortModule from "../../src/review/review-effort"; import * as repositorySettingsModule from "../../src/settings/repository-settings"; import * as sentryModule from "../../src/selfhost/sentry"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; @@ -15501,6 +15502,9 @@ describe("queue processors", () => { // #review-audit (#4220): the comment reads the LIVE `dirty` merge-state (not the stale stored one), so it must // NOT headline "safe to merge" while the disposition would auto-close the base-conflicting PR. expect(postedBody).not.toMatch(/safe to merge/i); + // #1955: no `.gittensory.yml` was fetched here (the raw-content URL isn't stubbed, so it 404s and the + // manifest resolves to null) — review.effort_score is absent/default OFF, so the effort chip must NOT render. + expect(postedBody).not.toMatch(/review effort:/); } finally { liveCiSpy.mockRestore(); } @@ -15829,6 +15833,247 @@ describe("queue processors", () => { } }); + // #1955: with the unified comment on AND `.gittensory.yml` opting into `review.effort_score`, the rendered + // comment gains the deterministic, no-AI "review effort: N/5 (~M min)" chip — computed by estimateReviewEffort + // from the SAME PR-files fetch the unified branch already does (no separate call). Mirrors the + // changed_files_summary test above but asserts the effort chip's presence + exact value instead. + it("renders the review effort chip when review.effort_score is on in .gittensory.yml", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + 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", + backfillEnabled: true, + privateTrustEnabled: 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" }]); + // .gittensory.yml opts into the deterministic effort score — no AI involved. + if (url === "https://raw.githubusercontent.com/JSONbored/gittensory/HEAD/.gittensory.yml") { + return new Response("review:\n effort_score: true\n"); + } + 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 — the unified branch (re)fetches them to count changed files AND (with the toggle above) to + // compute the effort estimate. A 10-added-line source file WITH a patch (weighted 10) plus a docs file with + // NO `patch` field (exercises the `typeof file.payload?.patch === "string" ? ... : undefined` fallback -> + // addedLineCount(undefined) = 0, so it contributes 0 weighted lines but still its per-file overhead): + // weighted 10 + 0 + 2 files * 3 overhead = effort 16 -> band 2, minutes round(16 * 0.5) = 8 + // (see estimateReviewEffort — src/review/review-effort.ts). + if (url.includes("/pulls/3/files")) + return Response.json([ + { + filename: "src/cache.ts", + additions: 10, + deletions: 1, + status: "modified", + patch: `@@ -1,1 +1,11 @@\n${Array.from({ length: 10 }, (_, i) => `+const x${i} = ${i};`).join("\n")}`, + }, + { filename: "README.md", additions: 2, deletions: 0, status: "modified" }, + ]); + if (/\/pulls\/3(?:\?|$)/.test(url)) return Response.json({ number: 3, mergeable_state: "clean" }); + // Gate check-run — must succeed so `gateEvaluation` is produced and the flag-ON branch runs. + // The pending check is POSTed (in_progress), then PATCHed to its completed conclusion. + 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/3/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/3/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-unified-comment-effort-score", + 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: 3, + title: "Fix webhook duplicate delivery again", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "unified789" }, + labels: [{ name: "bug" }], + body: "Fixes #1\n\nValidation: npm test", + }, + }, + }); + + expect(calls.comments).toBe(2); + expect(postedBody).toContain(""); + // The new deterministic, no-AI chip: band 2 (effort 16 <= BAND_MAX[1]=40), minutes round(16*0.5)=8. + expect(postedBody).toContain("`review effort: 2/5 (~8 min)`"); + } finally { + liveCiSpy.mockRestore(); + } + }); + + // #1955: the review-effort minutes persisted onto the public-stats audit event (independent of + // review.effort_score, which only gates the unified-comment CHIP) must never block the publish itself when the + // estimator throws — the publish still completes and simply omits `reviewEffortMinutes` from the event metadata + // (public-stats.ts's own COALESCE-style fallback then applies, same as a pre-#1955 historical row). + it("swallows an estimateReviewEffort failure when persisting the public-stats minutes — the publish still completes", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", commentMode: "all_prs", publicSurface: "comment_only", autoLabelEnabled: false, checkRunMode: "off", gateCheckMode: "enabled", aiReviewMode: "off", gatePack: "oss-anti-slop" }); + const estimateSpy = vi.spyOn(reviewEffortModule, "estimateReviewEffort").mockImplementationOnce(() => { + throw new Error("estimator blew up"); + }); + let commentPosted = false; + let publishedMetadata: Record | undefined; + const originalRecordAuditEvent = repositoriesModule.recordAuditEvent; + const auditSpy = vi.spyOn(repositoriesModule, "recordAuditEvent").mockImplementation(async (auditEnv, event) => { + if (event.eventType === "github_app.pr_public_surface_published") { + publishedMetadata = event.metadata as Record; + } + await originalRecordAuditEvent(auditEnv, event); + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/8/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/8")) return Response.json({ number: 8, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a8" }, labels: [], body: "Closes #1" }); + if (url.includes("/commits/a8/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a8/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/8/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/8/comments") && method === "POST") { commentPosted = true; return Response.json({ id: 1 }, { status: 201 }); } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + try { + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "effort-estimator-throws", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 8, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a8" }, labels: [], body: "Closes #1" }, + }, + }), + ).resolves.toBeUndefined(); + + expect(commentPosted).toBe(true); // the publish completed despite the estimator throwing + expect(estimateSpy).toHaveBeenCalled(); + expect(publishedMetadata).toBeDefined(); + expect(publishedMetadata).not.toHaveProperty("reviewEffortMinutes"); + } finally { + estimateSpy.mockRestore(); + auditSpy.mockRestore(); + } + }); + // #1958: with inline comments AND finding categories both on in .gittensory.yml (finding_categories rides on // inline_comments, exactly like suggestions did for #1956), the model is asked to self-categorize each // inlineFindings item, and BOTH surfaces render it — the posted inline review comment label AND the unified diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 384e49e994..316d594511 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }, linkedIssueSatisfaction: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index bd618829e8..06e062808a 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -275,6 +275,28 @@ describe("buildUnifiedCommentBody", () => { expect(body).toContain("> [!TIP]"); // success → ready → TIP alert }); + it("forwards the reviewEffort estimate into the rendered chip when present, and omits it otherwise (#1955)", () => { + const withEffort = buildUnifiedCommentBody({ + gate: gate(), + aiReview: { notes: "Clean change." }, + panelRows, + readinessTotal: 88, + changedFiles: 3, + footerMarkdown: footer, + reviewEffort: { band: 2, minutes: 12 }, + }); + expect(withEffort).toContain("`review effort: 2/5 (~12 min)`"); + const withoutEffort = buildUnifiedCommentBody({ + gate: gate(), + aiReview: { notes: "Clean change." }, + panelRows, + readinessTotal: 88, + changedFiles: 3, + footerMarkdown: footer, + }); + expect(withoutEffort).not.toContain("review effort:"); + }); + it("passes a public review update timestamp into the unified comment", () => { const body = buildUnifiedCommentBody({ gate: gate(), diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 9b555addd4..6f5d1127d6 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -269,6 +269,14 @@ describe("renderUnifiedReviewComment", () => { expect(pending).toContain("`CI pending`"); }); + it("renders the review-effort chip when present, and omits it entirely when absent (#1955)", () => { + const withEffort = renderUnifiedReviewComment({ ...base, reviewEffort: { band: 3, minutes: 42 } }, {}); + expect(withEffort).toContain("`review effort: 3/5 (~42 min)`"); + // Byte-identical-when-off: no reviewEffort field at all ⇒ the chip text never appears. + const withoutEffort = renderUnifiedReviewComment({ ...base }, {}); + expect(withoutEffort).not.toContain("review effort:"); + }); + it("lists failing check names + per-check details under a 'CI checks failing' section (FIX D3)", () => { const md = renderUnifiedReviewComment( { @@ -470,6 +478,17 @@ describe("buildUnifiedReviewInput", () => { expect(input.merged).toBe(true); expect(input.verdictReason).toBe("auto-merged after green CI"); }); + + it("threads the optional reviewEffort estimate through to the input when provided, omits it otherwise (#1955)", () => { + const withEffort = buildUnifiedReviewInput({ + changedFiles: 1, + reviews: [reviewNote("merge")], + reviewEffort: { band: 4, minutes: 90 }, + }); + expect(withEffort.reviewEffort).toEqual({ band: 4, minutes: 90 }); + const withoutEffort = buildUnifiedReviewInput({ changedFiles: 1, reviews: [reviewNote("merge")] }); + expect(withoutEffort.reviewEffort).toBeUndefined(); + }); }); describe("renderReviewingPlaceholder", () => {