test(profiles): add integration tests for ProfilesClient and switchLlm

[VascoSch92]

Summary

Follow-up to #148, which shipped ProfilesClient and RemoteConversation.switchLlm with mocked unit tests only (mocked global.fetch verifies wire shape but not server-side drift). This adds the live-server integration coverage tracked in OpenHands/software-agent-sdk#4745, in deterministic-api.integration.test.ts.

Profiles CRUD round-trip — save → get → list → activate → rename → delete via manager.profiles against a running agent-server. Beyond exercising each endpoint, it asserts behavior a mock can't:

• the saved LLM config round-trips (config.model) and survives the atomic rename;
• the default (no X-Expose-Secrets) response nulls api_key and reports api_key_set instead;
• active_profile tracks the active profile across activate and follows it through a rename to the new name;
• the old name 404s once renamed; the profile is gone from the list after delete.

Both names are cleaned up in finally (deleteProfile is idempotent server-side).

switchLlm — next to the existing switchProfile 404 check. switchLlm swaps the LLM in place keyed by usage_id (first-write-wins in the registry), so it switches to a fresh usage_id and reads the model back off the agent (state.getAgent()) to prove the swap actually took effect — reusing the agent's existing usage_id would be a silent no-op.

Both tests are deterministic (a dummy LLM, no provider calls) and live in deterministic-api.integration.test.ts, so they run in CI's deterministic integration job (npm run test:integration:deterministic) without an LLM key.

Tasks from OpenHands/software-agent-sdk#4745

• Profiles CRUD round-trip integration test (save → get → list → activate → rename → delete).
• switchLlm assertion next to the existing switchProfile 404 check.
• End-to-end coverage of X-Expose-Secrets: encrypted | plaintext (cipher round-trip).

The X-Expose-Secrets item stays deferred. The SDK feature it was blocked on (software-agent-sdk#3161) is now in the agent-server image CI runs (v1.23.0), but the encrypted round-trip needs an agent-server started with OH_SECRET_KEY configured — without a cipher the server returns 503 (Encryption not available: OH_SECRET_KEY is not configured). The current integration-tests.yml workflow starts the container without a cipher, so covering that mode is a separate change (workflow + a cipher-aware test). Leaving OpenHands/software-agent-sdk#4745 open to track it.

Verification

• npm run lint, npm run format:check — clean; npm test — unit suite green.
• New integration file type-checks under --strict.
• The endpoints and behaviors exercised (profiles router, /switch_llm, config round-trip, active_profile tracking through rename, usage_id swap semantics, 404-after-rename, idempotent delete) were verified against the agent-server source at the exact v1.23.0 tag CI runs.

[all-hands-bot]

🟢 Good taste - Excellent integration test coverage

This PR adds thorough, deterministic integration tests for profiles CRUD and switchLlm functionality. The tests exercise real API behaviors that mocks can't verify:

• Profiles test methodically walks through the full lifecycle: save → get → list → activate → rename → delete, verifying state at each step including non-obvious behaviors like active_profile tracking through rename and API key nulling in responses.
• switchLlm test uses a fresh usage_id and reads agent state back to prove the swap worked (well-commented rationale).
• Clean code: proper async/await, cleanup in finally blocks, clear naming, comments only where they add value.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW

Adds tests only, no production code changes. Tests are deterministic (no real LLM calls), properly isolated with unique names, and include cleanup for both success/failure paths.

VERDICT:
✅ Worth merging - High-quality integration tests that solve the real problem of verifying server-side behavior after shipping with mocks.

KEY INSIGHT:
Tests verify exactly the kinds of stateful behaviors (config round-tripping, active profile tracking, atomic rename semantics) that mocked unit tests fundamentally cannot catch.

---

Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/typescript-client/actions/runs/26460743099

[all-hands-bot]

🟢 Good taste - Excellent integration test coverage

This PR adds thorough, deterministic integration tests for profiles CRUD and switchLlm functionality. The tests exercise real API behaviors that mocks can't verify:

• Profiles test methodically walks through the full lifecycle: save → get → list → activate → rename → delete, verifying state at each step including non-obvious behaviors like active_profile tracking through rename and API key nulling in responses.
• switchLlm test correctly uses a fresh usage_id to avoid silent no-ops and verifies the swap actually took effect by reading the model back from the agent.

Both tests include proper cleanup in finally blocks and are well-commented where behavior is non-obvious. The code is clean, follows existing test file conventions, and will run in CI's deterministic integration job without requiring an LLM key.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW
  Test-only change with comprehensive coverage, proper isolation, and cleanup. No production code modified.

VERDICT:
✅ Worth merging: High-quality integration tests that verify real server behavior

KEY INSIGHT:
The tests verify atomic server behaviors (rename tracking active_profile, config persistence through rename, idempotent delete) that unit tests with mocked fetch cannot catch—exactly the gap integration tests should fill.

---

Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/typescript-client/actions/runs/26461177927

[all-hands-bot]

This cleanly delivers the live-server coverage described in #149 — the behaviors verified here (api_key nulling, active_profile tracking through rename, usage_id swap semantics) are fundamentally untestable with mocked fetch, so the upgrade from unit to integration coverage is well-justified.

Two 🟡 suggestions below: one to strengthen the list-step assertions using the full ProfileInfo shape, and one to assert active_profile clears after the active profile is deleted. Neither is a blocker; the test is otherwise thorough and well-structured.

The inline comments explaining non-obvious server semantics (first-write-wins usage_id, X-Expose-Secrets behavior) are genuinely useful documentation that should stay in the test.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation.

[all-hands-bot]

🟡 Suggestion: This only checks that the profile name appears in the list. ProfileInfo also surfaces model and api_key_set — verifying those here would confirm the list projection is consistent with the detail endpoint and would catch any field-mapping drift between the two representations:

Suggested change

	expect(list.profiles.some((profile) => profile.name === profileName)).toBe(true);
	const found = list.profiles.find((p) => p.name === profileName);
	expect(found).toBeDefined();
	expect(found?.model).toBe('dummy/model');
	expect(found?.api_key_set).toBe(true);

[all-hands-bot]

🟡 Suggestion: renamedProfile was the active profile when it was deleted, so active_profile should revert to null after the deletion. Adding that assertion would complete the lifecycle story this test is telling — currently the "profile is gone from the list" check and the active_profile cleanup are separate behaviors that aren't both verified:

Suggested change

	expect(finalList.profiles.some((profile) => profile.name === renamedProfile)).toBe(false);
	expect(finalList.profiles.some((profile) => profile.name === renamedProfile)).toBe(false);
	expect(finalList.active_profile).toBeNull();

[all-hands-bot]

🟢 Good taste — Solid integration test coverage for ProfilesClient and switchLlm.

Note: An existing automated review has already approved this PR. This is a complementary independent review confirming those findings and adding one minor observation.

---

Profiles CRUD test walks the full lifecycle (save → get → list → activate → rename → delete) and asserts meaningful state at every step, not just at the end:

• api_key nulling in the default (no X-Expose-Secrets) response — impossible to verify with mocked fetch
• active_profile pointer correctly following the profile through an atomic rename
• Old name 404-ing immediately after rename
• finally block cleans up both names idiomatically with .catch(() => undefined) — handles every failure path without masking the original error

switchLlm test is correct by design: using a fresh usage_id avoids the first-write-wins silent no-op, and reading the model back from conversation.state.getAgent() proves the swap actually mutated server state rather than just accepting the API response at face value. The inline comments explain the rationale clearly.

Minor observation: usage_id: 'switched' is a static string. In CI with a fresh container per run this is safe, but if the test server ever accumulates state across runs the usage_id would already be registered and the switch would silently no-op — the same trap the comment warns against. A uniqueFileName-style value (already imported) would make this unconditionally safe. Non-blocking for a fresh-server CI setup, but worth considering for robustness.

Deferred coverage (X-Expose-Secrets: encrypted | plaintext) is properly tracked in #149 with a clear explanation of the blocker — good hygiene.

---

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟢 LOW — Test-only changes. No production code modified. Tests are deterministic (dummy LLM, no provider calls), isolated with unique names, and clean up after themselves.

VERDICT:
✅ Worth merging — Tests verify exactly the stateful, server-side behaviors that mocked unit tests structurally cannot catch.

KEY INSIGHT:
The active_profile tracking through rename and the first-write-wins usage_id semantics are the kind of contract details that only live integration tests can enforce.

---

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better by adding a .agents/skills/custom-codereview-guide.md file with the /codereview trigger. See the customization docs for the required format.

---

This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation.

[all-hands-bot]

Minor: 'switched' is a static string — if the test server ever accumulates state across runs, this usage_id would already be registered and switchLlm would be a silent no-op (the exact trap the comment above warns about). A unique value (e.g. uniqueFileName('usage') or uniqueDirName('usage')) would make this unconditionally safe regardless of server state. Non-blocking in CI where a fresh container runs per job, but cheap to harden.

[VascoSch92]

Merging as they are just regression tests