[AgentProfile][ts-client] Test ACP provider credential descriptors + re-sync claude-code set_session_model mirror

[simonrosenberg]

• A human has tested these changes.

---

Why

Part of the AgentProfile epic (OpenHands/software-agent-sdk#3713), Phase 3 —
this is the "ACP provider descriptor extension" slice of
OpenHands/software-agent-sdk#3722. The canvas ACP authentication section
(sdk#3728) renders provider-specific credential forms by reading credential
metadata off ACP_PROVIDERS (api_key_env_var, base_url_env_var,
file_secrets) instead of hardcoding provider lists.

Those descriptor fields already exist on ACPProviderInfo and are
populated for all three providers — they landed with the registry mirror
work (#173/#187/#205), which post-dates the issue text (the issue still
references the pre-mirror src/utils/acp.ts; the registry now lives in
src/models/acp.ts). Their shape is a superset of the issue's request
(string | null plus a richer file_secrets mirroring the SDK
field-for-field), so changing them to optional ?: string would be a
regression. The remaining gaps were: no test locking the metadata in,
and a drifted mirror field that left CI red on main.

Summary

• Add src/__tests__/acp-providers.test.ts — asserts every built-in
  provider (claude-code, codex, gemini-cli) exposes api_key_env_var,
  base_url_env_var, and file_secrets, checks each provider's expected
  env-var values and file-secret descriptors, and covers getAcpProvider
  (known key, custom, unknown, and falsy inputs).
• Re-sync the one drifted mirror field: claude-code
  supports_set_session_model false → true to match SDK main, which
  flipped it (sdk#3654) after claude-agent-acp 0.30.0 was found to ignore
  session-_meta model selection. The validate-acp-providers drift check
  was red on main until this.

Issue Number

OpenHands/software-agent-sdk#3722 (epic OpenHands/software-agent-sdk#3713)

How to Test

Locally, against SDK main (what CI's validate-acp-providers pins to):

npm run build         # tsc clean
npm run lint          # 0 errors
npm run format:check  # clean
npm run test:coverage # 224 passed (13 suites), incl. 11 new acp-providers tests

pip install -r scripts/requirements-acp-check.txt
python scripts/check-acp-drift.py
# OK: src/models/acp-providers.json matches openhands-sdk (3 providers).

Type

• Refactor
• Bug fix
• Feature
• Breaking change
• Docs / chore

Notes

• Purely additive: no public type or interface change — ACPProviderInfo
  already carried the credential fields; this only adds test coverage and a
  one-line mirror re-sync.
• The CI security job (npm audit) is failing on main too — pre-existing
  transitive-dependency advisories (jest/babel/ws); this PR touches neither
  package.json nor package-lock.json.

🤖 Generated with Claude Code

[simonrosenberg]

CI on 7d92759: 6/7 green — build, smoke-test, test (20.x), test (22.x), integration-test, and validate-acp-providers all pass. This PR turns validate-acp-providers green (it is currently red on main from the claude-code supports_set_session_model drift this PR re-syncs).

The single red check is security (npm audit), which is pre-existing and also failing on main — transitive advisories in jest/babel/ws. This PR touches neither package.json nor package-lock.json, so it is out of scope here (tracked separately).

[all-hands-bot]

✅ Review complete.

This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here.

[all-hands-bot]

Code Review

🟢 Good taste — Clean, focused implementation with solid test coverage.

Summary

This PR adds comprehensive tests for the ACP provider credential descriptor fields (api_key_env_var, base_url_env_var, file_secrets) and fixes the supports_set_session_model value for claude-code from false to true.

Assessment

[CRITICAL ISSUES]
None.

[IMPROVEMENT OPPORTUNITIES]

• [src/tests/acp-providers.test.ts] Scope note: The PR title mentions "Extend ACPProviderInfo with credential descriptor fields" but the PR also changes supports_set_session_model from false to true for claude-code. This is a separate concern from the test coverage. Consider either:
  • Expanding the title to mention the supports_set_session_model fix, or
  • Moving this change to a separate PR focused on that fix

[STYLE NOTES]
None — tests are well-structured and follow the project's testing patterns.

[TESTING GAPS]
None — The tests provide good coverage:

• ✅ Verifies all three credential descriptor fields exist on every provider
• ✅ Tests specific env var values match expected values
• ✅ Verifies file secret structure for Codex and Gemini CLI
• ✅ Tests getAcpProvider() function behavior including edge cases (null, undefined, unknown keys)

[RISK ASSESSMENT]

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

This is a low-risk change. The JSON modification updates a single boolean flag, and the test file adds validation that these fields exist and have correct values. The tests run and pass.

Verdict

✅ Worth merging: Core logic is sound, tests provide good coverage, tests pass.

Key Insight

The PR correctly validates that all built-in ACP providers expose the credential metadata that the canvas UI requires, and the supports_set_session_model fix ensures claude-code can set its model via the protocol call.

---

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

[simonrosenberg]

Thanks for the review! Addressed the scope note by expanding the title to call out the supports_set_session_model re-sync.

I kept it in this PR rather than splitting it out because the validate-acp-providers drift check pins to SDK main and is currently red on main from exactly this drift (claude-code supports_set_session_model false→true, flipped in software-agent-sdk#3654). Splitting it would leave this PR's CI red, so the one-line mirror re-sync rides along with the test that depends on a green registry.