feat: remove openai tts params check

[YiminW]

No description provided.

[github-actions]

Review of feat: remove openai tts params check

Thanks for this. The direction — free-form params in the manifest, type handling in Python — is defensible, and bumping both manifest.json and pyproject.toml to 0.6.10 in lockstep is correct. My main concern is that the Python guards replacing the schema are not quite equivalent to what was removed, and one of them leaves the config in an unusable state.

1. url guard can leave self.url as None (functional bug)

if "url" in self.params:
    value = self.params["url"]
    self.url = value if isinstance(value, str) else None
    self.params.pop("url", None)
else:
    base_url = self.params.get("base_url", "https://api.openai.com/v1")
    if not isinstance(base_url, str):
        base_url = "https://api.openai.com/v1"   # <-- falls back

The two branches are inconsistent. base_url recovers to the default; url does not. With "url": 8080 (unquoted in JSON) we take the if branch, set self.url = None, pop the key, and never reach the base_url fallback. validate() does not check url either, so the extension starts up "successfully" and the failure surfaces later as an httpx error on the first synthesis request with None as the URL — hard to trace back to a config typo.

"url": "" reaches the same end state: isinstance("", str) is True, so the empty string is accepted and the key is popped.

Suggestion: fall through to the base_url/default path when url is not a usable string, and add a truthiness check on self.url in validate() so a bad endpoint fails fast as FATAL_ERROR rather than per-request.

2. Silent coercion with no diagnostics

All three guards swallow bad input silently. "speed": "fast" becomes 1.0, "api_key": 12345 becomes "". Previously the manifest float64/string declarations rejected these at property-load time with a type error pointing at the offending key; now the user gets either surprising behavior or a misleading downstream error ("API key or Authorization header is required" for what is actually a type problem).

Per docs/ai/L1/04_conventions.md error classification, invalid required config is FATAL_ERROR. At minimum please ten_env.log_warn() on each fallback so the substitution is visible in /tmp/task_run.log. Note _safe_float is a module-level free function with no ten_env, so surfacing a warning needs a return signal (e.g. return (value, used_default)) or moving the logic inline.

3. _safe_float accepts values OpenAI will reject

(TypeError, ValueError) does not cover everything that gets through float():

• "speed": "nan" / "inf" — float() succeeds, and nan serializes to the bare NaN token in the JSON payload, which is not valid JSON.
• "speed": false — yields 0.0, since bools are numbers to float().
• "speed": 100 — passes, then the vendor 400s; the documented OpenAI range is 0.25 to 4.0.

A range check collapses all of these into one guard and is cheap:

speed = _safe_float(self.params.get("speed", 1.0), 1.0)
self.params["speed"] = speed if 0.25 <= speed <= 4.0 else 1.0

nan fails every comparison, so this handles the non-finite cases for free.

4. Verify the schema loader reading of empty properties

"type": "object" with "properties": {} is an ambiguous way to express "free-form". Depending on the loader, empty properties may mean "no keys permitted" rather than "any key permitted" — which would break passthrough entirely rather than relaxing it. Worth confirming at runtime (start the extension with a custom param and check it reaches the payload), not just by reading the diff. If omitting properties altogether is the intended idiom, that is clearer.

5. Convention drift

Of 12 TTS extensions I sampled, 11 enumerate their params sub-properties in manifest.json; only tencent_tts_python uses an empty properties. This moves openai_tts2_python off the dominant pattern and also drops the type hints the TMAN Designer UI uses when users configure the extension. That may well be a deliberate direction change — but the PR body is empty, so a reviewer cannot tell.

Could you add a description covering the motivation? I suspect it is env-var expansion: an expression like ${env:OPENAI_SPEED|1.0} in property.json yields a string, which the old float64 declaration would reject, and that would explain _safe_float precisely. If that is the case, please say so, because it generalizes — the same string-arrives-from-env problem hits any numeric param added later, so a shared coercion helper in ten_ai_base may beat per-field guards here.

6. No test coverage for the new branches

Three new guards, zero tests. Existing tests only pass well-typed values, so every fallback path is uncovered. tests/test_params.py::test_url_and_base_url_configuration is the natural home:

• speed as "1.5" (the env-var case) — expect 1.5, not the default
• speed as "fast" — expect 1.0
• api_key as 12345 — expect "" and a clear failure
• url as 8080 — assert config.url is a usable endpoint (this currently fails, per item 1)
• base_url as a non-string — expect the default endpoint

Since the removal of the manifest schema is the crux, a test that loads a tests/configs/property_*.json through the real property system — with a string-typed speed — would guard the actual regression path better than constructing OpenAITTSConfig directly.

Existing assertions on "speed": 1.0 still hold, since _safe_float(1.0) returns 1.0.

Minor

• Title uses feat: for what is a validation/robustness change on a patch bump; fix: or refactor: fits better. Not a CI failure — commitlint accepts feat.
• Please run task format && task check && task lint in the container before merge; task lint fails on any pylint warning.

Verification note

I reviewed this statically. I could not execute the test suite — ten_ai_base is not vendored in the repo and no ten_agent_dev container was available in this environment, so the url-is-None path in item 1 is traced from the code rather than observed at runtime. Worth a quick manual confirmation.

Nothing here is a security concern: to_str() still encrypts api_key, vendor_metadata() masks it, and the client keeps stripping api_key/base_url from the payload.

[github-actions]

Review of "feat: remove openai tts params check"

The motivation makes sense: declaring a fixed params schema in manifest.json blocks passthrough of arbitrary vendor / third-party params, and moving to "properties": {} matches what 20+ other extensions under ai_agents/agents/ten_packages/extension/ already do (openai_llm2_python, tencent_tts_python, rime_tts, and others). Version bumps are correctly synced across manifest.json and pyproject.toml, and the Any used by the new helper was already imported at line 1, so there is no import gap.

The concern is that removing schema validation makes config.py the only remaining type guard, and the guards added here are partial. One of them converts a load-time rejection into a runtime failure.

1. Non-string url in params silently produces self.url = None (main issue)

if "url" in self.params:
    value = self.params["url"]
    self.url = value if isinstance(value, str) else None
    self.params.pop("url", None)

When params["url"] is not a string, self.url becomes None, and the else branch that would apply the base_url default is never reached because the outer branch was already taken. validate() (lines 82-97) does not check url, so nothing catches this at config time. The failure surfaces later in openai_tts.py:

async with self.client.stream("POST", self.config.url, headers=self.headers, json=payload)

Calling stream() with None raises on the first synthesis request. Before this PR the manifest rejected the bad type at load time; now it becomes a per-request runtime error, which is a worse failure mode than the validation that was removed. Falling through to the default endpoint preserves the intent of the guard without the dead end -- pop url first, use it only when it is a non-empty string, and otherwise run the existing base_url path (keeping the pylint: disable=no-member comments already used on those .get/.pop calls).

2. Coercing a bad api_key to empty string yields a misleading error

if "api_key" in self.params and not isinstance(self.params["api_key"], str):
    self.params["api_key"] = ""

validate() tests "api_key" in self.params and self.params["api_key"], and an empty string is falsy. So a user who writes "api_key": 12345 gets "API key or Authorization header is required for OpenAI TTS", which points at the wrong problem. Worse, if an Authorization header happens to be set, the bad key is discarded silently with no signal at all. Raising in validate(), or coercing with str(), would be more honest than blanking the field. One genuine upside of this guard: to_str() and _mask_metadata_secret() both assume a str, so it does prevent an encrypt() failure on the logging path.

3. Type guards are now inconsistent across params

api_key, url, base_url, and speed are guarded; model, voice, and instructions are not, and validate() only checks the latter two for truthiness rather than type. A non-string model still reaches the vendor and comes back as a 400. Since the manifest no longer types anything, it is worth picking one policy: guard every field the extension itself consumes, or rely on vendor errors uniformly. The current split will read as arbitrary to the next person in this file.

4. _safe_float admits values the vendor will reject

It only catches TypeError and ValueError, so several inputs pass through unchanged:

• float("inf") and float("1e400") give inf; float("nan") gives nan. Both serialize through json.dumps as bare Infinity / NaN, which is invalid JSON and will be rejected by the API.
• float(True) gives 1.0, so a boolean is silently accepted as a speed.
• There is no range check. The OpenAI API accepts speed within 0.25 to 4.0, so speed: 100 becomes a vendor 400.

Rejecting bool explicitly, adding a math.isfinite() check, and clamping to the documented 0.25-4.0 range would keep all of this local instead of surfacing as vendor errors.

5. No tests for any of the new behavior

This is the gap I would most want closed before merge. The purpose of the PR is relaxing validation, which shifts responsibility onto these coercion paths, and tests/test_params.py has no case for a non-numeric speed, a non-string url, a non-string api_key, or a non-string base_url. The existing suite covers only the happy path ("speed": 1.0 in the passthrough expectations at line 85). Each case is a few lines against OpenAITTSConfig plus update_params() directly, in the style already used by test_validate_with_authorization_header. A test asserting that a bad url still yields a usable endpoint would have caught issue 1.

Minor points:

• speed = self.params["speed"] if "speed" in self.params else 1.0 is just self.params.get("speed", 1.0). The file already uses .get(...) with a pylint: disable=no-member comment elsewhere, so the shorter form would be consistent.
• Dropping the manifest properties also removes the field hints the designer / playground UI surfaces for this extension. Presumably an accepted tradeoff, but worth a line in the PR description since it is user visible.
• The PR body is empty. A sentence on why the schema is being removed would help future readers see that this was deliberate rather than an accidental deletion.

Issue 1 is the one I would treat as blocking; the rest are judgment calls. Good to see the version bump handled in both files, which is easy to miss.

[github-actions]

Review: feat: remove openai tts params check

The core idea — stop declaring vendor params in the manifest and defend in code instead — is sound, and _safe_float is the right instinct: once speed is no longer declared float64, a value arriving from ${env:...} expansion is a string, so coercion in update_params() genuinely is now load-bearing. A few things below look like real problems, one of which I think can break the default config path.

Verification note. ten_ai_base (which owns AsyncTTS2HttpConfig / AsyncTTS2HttpExtension) is not vendored in this repo — it is a tman install dependency, so I could not read the base class. Anything below that depends on base-class behaviour is phrased as a question rather than a claim.

---

1. The new URL check in validate() may be unreachable — or a regression, depending on call order

def validate(self) -> None:
    if not isinstance(self.url, str) or not self.url.strip():
        raise ValueError("URL is required for OpenAI TTS")

update_params() always ends with self.url set (it falls back to https://api.openai.com/v1/audio/speech). So in the real lifecycle this branch is dead code — unless validate() runs before update_params(), in which case self.url is still None for every user who relies on the base_url default, and every default OpenAI config now raises at startup. That would be a hard regression.

The new test does not distinguish these cases, because it manufactures state that cannot occur naturally:

config.update_params()
config.url = ""      # <-- re-blanked by hand
config.validate()

Could you confirm the order ten_ai_base invokes these in? If update_params() always runs first, the check is unreachable and I would drop it (YAGNI); if it can run first, this needs a fix, not a test.

2. Out-of-range speed is silently discarded, and the range fights the PR goal

if not math.isfinite(converted) or not 0.25 <= converted <= 4.0:
    return default

Silent 1.0 on out-of-range. A user asking for speed: 5.0 gets normal-speed audio, no error, no log line — the least debuggable outcome. Prefer clamping to the nearest bound (4.0), or raising in validate(). Unparseable garbage to default is defensible; a deliberate out-of-range number to default is surprising.

The range contradicts the premise. This PR removes manifest-level validation so params can pass through freely — and openai_tts.py documents "Support for third-party TTS servers via base_url" and "Parameter passthrough". Then it hardcodes the OpenAI 0.25-4.0 semantic range in code, which is stricter than the manifest ever enforced. A self-hosted OpenAI-compatible server accepting speed: 6.0 now silently cannot. If the goal is passthrough, coerce the type and let the vendor reject the value.

Naming. _safe_float(value, default) is a generic name hiding a speed-specific constraint, and default is only ever 1.0. Either rename to _safe_speed(value) or take min/max params — I would pick the former per the YAGNI/KISS conventions in L1.

Minor: self.params["speed"] if "speed" in self.params else 1.0 reads better as self.params.get("speed", 1.0).

3. Leftover params["url"] can leak into the request payload

update_params() only pops url/base_url inside if not self.url:. When a top-level url is set and params["url"] is also present, the whole block is skipped, so params["url"] survives. The client then builds the payload as:

payload = {**self.config.params}
payload.pop("api_key", None)
payload.pop("base_url", None)   # note: no pop("url")

so url is sent in the JSON body to /audio/speech. OpenAI rejects unrecognised body params, so this is a 400 rather than a silent oddity. Pre-existing, but this PR reshapes exactly this code and removes the manifest schema that used to bound which keys appear — worth fixing here. (Existing test_url_in_params case 3 covers "top-level takes precedence" — does it assert "url" not in config.params?)

4. Security: the new test codifies an unencrypted Authorization in vendor_metadata()

extension.py masks api_key but passes the header through raw:

"api_key": _mask_metadata_secret(self.config.params.get("api_key", "")),
"authorization": authorization,   # <-- not masked

vendor_metadata() feeds connection_status_changed events and logs. config.to_str() correctly encrypts both; this path does not. The behaviour predates the PR, but the new test locks it in:

assert metadata["authorization"] == "Bearer header-secret"

Per L1 08_security and the sensitive-logging convention, I would route it through _mask_metadata_secret and update the assertion, rather than pin the leak with a test.

5. test_vendor_metadata.py — relative imports are inconsistent and may not import

from ..config import OpenAITTSConfig
from ..extension import OpenAITTSExtension

All 12 other import sites in tests/ use absolute imports inside the test function (from openai_tts2_python.config import ...). tests/bin/start runs pytest -s tests/, and conftest.py inserts parents[6] on sys.path. If pytest imports this module as top-level tests.test_vendor_metadata rather than openai_tts2_python.tests.test_vendor_metadata, from ..config raises ImportError: attempted relative import beyond top-level package. Please confirm this file actually runs under tests/bin/start (not just from an IDE) — and match the surrounding convention regardless. The module-level import of extension also pulls ten_runtime in at collection time, which the other files appear to defer deliberately.

Also in that file:

• test_vendor_metadata_prefers_authorization_header asserts that it does not prefer — both api_key and authorization are returned. The name contradicts the body.
• Two of the four tests duplicate test_vendor_metadata_does_not_convert_api_key_to_authorization and test_vendor_metadata_returns_raw_config_authorization_header, already in test_params.py.

6. Coverage gaps

The speed tests only assert the fallback path. An implementation that unconditionally returned 1.0 would pass the entire suite. Missing:

• Valid values are preserved — speed: 2.0 stays 2.0, and "2.0" (the string case this change exists for) coerces to 2.0. This is the most important missing test.
• Boundaries accepted — 0.25 and 4.0.
• The new api_key branch has no test at all — non-string api_key to "", and that validate() then correctly falls through to the Authorization header check.

Style: test_validate_rejects_empty_url uses try/except/else plus raise AssertionError; pytest.raises(ValueError, match="URL is required") is the idiomatic form.

7. Manifest: empty properties and cross-extension consistency

I read a sample of peer manifests — groq_tts_python, xai_tts_python, elevenlabs_tts2_python, gemini_tts_python, mistral_tts_python — and all keep fully typed nested params schemas. That makes openai_tts2_python the sole outlier.

• Is "properties": {} intentional versus dropping the key (or the whole params block)? An empty object is ambiguous to the next reader.
• If this is the first step of a repo-wide direction, say so in the PR body. The description is currently empty — for a change that alters a public config contract, a short rationale (e.g. manifest typing rejects ${env:} string expansion for numeric params) would help review and whoever bisects this later.

Minor: removing declared schema fields is arguably a minor bump rather than 0.6.9 -> 0.6.10. manifest.json and pyproject.toml are correctly in sync either way.

---

Before merge. Per L1 04_conventions, CI runs both task check (black, line-length 80) and task lint (pylint — a single warning is fatal):

sudo docker exec ten_agent_dev bash -c "cd /app && task format && task check && task lint"

Blocking for me: 1 (confirm the validate()/update_params() order — potential startup regression), 5 (confirm the new test file imports under the real runner), and a decision on the 2 range. 4 and 6 I would also like addressed, though they do not break anything today.

[github-actions]

Review: feat: remove openai tts params check

Nice direction overall — moving params to free-form and validating in Python is the right call for a passthrough vendor API, and the runtime-side hardening (_safe_float, URL validation, popping endpoint-only params) is a sensible replacement for what the manifest schema used to enforce. The empty "properties": {} schema matches what ~21 other extensions already do, and the version bump is applied consistently to both manifest.json and pyproject.toml.

A few things I would want addressed before merge, roughly in order of importance.

---

1. Un-masking the API key in vendor_metadata() looks like a security regression

-            "api_key": _mask_metadata_secret(
-                self.config.params.get("api_key", "")
-            ),
+            "api_key": self.config.params.get("api_key", ""),

This drops utils.encrypt() and returns the raw key, and the two tests that guarded it were flipped from != "test_api_key_123" to == "test_api_key_123". That inverts an assertion that was written specifically to prevent this.

It matters because vendor_metadata() is not an internal-only accessor — it gets embedded in metadata that leaves the extension. spatius_avatar_python/avatar_base.py:608 shows the shape:

error_metadata[VENDOR_METADATA_KEY] = self.get_vendor_metadata()
...
data = Data.create("error")

and integration_tests/asr_guarder/tests/test_connection_status.py:104 asserts on metadata.vendor_metadata arriving in connection_status_changed. So the value flows into graph messages and error payloads, which are routinely logged. docs/ai/L1/08_security.md is explicit: "Never log raw API keys, tokens, or credentials."

Note spatius encrypts its key in exactly this position. rime_tts and deepgram_asr_python do return raw keys, so the repo is genuinely inconsistent here — but this PR moves the OpenAI extension from the safe side to the unsafe side, which is the wrong direction to resolve that inconsistency.

Caveat on my confidence: ten_ai_base is an external dependency and is not vendored in the repo, so I could not read the base class to confirm the exact sink for the TTS path specifically. My evidence is the avatar base class and the ASR guarder test. If vendor_metadata() genuinely never leaves the process for AsyncTTS2HttpExtension, then this is fine and worth a comment saying so — but if that is not verified, I would restore _mask_metadata_secret. Either way, please say which in the PR description, since the change deletes a deliberate protection and its tests without explaining why.

Also worth noting: to_str() masking is untouched and still correct, so this is narrowly about the metadata path.

2. _safe_float validates type but not range

config = OpenAITTSConfig(params={"speed": "-5"})
config.update_params()   # -> params["speed"] == -5.0

OpenAI accepts speed in [0.25, 4.0]. The docstring on the new test says invalid params "must not leave the client unusable", but -5.0, 0, and 100 all pass through and produce a vendor 400 on the first synthesis request. Since the manifest no longer type-checks this field, _safe_float is the only remaining guard, so clamping (or falling back to 1.0) on out-of-range values would actually deliver the stated goal:

if not 0.25 <= converted <= 4.0:
    return default

The bool and non-finite handling is a good catch, though — isinstance(value, bool) before the float() call is the correct order.

3. test_validate_rejects_empty_url does not exercise a reachable state

config.update_params()
config.url = ""      # <- manually re-emptied after update_params already set it
config.validate()

update_params() can never leave url empty: if not self.url catches "" and assigns the default. So the test reaches its assertion only by mutating the object back into a state the normal flow cannot produce, which makes the new validate() check look like dead code.

There is a real case the new check catches — a whitespace-only top-level url, e.g. url=" ", where not self.url is False so it survives update_params() untouched. That is the case worth testing, and it would justify the check on its own.

Separately, the manual try/except/else is doing pytest.raises's job:

with pytest.raises(ValueError, match="URL is required for OpenAI TTS"):
    config.validate()

4. Missing test for the actual feature

The PR's whole purpose is letting arbitrary params through, but no test asserts that an unknown param survives. Worth adding, since this is the behavior the manifest change unlocks and the thing most likely to silently regress later:

config = OpenAITTSConfig(params={"api_key": "k", "some_new_openai_param": "x"})
config.update_params()
assert config.params["some_new_openai_param"] == "x"

5. Minor notes

• url in params was previously leaking into the request body. Before this change, when top-level url was set, params["url"] was left in place, and openai_tts.py:155-157 only pops api_key and base_url — so url was being POSTed to OpenAI as a body field. Popping both unconditionally in update_params() fixes that. Probably worth calling out as a fix rather than leaving it implicit, since it is a real behavior improvement hiding in a refactor.
• Non-string api_key coerced to "" then fails validate() with "API key or Authorization header is required". Reasonable fail-fast, but the message will confuse someone who passed an int — a type-specific error would be friendlier.
• update_params() is idempotent under the new code (second call: param_url is None, self.url already set) — good, worth keeping that property in mind if this grows.
• Commit type. feat: for the first commit is defensible but this is closer to refactor:/chore:. Passes commitlint either way, so non-blocking. Per AGENTS.md, remember the PR number gets appended on squash.
• Verification. I could not run task check / task lint / the test suite in this environment (no container, ten_runtime unavailable), so please confirm the strict pylint pass locally — docs/ai/L1/04_conventions.md notes even a single unused-import warning fails CI. The removed from ten_ai_base import utils in extension.py is correctly dropped alongside its only consumer, so that one is clean.

---

Summary: item 1 is the blocker — please either restore the masking or document why the raw key is safe on this path. Items 2–4 are test/robustness gaps that would make the new validation layer actually hold up its stated contract now that the manifest no longer does.