Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 4 additions & 1 deletion docs/web-app.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ The most-specific matching config wins when a review is submitted.
1. Open the New Review page.
2. Enter a PR URL or `owner/repo#123`.
3. Enter a trigger comment, for example `@askserge please review`.
4. Pick the provider and model.
4. Pick the provider and model. When the provider is Hugging Face, the model
field becomes a dropdown of tool-capable models served by the
[HF Inference Providers](https://router.huggingface.co) router; other
providers keep a free-text model field.
5. Start the review and watch the stream.
6. Edit the summary and comments.
7. Publish or discard the draft.
Expand Down
1 change: 1 addition & 0 deletions reviewbot/static/admin.html
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ <h2 id="form-title">Add a provider config</h2>
<label for="default_model">Default model</label>
<div class="hint">Used when the submit form leaves the model field empty.</div>
<input type="text" id="default_model" name="default_model" placeholder="model name">
<select id="default-model-select" style="display: none;"></select>
</div>
</div>

Expand Down
63 changes: 63 additions & 0 deletions reviewbot/static/admin.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
const apiKeyEl = document.getElementById("api_key");
const apiKeyHint = document.getElementById("api-key-hint");
const defaultModelEl = document.getElementById("default_model");
const defaultModelSelectEl = document.getElementById("default-model-select");
const repoPatternEl = document.getElementById("repo_pattern");
const allowedUsersEl = document.getElementById("allowed_users");
const allowedOrgsEl = document.getElementById("allowed_orgs");
Expand Down Expand Up @@ -51,6 +52,61 @@
if (def) defaultModelEl.value = def;
}

// HF Router model catalogue, lazily fetched once and cached. null until
// loaded; [] when unreachable (we then keep the free-text input).
let hfModels = null;

async function ensureHfModels() {
if (hfModels !== null) return hfModels;
try {
const r = await fetch("/llm-options/hf-models");
const data = r.ok ? await r.json() : {};
hfModels = Array.isArray(data.models) ? data.models : [];
} catch {
hfModels = [];
}
return hfModels;
}

function populateModelSelect() {
// The text input stays the source of truth; the dropdown mirrors it.
// A leading blank option keeps "no default" expressible (the submit
// form's model then wins). Preserve any current value even if the
// router doesn't list it.
const current = defaultModelEl.value.trim();
const models = [...(hfModels || [])];
if (current && !models.includes(current)) models.unshift(current);
defaultModelSelectEl.replaceChildren();
const blank = document.createElement("option");
blank.value = "";
blank.textContent = "— none (use submit-form model) —";
defaultModelSelectEl.appendChild(blank);
for (const m of models) {
const opt = document.createElement("option");
opt.value = m;
opt.textContent = m;
defaultModelSelectEl.appendChild(opt);
}
defaultModelSelectEl.value = current;
}

// Show a dropdown of HF Router models when the HF provider is selected;
// other providers keep the free-text input. Falls back to the text input
// when the model list can't be fetched.
async function updateModelControl() {
if (providerEl.value === "hf") {
const models = await ensureHfModels();
if (providerEl.value === "hf" && models.length) {
populateModelSelect();
defaultModelSelectEl.style.display = "";
defaultModelEl.style.display = "none";
return;
}
}
defaultModelSelectEl.style.display = "none";
defaultModelEl.style.display = "";
}

function resetForm() {
editing = null;
configIdEl.value = "";
Expand All @@ -69,6 +125,7 @@
submitBtn.textContent = "Save";
cancelBtn.style.display = "none";
updateProviderRow();
updateModelControl();
}

function startEdit(cfg) {
Expand All @@ -88,6 +145,7 @@
submitBtn.textContent = "Save changes";
cancelBtn.style.display = "";
updateProviderRow();
updateModelControl();
window.scrollTo({ top: form.offsetTop - 16, behavior: "smooth" });
}

Expand Down Expand Up @@ -264,6 +322,11 @@
if (!editing) {
defaultModelEl.value = defaultModels[providerEl.value] || "";
}
updateModelControl();
});

defaultModelSelectEl.addEventListener("change", () => {
defaultModelEl.value = defaultModelSelectEl.value;
});

resetForm();
Expand Down
1 change: 1 addition & 0 deletions reviewbot/static/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ <h2>Ask Serge to review a PR</h2>
<div>
<label for="llm-model">Model</label>
<input type="text" id="llm-model" name="llm_model" required placeholder="model name">
<select id="llm-model-select" style="display: none;"></select>
</div>
</div>
<div id="custom-base-row" style="display: none;">
Expand Down
69 changes: 69 additions & 0 deletions reviewbot/static/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
const commentEl = document.getElementById("comment");
const providerEl = document.getElementById("llm-provider");
const modelEl = document.getElementById("llm-model");
const modelSelectEl = document.getElementById("llm-model-select");
const baseUrlEl = document.getElementById("llm-base-url");
const customBaseRow = document.getElementById("custom-base-row");
const providerHint = document.getElementById("provider-hint");
Expand Down Expand Up @@ -125,6 +126,66 @@
modelEl.value = providerDefaultModels[providerEl.value] || "";
}

// HF Router model catalogue, lazily fetched once and cached. null until
// loaded; [] when the endpoint is unreachable (we then fall back to the
// free-text input rather than an empty dropdown).
let hfModels = null;

async function ensureHfModels() {
if (hfModels !== null) return hfModels;
try {
const r = await fetch("/llm-options/hf-models");
const data = r.ok ? await r.json() : {};
hfModels = Array.isArray(data.models) ? data.models : [];
} catch {
hfModels = [];
}
return hfModels;
}

function populateModelSelect() {
// The text input stays the source of truth for the submitted value;
// the dropdown mirrors it. Keep the current value selectable even if
// the router doesn't list it (e.g. a config default not yet live), so
// switching to HF never silently drops a configured model.
const current = modelEl.value.trim();
const options = [...(hfModels || [])];
if (current && !options.includes(current)) options.unshift(current);
modelSelectEl.replaceChildren();
for (const m of options) {
const opt = document.createElement("option");
opt.value = m;
opt.textContent = m;
modelSelectEl.appendChild(opt);
}
if (current && options.includes(current)) {
modelSelectEl.value = current;
} else if (options.length) {
modelSelectEl.value = options[0];
modelEl.value = options[0];
}
}

// Show a dropdown of HF Router models when the HF provider is selected;
// every other provider keeps the free-text input. Falls back to the text
// input when the model list can't be fetched.
async function updateModelControl() {
if (providerEl.value === "hf") {
const models = await ensureHfModels();
// Guard against the provider changing while the fetch was in flight.
if (providerEl.value === "hf" && models.length) {
populateModelSelect();
modelSelectEl.style.display = "";
modelEl.style.display = "none";
modelEl.required = false;
return;
}
}
modelSelectEl.style.display = "none";
modelEl.style.display = "";
modelEl.required = true;
}

function ingestProviderDefaults(providers) {
if (!Array.isArray(providers)) return;
for (const p of providers) {
Expand All @@ -151,10 +212,12 @@
applySavedLlmPrefs();
applyProviderModelDefault();
updateProviderFields();
await updateModelControl();
} catch {
applySavedLlmPrefs();
applyProviderModelDefault();
updateProviderFields();
await updateModelControl();
}
}

Expand Down Expand Up @@ -276,6 +339,7 @@
baseUrlEl.value = match.api_base;
}
updateProviderFields();
updateModelControl();
const modelPart = match.default_model ? ` · model ${match.default_model}` : "";
const scope =
match.repo_pattern && match.repo_pattern !== `${parsed.owner}/${parsed.repo}`
Expand Down Expand Up @@ -323,9 +387,14 @@
providerEl.addEventListener("change", () => {
updateProviderFields();
resetModelToProviderDefault();
updateModelControl();
saveLlmPrefs();
});
modelEl.addEventListener("change", saveLlmPrefs);
modelSelectEl.addEventListener("change", () => {
modelEl.value = modelSelectEl.value;
saveLlmPrefs();
});
baseUrlEl.addEventListener("change", saveLlmPrefs);

// Auto-fill provider/model from the matching DB config whenever the
Expand Down
72 changes: 72 additions & 0 deletions reviewbot/webapp.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,16 @@
_LLM_PROVIDER_ANTHROPIC: "claude-opus-4-6",
}

# HF Inference Providers catalogue. The public router /v1/models endpoint
# lists every model the HF Router can serve along with per-provider tool
# support; we surface the tool-capable subset as a dropdown on the submit /
# admin forms so users don't have to hand-type model ids. Cached in-process
# (the list is large and changes slowly) and refreshed lazily on expiry.
_HF_MODELS_URL = f"{_LLM_PROVIDER_BASES[_LLM_PROVIDER_HF]}/models"
_HF_MODELS_TTL_SECONDS = 15 * 60
_hf_models_lock = threading.Lock()
_hf_models_cache: dict[str, Any] = {"fetched_at": 0.0, "models": []}


# ---------------------------------------------------------------------------
# Session handling: signed cookies via itsdangerous (no DB).
Expand Down Expand Up @@ -1343,6 +1353,68 @@ def _provider_entry(pid: str, label: str, base_url: str) -> dict:
)


def _tool_capable_hf_models(payload: Any) -> list[str]:
"""Extract the ids of HF Router models that can drive an agentic
review — at least one ``live`` provider advertising ``supports_tools``.
Serge's review loop relies on browse tools, so models that can't call
tools would degrade silently; we keep them out of the dropdown. Sorted
case-insensitively and de-duplicated."""
data = payload.get("data") if isinstance(payload, dict) else None
if not isinstance(data, list):
return []
ids: set[str] = set()
for entry in data:
if not isinstance(entry, dict):
continue
model_id = entry.get("id")
if not isinstance(model_id, str) or not model_id:
continue
providers = entry.get("providers")
if not isinstance(providers, list):
continue
if any(
isinstance(p, dict)
and p.get("status") == "live"
and p.get("supports_tools") is True
for p in providers
):
ids.add(model_id)
return sorted(ids, key=str.lower)


async def _fetch_hf_router_models() -> list[str]:
"""Tool-capable HF Router model ids, cached for ``_HF_MODELS_TTL_SECONDS``.
On a fetch error the last good list (possibly empty) is returned so a
transient router blip doesn't blank out the dropdown — the form falls
back to a free-text box when the list is empty."""
now = time.monotonic()
with _hf_models_lock:
cached = _hf_models_cache["models"]
if cached and now - _hf_models_cache["fetched_at"] < _HF_MODELS_TTL_SECONDS:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache check if cached and ... treats an empty list as a cache miss. This means a successful fetch that returns [] (or an empty cache seeded in tests) will never be served from cache and will trigger a fresh HTTP request every time.

Use the timestamp to determine whether the cache has been populated instead:

Suggested change
if cached and now - _hf_models_cache["fetched_at"] < _HF_MODELS_TTL_SECONDS:
with _hf_models_lock:
cached = _hf_models_cache["models"]
if _hf_models_cache["fetched_at"] > 0 and now - _hf_models_cache["fetched_at"] < _HF_MODELS_TTL_SECONDS:
return cached

return cached
try:
async with httpx.AsyncClient(timeout=10) as client:
resp = await client.get(_HF_MODELS_URL)
resp.raise_for_status()
models = _tool_capable_hf_models(resp.json())
except (httpx.HTTPError, ValueError) as exc:
log.warning("Failed to fetch HF router models from %s: %s", _HF_MODELS_URL, exc)
with _hf_models_lock:
return _hf_models_cache["models"]
with _hf_models_lock:
_hf_models_cache["models"] = models
_hf_models_cache["fetched_at"] = now
return models


@app.get("/llm-options/hf-models")
async def hf_models(request: Request) -> JSONResponse:
"""Tool-capable models served by the HF Router, for the provider
dropdown that appears when ``hf`` is the selected provider."""
_require_user(request)
return JSONResponse({"models": await _fetch_hf_router_models()})


@app.get("/reviews/lookup-provider")
def lookup_provider(request: Request, owner: str, repo: str) -> JSONResponse:
"""Pre-flight match for the submit form: given a (owner, repo)
Expand Down
Loading
Loading