Repository navigation
Conversation
aivong-openhands
left a comment
There was a problem hiding this comment.
Reviewed against .agents/skills/code-review.md.
The Replicated side looks solid: the existing azure-<deployment> route names and the default model are preserved for upgrades (azure_provider_mode defaults to azure). Foundry gets its own credentials and its own drop_params knob, and the azure_ai- / azure-ai- name collision is covered by a test. Both new secrets are in checksum/litellm-credentials and secretsChecksum, and drop_params changes still roll LiteLLM through the subchart's checksum/config. CI is green. Locally, test_replicated_llm_provider_routes.py passes (76 tests), and so does check_litellm_checksum.py.
I found one important issue in the chart change. It is inline: OPENHANDS_PROVIDER_BASE_URL breaks silently when a consumer overrides LITE_LLM_API_URL through env. The fix is small, so I'm leaving a comment rather than approving for now.
This review was generated by an AI agent (OpenHands) on behalf of the user.
|
Thanks for the fix! I independently validated release 1749 (0.75.0) on my Azure Replicated install. Azure model discovery remained available across background refreshes, and both an Azure-backed conversation and automation completed terminal commands successfully using the Replicated configuration. |
|
@OpenHands /codereview-roasted Review exactly commit f278d93. If the PR head changes, report that and do not approve another commit. Review only: do not push code, merge, deploy, or change attribution. Check correctness, regression coverage, and whether the PR validation claims match the evidence. Keep code comments short. Post the substantive review and link it in your completion message. End the substantive review with these plain lines (no code fence), choosing one verdict: REVIEWED_HEAD: f278d93 Use VERDICT: CHANGES_REQUESTED instead when findings require changes. If unable to review, explain the blocker without an approval verdict. |
|
I'm on it! ak684 can track my progress at all-hands.dev |
ak684
left a comment
There was a problem hiding this comment.
Review of f278d9370aa8087f7d4b6823c98e466a15950944
Reviewed the exact head commit. The PR head is unchanged and matches the requested SHA. No code was modified; this is a review-only pass.
Scope
9 files: two new KOTS config option groups (azure_provider_mode, azure_ai_*, azure_drop_params/azure_ai_drop_params), a rewritten Azure optionalValues block in replicated/openhands.yaml covering api_key / service_principal / Foundry / both, azure_ai secret wiring, OPENHANDS_PROVIDER_BASE_URL in the chart env, checksum updates, and new tests.
Correctness
- Provider coexistence. The single
llm_provider=azureblock builds both route sets with a shared$seendict, so azure andazure_airoutes cannot collide, and empty textareas render nothing. Verified forazure,azure_ai, andbothmodes. whengating.azure_provider_modecorrectly hides the OpenAI-only fields inazure_aimode and the Foundry fields inazuremode;azure_auth_methoddefaults toapi_keyand resolves empty inazure_aimode. A stale storedservice_principalvalue does not contaminateazure_airoutes (option hidden ⇒ empty).- Upgrade preservation. For an existing install (no
azure_provider_mode, legacyazure_deployment_nameseeded intoazure_deployments), the renderedLITELLM_DEFAULT_MODELstays byte-identical (litellm_proxy/azure-<first>) and route names unchanged (azure-<deployment>). Verified by direct render. - API-version stability fix. Rendering
api_versionfrom theConfigOptiondirectly (instead ofos.environ/AZURE_API_VERSION) keeps generated route IDs stable against LiteLLM's startup overwrite of the env — matches the PR's reproduction claim. - Secrets/checksums.
azure_ai_api_key/azure_ai_endpointare added to the LiteLLM credentials checksum (27 deps) and thesecretsChecksum. Bothcheck_litellm_checksum.pyandcheck_secret_checksum.pypass. The subchart's ownchecksum/configannotation rolls the LiteLLM pod whenmodel_list/drop_paramschange, independent of the credentials hash. OPENHANDS_PROVIDER_BASE_URL: $(LITE_LLM_API_URL).LITE_LLM_API_URLis always set (bundled FQDN or externallitellm.url), so the interpolation resolves either way and Kubernetes$(VAR)expansion is order-independent. The env-var dedup inopenhands.envlets consumers override it.- Foundry routes omit
api_version(defaulted client-side by LiteLLM) — consistent with the real-request validation documented in the PR body; not independently reproducible here.
Regression coverage
scripts/test_replicated_llm_provider_routes.py: 76 pass, incl. the new coexist and foundry-only tests.scripts/test_litellm_credentials_checksum.py: 31 pass.scripts/test_replicated_llm_key_refresh_warm_env.py: 2 pass (needshelm dependency build).helm unittestoncharts/openhands: 184 pass, incl. the 2 new managed-proxy env tests;helm lintpasses on both charts.
Note: the PR numbers "108" regression tests; the three suites total 76 + 31 + 2 = 109 here — a counting nit, not a coverage gap.
CI evidence
All 24 check runs on the head commit are green, including Test scripts, Helm Unit Tests, Check secret checksum, Validate Replicated Manifests, and pr.
Minor observations (non-blocking)
charts/openhands/templates/troubleshoot/support-bundle.yaml'slitellm-env-secretspresence-collector list was not extended withAZURE_AI_API_KEY/AZURE_AI_API_BASE.includeValue: falsemeans no value exposure and secret content is still redacted, so this only affects presence reporting in support bundles. Worth a one-line addition.- In
bothmode the default model always resolves to the Azure OpenAI route; switching to a Foundry default is a manual step. Reasonable given the "preserve existing default" goal, but worth noting in the field help.
The validation claims match the evidence: local renders, the test suites, the linter-backed checksum, and the external confirmation from @rajshah4 of release 1749 on a real Azure Replicated install corroborate the live-validation section of the PR body.
REVIEWED_HEAD: f278d93
REVIEW_REQUEST: 89440fa9ada64adf9dc98f5f1e995228
VERDICT: APPROVED
ak684
left a comment
There was a problem hiding this comment.
Follow-up verdict correction
After completing the full review I inspected the inline thread on charts/openhands/templates/_env.yaml (comment from aivong-openhands, marked Important) and confirmed it with a helm render:
helm template t charts/openhands --set enabled=true --set litellm-helm.enabled=false \
--set litellm.url=https://proxy.example/v1 --set env.LITE_LLM_API_URL=https://override.example
results in OPENHANDS_PROVIDER_BASE_URL ($(LITE_LLM_API_URL)) appearing before the overridden LITE_LLM_API_URL in the container env list. Kubernetes only expands $(VAR) references to vars defined earlier, so a consumer that overrides env.LITE_LLM_API_URL (a supported, documented override under this chart's env contract) gets the literal string $(LITE_LLM_API_URL) as the provider base URL — every OpenHands-managed conversation fails silently. The Replicated installer path is unaffected (it never overrides LITE_LLM_API_URL), which is why the live-validation and the new tests pass, but the public chart path regresses against the codebase's override-friendly env requirements.
The fix is small and order-independent — compute the URL once and render it into both vars, e.g.:
{{- $liteLlmUrl := .Values.litellm.url }}
{{- if and (index .Values "litellm-helm") (index .Values "litellm-helm" "enabled") }}
{{- $liteLlmUrl = printf "http://%s-litellm.%s.svc.cluster.local:4000" .Release.Name .Release.Namespace }}
{{- end }}
- name: LITE_LLM_API_URL
value: {{ $liteLlmUrl }}
- name: OPENHANDS_PROVIDER_BASE_URL
value: {{ $liteLlmUrl }}and update managed_llm_proxy_env_test.yaml to assert the concrete URL plus an override case (env.LITE_LLM_API_URL), so the regression is actually covered.
Everything else in my review stands (route coexistence, when gating, upgrade preservation, checksums, CI). On the strength of the unaddressed Important finding above, the verdict changes.
REVIEWED_HEAD: f278d93
REVIEW_REQUEST: 89440fa9ada64adf9dc98f5f1e995228
VERDICT: CHANGES_REQUESTED
|
Substantive review posted on the exact head commit Summary: the Azure AI Foundry / drop-params config, checksum wiring, upgrade preservation, and route coexistence all verify locally (76 route tests, 31 checksum tests, 2 warm-env tests, 184 helm-unittest, helm lint; all 24 CI checks green). One unaddressed Important finding changes the verdict: VERDICT: CHANGES_REQUESTED (Note: |
|
The review is complete. Here's a summary of what I did. Review posted to PR #1337Reviewed the exact head commit What I checked (and the evidence)Correctness — all verified by rendering the KOTS templates locally:
Regression coverage — all locally reproduced:
The finding that changes the verdictA pre-existing inline comment (flagged Important) on VERDICT: CHANGES_REQUESTED — with a concrete order-independent fix (compute the URL once, render into both vars) and a test that overrides Posted as two reviews on the exact commit and a summary reply to the comment thread addressed to me:
Note: the session |
|
@OpenHands /codereview-roasted Review exactly commit c6cf50e. If the PR head changes, report that and do not approve another commit. Review only: do not push code, merge, deploy, or change attribution. Check correctness, regression coverage, and whether the PR validation claims match the evidence. Keep code comments short. Post the substantive review and link it in your completion message. End the substantive review with these plain lines (no code fence), choosing one verdict: REVIEWED_HEAD: c6cf50e Use VERDICT: CHANGES_REQUESTED instead when findings require changes. If unable to review, explain the blocker without an approval verdict. |
|
I'm on it! ak684 can track my progress at all-hands.dev |
ak684
left a comment
There was a problem hiding this comment.
Reviewed exactly commit c6cf50e66bd9dc52ed7d06881684dd888b332eda; the PR head is unchanged and matches the requested SHA (verified via API). Review-only — no code pushed.
Scope of this head commit
Follows up the prior review's Important finding on charts/openhands/templates/_env.yaml: OPENHANDS_PROVIDER_BASE_URL was $(LITE_LLM_API_URL), a literal reference that Kubernetes only expands if the target var appears earlier in the env list. Under this chart's env dedup contract, a consumer override env.LITE_LLM_API_URL is appended after this entry, so the managed-provider base URL silently degraded to the literal string. The commit computes the proxy URL once ($liteLlmUrl), emits it for LITE_LLM_API_URL, and renders the same value (or the env.LITE_LLM_API_URL override) directly into OPENHANDS_PROVIDER_BASE_URL — order-independent by construction. managed_llm_proxy_env_test.yaml now asserts the concrete URLs and adds an override case (env.LITE_LLM_API_URL), which fails on the previous head and passes here.
Verification (independent)
scripts/test_replicated_llm_provider_routes.py: 76 passed, includingtest_azure_providers_coexist(4 param sets × 2 auth modes) andtest_foundry_only_does_not_require_openai_fields.scripts/check_litellm_checksum.py: OK — 27 LiteLLM credential deps covered (matches the bumpedEXPECTED_DEPENDENCY_COUNT = 27);scripts/check_secret_checksum.py: OK — 47 password fields.scripts/test_litellm_credentials_checksum.py+test_litellm_budget_cache.py: 32 passed.helm unittest charts/openhands(with subcharts off): 160 passed, including the 3 managed-proxy env tests;helm unittest charts/openhands-secrets: 12 passed.- CI on the head is all green (
helm-unittest,test-scripts,lint-and-test openhands/openhands-secrets,check-secret-checksum, scans).
Correctness checks in the earlier commits (unchanged, still sound)
azure_provider_modedefaults toazure, so theazure-<deployment>route names and default model survive upgrades; Foundry routesazure_ai-<deployment>; theazure-ai-vsazure_ai-name-collision case is covered by the coexist test.whengating keeps OpenAI-only fields hidden inazure_aimode and Foundry fields hidden inazuremode; stale stored values resolve empty.- The rendered
api_version(vsos.environ/AZURE_API_VERSION) keeps the route key stable across LiteLLM's startup overwrite; Foundry routes carrycustom_llm_provider: azure_aifor Responses routing. azure_ai_api_key/azure_ai_endpointare wired intolitellm-env-secretsand covered by both checksums.
The warm-env tests could not be run here (they need chart deps not vendored in this checkout; the failure is a missing-dependency error before any Azure logic), but the test-scripts CI check on this head passed.
No findings at or above the important level. The prior Important finding is resolved and regression-covered.
REVIEWED_HEAD: c6cf50e
REVIEW_REQUEST: 8fb5986a76384bf4bded02c1ecd2c0a4
VERDICT: APPROVED
|
Review complete. Summary of what I did: Review posted to PR #1337Substantive review: PR review 5532004461 — attached to the exact head commit Verified:
One note: the token I have belongs to the PR author, so GitHub rejects an |
The Replicated Admin Console currently only creates
azure/routes, so installs that need both Azure OpenAI and Azure AI Foundry must edit LiteLLM manually. This adds separate or combined provider configuration and an independent drop-unsupported-parameters toggle for each provider. Existing Azure deployment names and the default provider are preserved; Foundry uses separate credentials and explicitazure_airouting.End-to-end testing exposed two additional blockers fixed here:
AZURE_API_VERSIONduring startup. Background reconciliation then removes Azure routes whose generated ID changed. Render the configured, nonsecret API version directly into each route to keep it stable.OPENHANDS_PROVIDER_BASE_URL. Set it from the already configuredLITE_LLM_API_URLso runtimes use the installation's bundled or external proxy.Validation:
afbc293f1edd41a3ad466158349c8d86ran Python and returnedAZURE_131_OKusingazure-upgrade-131(openhands/azure-gpt-5.3-codex).394c3d20aa28485dae9c7d6d84a053ecran Python and returnedFOUNDRY_131_OKusingfoundry-upgrade-131(openhands/azure_ai-gpt-5.4).Separate app limitation found during testing: creating a new profile with the already selected default model reports "Model is required." Creating with another model and editing to Azure succeeds. This PR does not change that frontend form; the final Azure test used that UI workaround.
Up to date with main as of 2026-10-15 (merged with no conflicts; the scripts tests pass apart from local failures that also occur on main). The live validation above ran on the earlier base. Claude on Foundry and live service-principal authentication have not been tested. Addresses the installer portion of HF #131 / FDE-96; does not cover the ticket's separate budget/custom-parameter requests.