Skip to content

feat(config): expose dual-AI combine strategy as a per-repo setting - #2677

Merged
JSONbored merged 2 commits into
mainfrom
feat/per-repo-dual-ai-combine-config
Jul 3, 2026
Merged

feat(config): expose dual-AI combine strategy as a per-repo setting#2677
JSONbored merged 2 commits into
mainfrom
feat/per-repo-dual-ai-combine-config

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • ai-review.ts's dual-AI CombineStrategy/onMerge/reviewers config (src/services/ai-review.ts:90-92, 1099-1101) was only settable via the self-host operator's AI_REVIEW_PLAN boot config (env.AI_REVIEW_PLAN, src/env.d.ts:77) — there was no review.combine/review.onMerge/review.reviewers field in the manifest schema, so a repo could not refine the operator's default.
  • Adds gate.aiReview.combine / gate.aiReview.onMerge / gate.aiReview.reviewers to FocusManifestGateConfig in src/signals/focus-manifest.ts, parsed and round-tripped the same way as the existing sibling fields in that block (aiReviewMode, aiReviewByok, aiReviewCloseConfidence, ...).
  • Follows the exact precedent set by gate.aiReview.closeConfidence (src/types.ts, src/signals/focus-manifest.ts:1266 region): config-as-code only — no DB column, no Drizzle field, no migration, no dashboard settings-write route in src/api/routes.ts. Verified aiReviewCloseConfidence has none of those either before following suit.
  • runAiReviewForAdvisory in src/queue/processors.ts now forwards settings.aiReviewCombine/aiReviewOnMerge/aiReviewReviewers (already resolved by resolveEffectiveSettings) into runGittensoryAiReview's input, in place of relying solely on the unconditional env.AI_REVIEW_PLAN read.

Precedence design decision (the core of this issue)

onMerge carries real strictness semantics, unlike combine:

  • either — ANY single reviewer's blocker holds/blocks the PR (the stricter rule).
  • both — requires every reviewer to agree before a blocker counts (the more permissive rule).

So a naive per-repo override could let a repo silently loosen an operator-imposed floor (e.g. operator sets either instance-wide to guarantee any flagged blocker is caught; a repo sets both and now needs both reviewers to agree, which is strictly weaker). I resolved this by adding resolveEffectiveAiReviewOnMerge in src/services/ai-review.ts, called from runGittensoryAiReview where both the per-repo input.onMerge and the operator's plan?.onMerge are visible:

  • Operator floor either + repo override bothclamped back to either (an attempted loosening).
  • Operator floor either + repo override eithereither (a no-op tightening — never treated as a clamp).
  • Operator floor both (or unset) → the repo override wins unclamped — there is no stricter floor to violate.

A fired clamp increments gittensory_ai_review_onmerge_clamped_total{mode} (mirrors the existing gittensory_ai_review_inconclusive_total observability pattern in the same file) so the attempted loosening is surfaced, not silently dropped.

combine itself (single/consensus/synthesis) is not floor-clamped — the three strategies aren't ordered by strictness, so there's no single "loosening" direction to clamp against. reviewers (which providers run) also isn't floor-clamped — the floor is about what triggers a hold/block, not who evaluates it.

Unrelated fix required to keep this change UI-safe

CombineStrategy/OnMerge were originally defined in src/services/ai-review.ts, which pulls in ambient Cloudflare Workers types (Env, D1Database, ...) that the apps/gittensory-ui workspace's tsconfig.json lib doesn't declare. src/signals/focus-manifest.ts and src/types.ts are both imported by the UI (maintainer-settings.tsx, registration-workspace.ts), so a type-only import("../services/ai-review").CombineStrategy reference from either file dragged that whole module graph into the UI's typecheck and broke npm run ui:typecheck (Cannot find name 'Env' / 'D1Database' across src/db/*.ts, src/review/*.ts, etc.). Moved the canonical CombineStrategy/OnMerge definitions into src/types.ts (a zero-import leaf file, alongside the existing GateRuleMode/GatePolicyPack) and re-exported them from src/services/ai-review.ts for backward compatibility with existing importers (src/env.d.ts, test files).

Test plan

  • npm run typecheck — clean.
  • npm run ui:typecheck — clean (confirmed this specifically catches the Env/D1Database leak described above; reproduced it, fixed it, reproduced the fix).
  • npx vitest run test/unit/focus-manifest.test.ts test/unit/ai-review.test.ts test/unit/ai-review-advisory.test.ts — 299 passed. New coverage:
    • a repo without an override inherits the operator's onMerge floor unchanged (zero behavior change).
    • a repo tightening either → either against an either floor is a no-op, not a clamp.
    • a repo attempting to loosen either → both against an either floor is clamped back to either, end-to-end through runGittensoryAiReview, and asserted via the gittensory_ai_review_onmerge_clamped_total metric (logged/surfaced, not silently ignored).
    • a repo picking both against a both (or unset) floor is honored unclamped.
    • pure precedence-logic unit tests for resolveEffectiveAiReviewOnMerge covering every combination of {null, undefined, either, both} × {null, undefined, either, both}.
    • gate.aiReview.combine/onMerge/reviewers parsing: valid values, invalid values (warn + drop), round-trip through gateConfigToJsonparseFocusManifest, resolution into effective settings, and the manifest-present flag.
    • gate.aiReview.reviewers list handling: valid entries, non-mapping/blank-model entries dropped individually, the 4-entry cap, and the "absent means null not []" contract.
    • runAiReviewForAdvisory wiring test: settings.aiReviewCombine/aiReviewReviewers reach the actual reviewer call (only the configured reviewer runs).
    • a repo without any override sees the byte-identical default Workers-AI pair + consensus strategy.
  • npm run test:coverage (unsharded) — 6748 tests passed, 0 failed; every line I added in focus-manifest.ts and ai-review.ts is covered (verified the reported "Uncovered Line #s" for both files fall entirely in pre-existing code untouched by this diff, via git diff --unified=0).
  • npm run test:ci — full local gate green (actionlint, migrations check, cf-typegen check, typecheck, coverage suite, workers test, mcp build/pack, UI openapi/lint/typecheck/test/build).
  • npm audit --audit-level=moderate — 0 vulnerabilities.
  • npm run ui:openapi:settings-parityRepositorySettingsSchema matches RepositorySettings (65 fields); regenerated and committed apps/gittensory-ui/public/openapi.json via npm run ui:openapi.

Closes #2567

ai-review.ts's dual-AI CombineStrategy/onMerge/reviewers config was only
settable via the self-host operator's AI_REVIEW_PLAN boot config, with no
per-repo override in .gittensory.yml. Adds gate.aiReview.combine/onMerge/
reviewers to the focus manifest so a repo can refine the operator's plan,
following the same config-as-code-only pattern already used for
gate.aiReview.closeConfidence (no DB column, no dashboard write route).

The onMerge field carries real strictness semantics (either = any one
reviewer's blocker holds/blocks; both = requires every reviewer to agree),
so a naive per-repo override could let a repo silently loosen an
operator-imposed floor. resolveEffectiveAiReviewOnMerge in ai-review.ts
clamps a repo's "both" back to the operator's "either" floor, incrementing
a metric so the clamp is surfaced rather than silently ignored; a repo may
still freely tighten, and when the operator set no floor any value passes
through unclamped.

CombineStrategy/OnMerge move from services/ai-review.ts into types.ts as
their canonical definition (re-exported from ai-review.ts for compat):
focus-manifest.ts and types.ts are imported by the UI workspace, and a
type-only reference into ai-review.ts pulled its ambient Cloudflare
Workers types (Env, D1Database, ...) into the UI's typecheck graph and
broke it.

Closes #2567
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 3, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui 8e2fc7e Commit Preview URL

Branch Preview URL
Jul 03 2026, 05:50 AM

@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.13%. Comparing base (4c72212) to head (8e2fc7e).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2677   +/-   ##
=======================================
  Coverage   96.13%   96.13%           
=======================================
  Files         239      239           
  Lines       26739    26779   +40     
  Branches     9699     9721   +22     
=======================================
+ Hits        25705    25745   +40     
  Misses        424      424           
  Partials      610      610           
Files with missing lines Coverage Δ
src/openapi/schemas.ts 100.00% <ø> (ø)
src/queue/processors.ts 92.74% <100.00%> (+0.01%) ⬆️
src/services/ai-review.ts 95.36% <100.00%> (+0.09%) ⬆️
src/signals/focus-manifest.ts 99.32% <100.00%> (+0.02%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 3, 2026
@loopover-orb

loopover-orb Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 05:40:47 UTC

9 files · 1 AI reviewer · no blockers · readiness 82/100 · CI green · dirty

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change wires new config-as-code AI review settings through manifest parsing, effective settings, advisory execution, OpenAPI schema, and the AI combiner, with tests covering parse/round-trip/precedence and the onMerge clamp. The core precedence rule is implemented at the correct layer because runGittensoryAiReview can see both the repo override and operator plan. I do not see a reachable correctness break in the provided diff.

Nits — 7 non-blocking
  • nit: src/signals/focus-manifest.ts:503 sets MAX_AI_REVIEW_REVIEWERS to 4 even though the nearby comment says the combiner only consumes reviewer[0]/[1], so the parser accepts two inert entries that can confuse repo authors.
  • nit: src/signals/focus-manifest.ts:532 silently drops a non-string or blank fallback instead of warning, unlike invalid model entries; that makes typos in fallback harder to diagnose.
  • nit: src/types.ts:626 describes the loosening example as `both → an attempted loosening` without naming the operator floor, which makes the strictness explanation harder to verify from the type docs alone.
  • src/signals/focus-manifest.ts:503 should either cap reviewers at 2 or explain precisely which future path consumes entries 3 and 4.
  • src/signals/focus-manifest.ts:532 should warn when `fallback` is present but invalid, matching the parser's treatment of invalid reviewer entries.
  • 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.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #2567
Related work ⚠️ 1 scoped overlap Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:L; 1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 61 registered-repo PR(s), 52 merged, 500 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 61 PR(s), 500 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
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: not available
  • Official Gittensor activity: 61 PR(s), 500 issue(s).
  • Related work: Titles/paths share 6 meaningful terms. (issue #2212, issue #2567)
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

# Conflicts:
#	apps/gittensory-ui/public/openapi.json
#	src/queue/processors.ts
#	test/unit/focus-manifest.test.ts
@JSONbored
JSONbored merged commit dc9cbde into main Jul 3, 2026
10 checks passed
@JSONbored
JSONbored deleted the feat/per-repo-dual-ai-combine-config branch July 3, 2026 05:57
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.

Development

Successfully merging this pull request may close these issues.

feat(config): expose the dual-AI combine strategy as a per-repo .gittensory.yml setting

1 participant