Skip to content

feat(review): add deterministic review-effort chip and wire real per-PR minutes - #3666

Merged
JSONbored merged 2 commits into
mainfrom
claude/orb-phase1-1955
Jul 6, 2026
Merged

feat(review): add deterministic review-effort chip and wire real per-PR minutes#3666
JSONbored merged 2 commits into
mainfrom
claude/orb-phase1-1955

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Closes #1955

Adds a deterministic, pure per-PR review-effort/complexity score to the review surfaces. The estimator itself (estimateReviewEffort — 1-5 band + minutes, weighted by added-line volume and file-type mix, no AI) already shipped in a prior PR (src/review/review-effort.ts, #2924/#2068); this PR wires it into the three places that were not yet done:

  • review.effort_score — a new FocusManifestReviewConfig boolean toggle, wired end-to-end exactly like its sibling review.changed_files_summary (parse/serialize/resolve in src/signals/focus-manifest.ts, the .gittensory.yml.example reference block, and the REVIEW_FIELD_TOKENS exhaustiveness map in test/unit/focus-manifest.test.ts). Default off/null, byte-identical when absent.
  • Unified comment render — when the toggle is on, src/review/unified-comment.ts's statusChips() appends a compact `review effort: N/5 (~M min)` chip (mirroring how the readiness/CI chips are conditionally appended), threaded through buildUnifiedReviewInputunified-comment-bridge.tssrc/queue/processors.ts (resolved unconditionally alongside changedFilesSummaryEnabledForReview, since it is deterministic and has nothing to do with whether the AI review ran).
  • Public statssrc/review/public-stats.ts's MINUTES_SAVED_PER_PR flat constant is no longer the only figure behind "time saved": the processor now persists estimateReviewEffort's minutes onto the same github_app.pr_public_surface_published audit event public-stats already reads, and getPublicStats averages the real per-PR minutes via AVG(json_extract(metadata_json, '$.reviewEffortMinutes')), falling back to the flat constant when the ledger is empty or predates this feature (an all-NULL average). Regression test proves the real estimate is used when present, and a real-D1 test proves the SQL round-trip end to end.

Scope note (read before merge)

src/review/stats.ts (the maintainer dashboard feed) is deliberately not wired to a per-PR effort aggregate. I traced its full read path: computeStats only reads review_targets / review_audit, the legacy reviewbot ledger — nothing writes new rows to review_targets since the self-host convergence cutover, and every review_audit write (pr_outcome, gate_decision) carries only decision/outcome metadata, never a PR's changed files or patches. There is no live source that module could aggregate a real per-PR estimate from today (unlike public-stats.ts, which reads the still-active audit_events ledger). Adding a same-shaped field there would only ever read back a permanently-null placeholder, so I left it out rather than fake it, and documented this explicitly in the file header (stats.ts lines 18-31). A real maintainer-dashboard effort aggregate needs its own persisted source and is a genuine follow-up, not a one-line addition to this file.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint — not run locally (no .github/workflows/** changes in this PR); CI runs it.
  • npm run typecheck
  • npm run test:coverage locally, scoped to the changed files (not the full unsharded suite, per this repo's own guidance to avoid redundant local runs): ran the 7 affected test files (focus-manifest.test.ts, unified-comment.test.ts, unified-comment-bridge.test.ts, public-stats.test.ts, signals-coverage.test.ts, review-effort.test.ts, queue.test.ts — 1206 tests, all passing) with coverage, then cross-referenced every new-diff line/branch/function from git diff origin/main --unified=0 against coverage/coverage-final.json's statementMap/branchMap/s/b arrays with a small Node script. Result: 100% statement, branch, and function coverage on every changed line in all 6 touched src/** files, with zero uncovered lines/branches remaining.
  • npm run test:workers — not run locally (no Cloudflare Workers-pool-specific code touched); CI runs it.
  • npm run build:mcp — not run locally (no MCP package changes); CI runs it.
  • npm run test:mcp-pack — not run locally (no MCP package changes); CI runs it.
  • npm run ui:openapi:check — not run locally; no API routes, OpenAPI schema, or StatsPayload/PublicStatsPayload shape change that's OpenAPI-surfaced (both payload types were extended in a way that doesn't touch any documented route contract — no new field was added to either exported payload shape, only internal aggregation logic changed). CI runs it as a backstop.
  • npm run ui:lint / ui:typecheck / ui:build — not run locally (no apps/gittensory-ui/** changes); CI runs them.
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests — N/A, no auth/session/CORS surface touched.
  • API/OpenAPI/MCP behavior is updated and tested where needed — N/A, no API/OpenAPI/MCP surface touched.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks — N/A, no apps/gittensory-ui/** changes.
  • Visible UI changes include a UI Evidence section — N/A, no visible UI change (this PR only changes a GitHub PR comment's markdown text and two backend aggregation modules).
  • Public docs/changelogs are updated where needed — N/A for CHANGELOG.md (never edited in a normal PR); the only doc surface touched is .gittensory.yml.example, which is updated in this PR.

UI Evidence

N/A — no visible UI change. The only rendered-text change is the new optional chip line in the GitHub PR review comment (`review effort: N/5 (~M min)`), which is exercised by unit tests, not a UI screenshot.

Notes

  • Exact test counts: 1206 tests across the 7 affected files, all passing (444 focus-manifest, 59 unified-comment, 63 unified-comment-bridge, 17 public-stats, 38 signals-coverage, 6 review-effort, 579 queue.test.ts — 6 of which are new: 2 wiring tests for the effort chip on/off, 1 estimator-throws fail-safe test, plus 3 new public-stats.test.ts tests for the real-average/fallback/real-D1 cases).
  • Rebased onto origin/main immediately before pushing (main moved by 2 commits mid-session); no conflicts.

@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. label Jul 6, 2026
@loopover-orb

loopover-orb Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-06 02:00:28 UTC

14 files · 1 AI reviewer · no blockers · readiness 82/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR correctly wires the already-shipped `estimateReviewEffort` (#2924/#2068) into three previously-unwired surfaces: a new `review.effort_score` manifest toggle (following the exact `changedFilesSummary` pattern end-to-end through parse/serialize/resolve/empty-manifest), a unified-comment status chip, and a real per-PR minutes figure persisted onto the existing `pr_public_surface_published` audit event that `public-stats.ts` now averages via `json_extract`/AVG with a documented NULL-safe fallback to the flat `MINUTES_SAVED_PER_PR` constant. The estimator computation for public-stats is wrapped in a `.catch(() => undefined)` so a thrown estimator never blocks the publish, and this fail-safe path plus the real-D1 round-trip are both directly tested. The `src/review/stats.ts` change is a documentation-only scope note explaining why the legacy ledger is deliberately NOT wired (no live source to aggregate from) rather than faking scaffolding — a good restraint call that matches this repo's anti-fabrication bar.

Nits — 6 non-blocking
  • src/review/public-stats.ts's new `avgReviewEffortMinutes` averages over every `pr_public_surface_published` row in the allowlist, not scoped to exactly the same `reviewed` population (merged+closed+commented) used in the surrounding formula — worth a comment or verification that the two counts stay in sync as edge cases (e.g. re-published events) accumulate.
  • The `1 | 2 | 3 | 4 | 5` band union is repeated verbatim across unified-comment.ts, unified-comment-bridge.ts, and (implicitly) review-effort.ts — consider a single exported `ReviewEffortBand` type alias to avoid drift if the band range ever changes.
  • The 'review effort: N/5 (~M min)' chip format string is duplicated in unified-comment.ts and mirrored in the bridge doc-comment/example yml — low risk but a shared formatter would keep it from silently diverging from the changed-files-summary chip style.
  • Confirm there's exactly one `recordAuditEvent` call site for `github_app.pr_public_surface_published` in `maybePublishPrPublicSurface` (processors.ts) — if a second early-return path also records this event type without the `reviewEffortMinutesForStats` field, public-stats' average would silently under-count on that path.
  • Consider naming the `MINUTES_SAVED_PER_PR` fallback comment more prominently near the SQL query itself (public-stats.ts) so future readers don't need to cross-reference the file header to understand the NULL-average degrade path.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #1955
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 55 registered-repo PR(s), 46 merged, 457 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 55 PR(s), 457 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 55 PR(s), 457 issue(s).
  • Related work: Titles/paths share 5 meaningful terms. (issue #2189, issue #1681)
  • Related work: Titles/paths share 5 meaningful terms. (issue #1683, issue #1681)
  • Related work: Titles/paths share 5 meaningful terms. (issue #1680, issue #1681)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • No action.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

…PR minutes

Adds a review.effort_score manifest toggle (mirroring review.changed_files_summary
end-to-end: focus-manifest parse/serialize/resolve, the .gittensory.yml.example
reference block, and the exhaustiveness test map) that renders a compact
"review effort: N/5 (~M min)" chip in the unified review comment, using the
already-shipped deterministic estimateReviewEffort estimator. Default off and
byte-identical when absent.

Also persists the same per-PR minutes onto the pr_public_surface_published audit
event and averages them in the public stats feed, replacing the flat
MINUTES_SAVED_PER_PR constant with a real per-PR figure while keeping the
constant as a documented fallback for ledger rows that predate this feature.
stats.ts (the maintainer dashboard) is left unchanged: it only reads the frozen
legacy review_targets/review_audit ledger, which has no live per-PR file source
to aggregate from.
@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.29%. Comparing base (2156f42) to head (df96cf0).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3666   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         314      314           
  Lines       32057    32076   +19     
  Branches    11748    11757    +9     
=======================================
+ Hits        29906    29925   +19     
  Misses       1517     1517           
  Partials      634      634           
Files with missing lines Coverage Δ
src/queue/processors.ts 94.04% <100.00%> (+0.02%) ⬆️
src/review/public-stats.ts 96.49% <100.00%> (+0.06%) ⬆️
src/review/stats.ts 98.57% <ø> (ø)
src/review/unified-comment-bridge.ts 99.33% <100.00%> (+<0.01%) ⬆️
src/review/unified-comment.ts 99.44% <100.00%> (+<0.01%) ⬆️
src/signals/focus-manifest.ts 99.04% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 6, 2026
…lock

config/examples/gittensory.full.yml must stay byte-identical to
.gittensory.yml.example from the WHERE IT LIVES marker onward
(test/unit/config-templates.test.ts), which this branch's rebase onto
main missed since the new review.effort_score documentation only
landed in the root file's auto-merged copy.
@JSONbored
JSONbored merged commit a9b344e into main Jul 6, 2026
10 checks passed
@JSONbored
JSONbored deleted the claude/orb-phase1-1955 branch July 6, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

Deterministic review-effort / complexity score + review-time ROI signal

1 participant