Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .gittensory.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions config/examples/gittensory.full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
55 changes: 48 additions & 7 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -7820,6 +7842,7 @@ async function maybePublishPrPublicSurface(
publishedOutputs,
failedOutputs,
gateCheckFinalized: gateFinalized,
...(reviewEffortMinutesForStats !== undefined ? { reviewEffortMinutes: reviewEffortMinutesForStats } : {}),
},
});
await recordGithubProductUsage(env, "pr_public_surface_published", {
Expand Down Expand Up @@ -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 &&
Expand Down Expand Up @@ -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 }
: {}),
Expand Down
37 changes: 32 additions & 5 deletions src/review/public-stats.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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. */
Expand Down Expand Up @@ -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<DispositionRow[]>([]),
Promise.resolve<{ project: string; reversed: number }[]>([]),
Promise.resolve<{ reviewed: number; merged: number }[]>([]),
Promise.resolve<{ avgMinutes: number | null }[]>([]),
])
: await Promise.all([
safeAll<DispositionRow>(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
14 changes: 14 additions & 0 deletions src/review/stats.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) ────────────────

Expand Down
6 changes: 6 additions & 0 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions src/review/unified-comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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). */
Expand Down Expand Up @@ -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(" · ");
}

Expand Down Expand Up @@ -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;
Expand All @@ -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 } : {}),
};
}

Expand Down
Loading
Loading