Skip to content

fix(selfhost): preserve legacy dual-review behavior for multi-provider AI_PROVIDER - #3625

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-dual-ai-reviewer-configuration-downgrade
Jul 6, 2026
Merged

fix(selfhost): preserve legacy dual-review behavior for multi-provider AI_PROVIDER#3625
JSONbored merged 1 commit into
mainfrom
codex/fix-dual-ai-reviewer-configuration-downgrade

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • A recent change collapsed comma-separated AI_PROVIDER lists into a single reviewer plus fallback unless AI_DUAL_REVIEW was explicitly set, silently downgrading deployments that relied on an explicit multi-provider dual-review plan.
  • Operator-configured AI_COMBINE / AI_ON_MERGE semantics were intended to preserve two independent reviewers for legacy multi-provider configs and must not be ignored.

Description

  • Restore legacy dual-review behavior when the operator explicitly sets AI_COMBINE or AI_ON_MERGE by treating those env values as an indicator that a multi-provider AI_PROVIDER list intends two independent reviewers, while leaving plain comma-separated lists without those flags as single-reviewer fallback chains (change in resolveAiReviewerPlan in src/selfhost/ai.ts).
  • Add a regression test that asserts legacy multi-provider AI_COMBINE/AI_ON_MERGE configs resolve to two independent reviewers (update in test/unit/selfhost-ai.test.ts).
  • Regenerate the self-host env reference to keep generated line references in sync after the resolver changes (update in apps/gittensory-ui/src/lib/selfhost-env-reference.ts).

Testing

  • Ran npm run selfhost:env-reference:check and it succeeded updating/validating the generated env reference file.
  • Ran targeted unit tests npx vitest run test/unit/selfhost-ai.test.ts -t "resolveProviderNames|resolveAiReviewerPlan" and npx vitest run test/unit/ai-review.test.ts -t "self-host dual-AI plan", and the focused tests passed.
  • Attempted the full local gate via npm run test:ci; the run progressed to coverage but could not be completed in this environment (long-running suites / coverage aggregation caused the invocation to be interrupted), and npm audit --audit-level=moderate returned a registry error (403) when attempted here.

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 5, 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 d1879a7 Commit Preview URL

Branch Preview URL
Jul 06 2026, 12:48 AM

@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:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 5, 2026
@loopover-orb

loopover-orb Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-06 00:52:16 UTC

3 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This is a narrow, well-targeted revert-and-fix: resolveAiReviewerPlan in src/selfhost/ai.ts now treats an explicit AI_COMBINE or AI_ON_MERGE as a legacy signal that a multi-provider AI_PROVIDER list wants two independent reviewers, restoring dual-review behavior that a recent change silently regressed, while leaving plain comma-separated lists without those flags on the single-reviewer fallback path. The single-provider early return was hoisted above the AI_DUAL_REVIEW check but is otherwise unchanged in logic (fallback resolves to undefined either way), so it's behavior-preserving. The regression test explicitly covers both the legacy-config-restores-dual-review case and confirms the fallback-chain default is unchanged when AI_COMBINE/AI_ON_MERGE are absent, and the generated env-reference line numbers are consistently updated to match the moved code.

Nits — 4 non-blocking
  • The PR description doesn't reference a specific tracked issue number for this regression fix — worth confirming there's an open issue this closes per the contribution policy.
  • src/selfhost/ai.ts's resolveAiReviewerPlan is approaching the flagged 400-line file threshold; consider whether the dual-review resolution logic could be split out, though this diff doesn't need to be the one to do it.
  • Consider adding a test case for AI_COMBINE/AI_ON_MERGE set to an empty string to explicitly document that empty-string values do NOT trigger legacy dual-review (the .trim() !== "" check implies this but it's not directly asserted in the visible diff).
  • If not already covered elsewhere, a test with exactly 2 providers plus only AI_COMBINE (no AI_ON_MERGE) would round out coverage of the hasLegacyDualReviewConfig OR-condition's two branches.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 55 registered-repo PR(s), 46 merged, 464 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 55 PR(s), 464 issue(s).
Gate result ❌ Blocking Repo-configured hard 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: not available
  • Official Gittensor activity: 55 PR(s), 464 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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

@JSONbored
JSONbored force-pushed the codex/fix-dual-ai-reviewer-configuration-downgrade branch from 4da23eb to d1879a7 Compare July 6, 2026 00:46
@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 (d1879a7).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3625   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         314      314           
  Lines       32057    32058    +1     
  Branches    11748    11749    +1     
=======================================
+ Hits        29906    29907    +1     
  Misses       1517     1517           
  Partials      634      634           
Files with missing lines Coverage Δ
src/selfhost/ai.ts 98.59% <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
@JSONbored
JSONbored merged commit 26763ff into main Jul 6, 2026
13 checks passed
@JSONbored
JSONbored deleted the codex/fix-dual-ai-reviewer-configuration-downgrade branch July 6, 2026 00:55
loopover-orb Bot pushed a commit that referenced this pull request Jul 6, 2026
…ence doc (#3676)

firstReference embedded exact file:line, so any unrelated line shift in a
scanned source file made the generated doc go stale -- two concurrent PRs
touching the same file produced conflicting regenerations and collided on
rebase (3+ past reactive regeneration commits, most recently PR #3625).
Drop the line number; the file path alone is stable across unrelated edits
and only changes when a read is actually added, removed, or relocated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

1 participant