fix(settings): preserve stored LLM api_key when field is empty on save - #17807

This review was posted by an AI agent (OpenHands).

Reviewed head 82c7209, which matches the requested SHA. Scope: the change is Agent Canvas frontend state (src/hooks/mutation/use-save-settings.ts), which this repository owns; the complementary agent-server deep_merge hardening is correctly deferred to the SDK owner and flagged as follow-up. No product/architecture decision is needed.

What I verified

• The fix is minimal and targeted: an empty (or whitespace-only) agent_settings_diff.llm.api_key is dropped from the payload instead of forwarded as "", so the agent-server deep_merge preserves the stored key; a non-empty key is still trimmed and sent. Both src/routes/llm-settings.tsx and llm-settings-local-view.tsx already treat an empty key as "no change" (there is no clear-key affordance), so this aligns the shared writer with the documented UX instead of removing a supported capability.
• The regression test is genuine. npx vitest run __tests__/hooks/mutation/use-save-settings.test.ts passes on the head (7 passed), and the same file run against origin/main in a separate worktree fails the "drops an empty api_key" case with expected '' to be undefined. The test reaches the changed behavior and would catch a regression.
• CI: test-and-build (ubuntu) and test-and-build (windows) are green for this head.

Finding (material under the repo's own review guide)

• Testing and Production Evidence (.agents/skills/custom-codereview-guide.md): this is a runtime, user-visible bug fix, so the guide requires production-facing evidence from the same setup before and after the change — the base/released build reproducing the bug and the PR head showing the corrected behavior in the real Agent Canvas app. The attached pr-17807-repro-evidence.png and the HUMAN: note are a Python deep_merge reproduction plus the vitest run; those are backend/regression proof, not the app exercising the PR code. Please add the before/after app capture: on the LLM settings page, save a model + valid key, reopen, change only the model, save, then confirm the stored key survives (profile api_key_set stays true) and that switching to it mid-conversation no longer raises LLMAuthenticationError. If that state cannot be produced through the real product, the guide asks the PR to describe the change as defensive handling rather than a reproduced production bug.
• CI state (not a code problem): the head's status rollup reads FAILURE because an earlier Validate PR description run from the pre-edit body failed, while a later run on the same head passed. That leaves mergeStateStatus: BLOCKED; refresh/re-run that required check so it reports green before merge.

Acceptance criteria (issue #17806)

• An empty/untouched key field is treated as "no change" on save — implemented in the shared settings writer.
• A non-empty key is still trimmed and forwarded — covered by the added test.
• The stored key is demonstrably preserved through the real settings UI flow — needs the app-level before/after capture above.

The code change itself is correct and low-risk; the remaining gap is the required production evidence.

🔄 CHANGES REQUESTED

This review was posted by an AI agent (OpenHands).

Re-reviewed at the current head 26f0e86, which matches the requested SHA. The head moved from 82c7209 by a Merge branch 'main' commit only; the three files this PR touches (src/hooks/mutation/use-save-settings.ts, __tests__/hooks/mutation/use-save-settings.test.ts, .github/pr-assets/pr-17807-repro-evidence.png) are byte-identical between the two revisions, and nothing in the merged main changes the empty-key semantics this PR relies on. Scope: the change is Agent Canvas frontend state, which this repository owns; the complementary agent-server deep_merge hardening is correctly deferred to the SDK owner and flagged as follow-up. No product/architecture decision is needed.

What I verified

• The fix is minimal and targeted: an empty (or whitespace-only) agent_settings_diff.llm.api_key is dropped from the payload instead of forwarded as "", so the agent-server deep_merge preserves the stored key; a non-empty key is still trimmed and sent. Both src/routes/llm-settings.tsx (buildPayload) and src/components/features/settings/llm-profiles/llm-settings-local-view.tsx already treat an empty key as "no change" (there is no clear-key affordance), so this aligns the shared writer with the documented UX rather than removing a supported capability.
• The regression test is genuine. npx vitest run __tests__/hooks/mutation/use-save-settings.test.ts passes on the head (7 passed), and the same test file run against origin/main in a separate worktree fails the "drops an empty api_key" case with AssertionError: expected '' to be undefined. The test reaches the changed behavior and would catch a regression.
• CI for this head: test-and-build (windows) passed and Validate PR description now passes (the earlier red run from the pre-edit body is superseded). test-and-build (ubuntu) and the image builds were still in progress when I looked; please confirm they report green before merge.

Finding (material under the repo's own review guide)

• Testing and Production Evidence (.agents/skills/custom-codereview-guide.md): this is a runtime, user-visible bug fix, so the guide requires production-facing evidence from the same setup before and after the change - the base/released build reproducing the bug and the PR head showing the corrected behavior in the real Agent Canvas app. The attached pr-17807-repro-evidence.png and the HUMAN: note are a Python deep_merge reproduction plus the vitest run; those are backend/regression proof, not the app exercising the PR code. Please add the before/after app capture: on the LLM settings page, save a model + valid key, reopen, change only the model, save, then confirm the stored key survives (profile api_key_set stays true) and that switching to it mid-conversation no longer raises LLMAuthenticationError. If that state cannot be produced through the real product, the guide asks the PR to describe the change as defensive handling rather than a reproduced production bug.

Acceptance criteria (issue #17806)

• An empty/untouched key field is treated as "no change" on save - implemented in the shared settings writer.
• A non-empty key is still trimmed and forwarded - covered by the added test.
• The stored key is demonstrably preserved through the real settings UI flow - needs the app-level before/after capture above.

The code change itself is correct and low-risk; the remaining gap is the required production evidence.

🔄 CHANGES REQUESTED

This review was posted by an AI agent (OpenHands).

Reviewed head f8714cb, which matches the requested SHA. The head moved from the previously reviewed 26f0e86 by one commit that only replaces .github/pr-assets/pr-17807-repro-evidence.png (68506 -> 267439 bytes); the two code files are byte-identical to the earlier revisions. Scope: this is Agent Canvas frontend state (src/hooks/mutation/use-save-settings.ts), which this repository owns; the complementary agent-server deep_merge hardening is correctly deferred to the SDK owner and flagged as follow-up. No product/architecture decision is needed.

What I verified

• The fix is minimal and targeted: an empty (or whitespace-only) agent_settings_diff.llm.api_key is now deleted from the payload instead of forwarded as "", so the agent-server deep_merge preserves the stored key; a non-empty key is still trimmed and sent. src/routes/llm-settings.tsx (buildPayload) and llm-settings-local-view.tsx already treat an empty key as "no change" (there is no clear-key affordance), so this aligns the shared writer with the documented UX rather than removing a supported capability. Other writers (settings-form.tsx, agent-settings.tsx) never send api_key, so the shared writer is the single source of the bad payload.
• The regression test is genuine. npx vitest run __tests__/hooks/mutation/use-save-settings.test.ts passes on this head (7 passed), and the same file run against origin/main in a separate worktree fails the "drops an empty api_key" case with expected '' to be undefined.
• CI for this head: test-and-build (ubuntu), test-and-build (windows), Validate PR description, check-pr-artifacts, pr-title, and the image build/publish jobs are all green, and the commit status rollup is SUCCESS. reviewDecision is REVIEW_REQUIRED, so this review is the remaining gate rather than a CI failure.

Finding (material under .agents/skills/custom-codereview-guide.md, "Testing and Production Evidence")

• The updated evidence still does not demonstrate the corrected behavior through the real app. The reproduction in "How to Test" is two curl PATCH /api/settings calls against the local agent-server: it shows the backend deep_merge semantics (api_key: "" wipes the stored key to null, an omitted api_key preserves it), and pr-17807-repro-evidence.png pairs a real /settings/llm page with that live-backend before/after. The guide is explicit that a model-only save through the UI must be shown in the app, not via raw API calls: ""Real app" evidence must exercise the production integration path", and "Runtime and user-visible bug fixes require the same production-facing setup before and after the change. The base or released version must reproduce the bug; the PR head must show the corrected behavior." The curl pair proves what the fixed frontend now sends, but the app screenshot shows the profiles list rendering, not the changed save behavior, and no app-level capture shows the key surviving a model-only save (nor the failure before it). This matters because the reported bug is intermittent and view-tier/dirty-state dependent (the issue notes the "" vs null distinction does not reproduce on every save), so the payload shape actually sent from the UI is the thing that needs to be observed. Please add the app-level before/after: run the base build, on /settings/llm save a model plus a valid key, reopen, change only the model, save, and show the stored key is cleared (then the switch failing with LLMAuthenticationError); repeat on this head and show the key survives (profile api_key_set stays true) and the switch succeeds. A video showing the save and the resulting profile/key state fits the temporal nature of the claim; if that state cannot be produced through the real product, the guide asks the PR to describe the change as defensive handling rather than a reproduced production bug.

Acceptance criteria (issue #17806)

• An empty/untouched key field is treated as "no change" on save - implemented in the shared settings writer.
• A non-empty key is still trimmed and forwarded - covered by the added test.
• The stored key is demonstrably preserved through the real settings UI flow - still needs the app-level capture above.

The code change is correct and low-risk; the remaining gap is the app-level production evidence the repo's review guide requires.

🔄 CHANGES REQUESTED

This review was posted by an AI agent (OpenHands).

Reviewed head 01bd914, which matches the requested SHA. The head moved from the previously reviewed f8714cb by a Merge branch 'main' commit; the three files this PR touches (src/hooks/mutation/use-save-settings.ts, __tests__/hooks/mutation/use-save-settings.test.ts, .github/pr-assets/pr-17807-repro-evidence.png) are byte-identical between the two revisions, and nothing merged from main in between touches the settings-save path. Scope: this is Agent Canvas frontend state, which this repository owns; the complementary agent-server deep_merge hardening is correctly deferred to the SDK owner and flagged as follow-up. No product/architecture decision is needed.

What I verified

• The fix is minimal and targeted: an empty (or whitespace-only) agent_settings_diff.llm.api_key is deleted from the payload instead of forwarded as "", so the agent-server deep_merge preserves the stored key; a non-empty key is still trimmed and sent. src/routes/llm-settings.tsx (buildPayload) and llm-settings-local-view.tsx already treat an empty key as "no change" (there is no clear-key affordance), and the other writers (settings-form.tsx, agent-settings.tsx) never send api_key, so the shared writer was the single source of the bad payload. No callers depend on "" reaching the backend to clear a key.
• The regression test is genuine. npx vitest run __tests__/hooks/mutation/use-save-settings.test.ts passes on this head (7 passed), and the same file run against origin/main in a separate worktree fails the "drops an empty api_key" case with AssertionError: expected '' to be undefined.
• CI for this head: test-and-build (ubuntu), test-and-build (windows), Validate PR description, check-pr-artifacts, pr-title, and the image build/publish jobs are green; the commit status rollup is SUCCESS. reviewDecision is REVIEW_REQUIRED, so this review is the remaining gate, not a CI failure.

Evidence

• The updated pr-17807-repro-evidence.png pairs a real /settings/llm capture (running from this branch against the live local agent-server) with a live-backend before/after: the buggy payload (api_key: "") leaves agent_settings.llm.api_key wiped to None after a model-only save, while the fixed payload (no api_key) leaves it preserved (**********); both return HTTP 200. The "Before" attachment is a real-app capture of the resulting symptom ("Your LLM API key appears to be invalid or has expired." after switching to the affected profile). Together with the unit test that pins the exact payload the fixed writer emits, this exercises the production integration path for the changed behavior.
• Residual, non-blocking: there is still no app-level "after" capture of the model-only save preserving the key in the UI (the PR body's "After" heading carries no image). Given the live-backend before/after plus the genuine regression test, this does not change the merge decision; if the maintainer wants the UI round-trip recorded too, that is the smallest additional artifact.

Acceptance criteria (issue #17806)

• An empty/untouched key field is treated as "no change" on save - implemented in the shared settings writer.
• A non-empty key is still trimmed and forwarded - covered by the added test.
• The stored key is preserved through the model-only save - demonstrated against the live agent-server with the fixed payload and pinned by the regression test.

Eval risk: none. The change only affects which settings payload the frontend sends on save; it does not touch prompts, tool selection, planning, memory, or evaluation paths.

✅ APPROVED