Skip to content

fix(asr): align iFLYTEK integration for 0.2.1 - #2308

Open
dillonliang224 wants to merge 1 commit into
mainfrom
fix/iflytek-asr-0.2.1-official
Open

dillonliang224 wants to merge 1 commit into
mainfrom
fix/iflytek-asr-0.2.1-official

Conversation

@dillonliang224

Copy link
Copy Markdown
Contributor

Align reconnect error naming and Python compatibility with the reviewed conversational agent integration. Update extension metadata, release documentation, and affected tests for the 0.2.1 package.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Reviewed the full diff plus the surrounding package (reconnect_manager.py, extension.py, protocol.py, addon.py, tests, manifest/pyproject) and the lint configuration in this repo. This is a tight, well-scoped patch: an exception rename, a stdlib import modernization, a lint suppression, and a patch version bump with changelog. No functional behavior changes. Verification below is static — I did not execute the test suite (it needs ten_runtime_python/ten_ai_base on PYTHONPATH).

What is correct

The rename is complete. All 10 references to ReconnectLimitReached on main are updated: the class definition and raise in reconnect_manager.py; the import, except, construction, and isinstance in extension.py; and both test files. No stale references remain, and __init__.py only does from . import addon, so the class was never part of the public surface of the package — a rename without a back-compat alias is safe here.

Line-length discipline holds. CI enforces black --line-length 80 (ai_agents/Taskfile.yml, black-format-check). The wrapped raise in reconnect_manager.py was necessary (the one-liner would be 86 chars), and the widened test_vendor_error.py call lands at exactly 80, which passes. Good attention to detail.

collections.abc.Mapping is the right call. typing.Mapping has been deprecated since 3.9, and with requires-python = ">=3.10" both the isinstance() checks (protocol.py lines 189, 215, 272) and the subscripted annotations work unchanged. No behavior difference.

Docs version bumps are correctly scoped. PRODUCTION_READINESS.md states "this checklist applies to version X", so bumping it to 0.2.1 is right. The remaining 0.2.0 mentions in README.md and docs/README.*.md are historical statements ("starting with version 0.2.0, params must be nested") and should NOT be bumped — please leave those as-is. manifest.json, pyproject.toml, and the changelog cover the actual version surface; nothing was missed.

Main observation: the lint suppression targets a linter this repo does not run

The noqa: PLC0415 on the lazy import in addon.py is a ruff code (import-outside-top-level). Two things follow:

  1. Nothing in CI will act on it. .github/workflows/ai_agents.yaml runs task check (black) and task lint, which calls agents/scripts/pylint.sh, which runs pylint with tools/pylint/.pylintrc. That rcfile disables I,C,R wholesale (line 136), so the pylint equivalent C0415 (import-outside-toplevel) is already off repo-wide. The only ruff reference in the tree is .coderabbit.yaml — review-bot tooling, not an enforced gate. If the goal were to silence pylint, the idiomatic form would be pylint: disable=import-outside-toplevel, but that is also unnecessary given the rcfile.

  2. It creates a one-off divergence. Several other addons do the identical lazy import with no suppression (anthropic_llm2_python, aliyun_asr, azure_mllm_python). Of the ~41 suppression comments across ai_agents/, every one is F401/F811 — this would be the first PLC0415. There is also a small RUF100 (unused-noqa) risk if ruff is ever run without PLC0415 selected.

The same reasoning applies to the rename itself, which reads as satisfying the ruff rule N818 (error-suffix-on-exception-name) — a good convention, but with no committed ruff config it will not be enforced against the other 90+ extensions. Suggestion: either drop the suppression and keep just the rename, or (better, if the team wants these conventions) land a shared ruff config and wire it into Taskfile.yml so the whole tree converges rather than one package.

Relatedly, the changelog says the change "document[s] the intentional lazy extension import", but a bare suppression code is not documentation. If the intent is to explain WHY the import is deferred (import cost, or circularity at addon registration), a short prose comment carries that; the code alone does not.

Minor

  • Duplicated message literal. The string "maximum reconnection attempts reached" now appears in both reconnect_manager.next_delay() and extension.py:393. Since both sites are being touched anyway, a module-level constant (or a zero-arg factory on the exception class) would keep them from drifting.
  • Commit type. A patch bump is defensible precisely because the class is not exported, so the version is fine as-is — but per AGENTS.md this is closer to chore:/refactor: than fix:, since no user-visible defect is repaired. Branch name and title format otherwise match the conventions, and the changelog date matches the bump.

Test coverage, performance, security

Test changes are the correct mechanical follow-through. The pytest.raises(ReconnectLimitReachedError) assertion in test_reconnect_manager.py and the vendor-error classification path in test_vendor_error.py (which pins the vendor code to reconnect_exhausted) together mean a missed rename site at extension.py:575 would have failed the suite. No new tests are warranted for a rename.

test_package_contract.py validates manifest/property alignment but does not assert the version string, so the bump itself is unverified by tests — probably fine, though a one-line assert that the manifest.json and pyproject.toml versions agree would cheaply prevent those two drifting apart in future bumps.

No performance implications. No security surface touched — no credential handling, logging, or redaction logic changed.

Verdict

Good to merge once the PLC0415 suppression question is settled. My preference is to drop it (it is inert under the actual lint config of this repo, and inconsistent with sibling addons), or to pair it with a real ruff config. Everything else in the patch is clean and correct.

@dillonliang224
dillonliang224 force-pushed the fix/iflytek-asr-0.2.1-official branch from dec4da4 to b3ee178 Compare September 2, 2026 05:56
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review: fix(asr): align iFLYTEK integration for 0.2.1

Small, well-scoped PR. The rename is complete and behavior-preserving, and the Mapping import fix is correct. One substantive issue with the new noqa pragma, plus a minor compat suggestion.

1. # noqa: PLC0415 is a no-op in this repo (main finding)

addon.py:

from .extension import IFlytekAsrExtension  # noqa: PLC0415

This suppresses nothing in CI, for two independent reasons:

  • Wrong linter. PLC0415 is a ruff rule code. This repo's lint gate is pylint — agents/scripts/pylint.sh invoking tools/pylint/.pylintrc. There is no ruff config anywhere in the tree. pylint does not honor # noqa comments at all; it only reads # pylint: disable=....
  • Rule is already off. Even under pylint's own name for it (C0415 / import-outside-toplevel), .pylintrc disables the whole C category (I,C,R, in [MESSAGES CONTROL]), so it could never have fired.

The repo idiom is # pylint: disable=... (51 occurrences). The only # noqa uses in agents/ are F401/F811 in test files, which pylint skips anyway (ignore=...,tests,...).

Since the goal per the CHANGELOG is to document the lazy import, I'd suggest a plain comment stating the actual reason:

# Imported lazily so the extension module is not loaded until an
# instance is requested.
from .extension import IFlytekAsrExtension

Relatedly, the CHANGELOG line "document the intentional lazy extension import for lint compatibility" attributes a motive that doesn't hold — nothing was failing lint. Worth rewording to whatever the real reason is (import cost? circular import?).

Optional, and pre-existing rather than introduced here: this is the only addon.py in the repo doing a deferred import — every other extension imports at module top level, matching the 04_conventions.md example. If there is no concrete reason for the deferral, dropping it entirely is more consistent than documenting it.

2. Renamed exception has no back-compat alias (minor)

ReconnectLimitReachedReconnectLimitReachedError is a source-breaking change to an exported class landing in a patch bump (0.2.0 → 0.2.1). I confirmed there are zero references outside this extension package, and no __all__ or docs/JSON referencing the old name, so in-tree impact is nil. But any out-of-tree code doing except ReconnectLimitReached: now silently stops catching — a failure mode with no error message.

A one-line alias would make it free:

ReconnectLimitReached = ReconnectLimitReachedError  # deprecated alias

Reasonable to skip if this extension is treated as having no external consumers — flagging so it's a deliberate call rather than an oversight.

What checks out

  • Rename is complete. All 6 production references (reconnect_manager.py, extension.py) and all 4 test references updated; no stragglers repo-wide.
  • Behavior is preserved on the error path. I traced _send_framework_error: vendor_code for this branch is the hardcoded literal "reconnect_exhausted", not derived from __class__.__name__, so the wire output is unchanged. The getattr(error, "code", "client_error") default is overridden by the isinstance branch as before. test_vendor_error.py's code == "reconnect_exhausted" assertion still holds — no wire-format regression.
  • Mapping import is correct and safe. typing.Mapping has been deprecated since 3.9. Note this symbol is used at runtime in three isinstance() checks (protocol.py:189, 215, 272), not just annotations — collections.abc.Mapping supports isinstance and, with requires-python = ">=3.10", subscripting (Mapping[str, Any]) works at runtime too. No behavioral change.
  • Formatting. The reformatted raise in reconnect_manager.py is exactly what black --line-length 80 produces (adding Error pushed the original past 80 cols), so task format was clearly run.
  • Version bump is consistent. manifest.json, pyproject.toml, and PRODUCTION_READINESS.md all moved to 0.2.1. The 0.2.0 mentions left in the localized READMEs are historical references to the params migration and correctly untouched — worth noting since a blind grep-and-bump would have wrongly rewritten them.
  • Test coverage is proportionate. Mechanical import/assertion updates are the right scope; a pure rename plus import modernization introduces no new behavior needing new tests.

Conventions

Commit/PR title fix(asr): align iFLYTEK integration for 0.2.1 satisfies commitlint (conventional header, lowercase subject, no trailing period, under 100 chars). Since neither the rename nor the import change fixes a user-visible defect, refactor: or chore: would describe this more accurately — but fix: is a valid type and won't fail CI, so this is a nit.

Only item I'd treat as blocking is #1, and just the two-line fix (drop the dead pragma, correct the CHANGELOG claim).

Two caveats on my verification: I reviewed the diff against the base checkout and could not check out the PR head, so line-level context around the edits is inferred from the diff rather than read in place. I also could not execute pylint, black, or pytest in this environment — the formatting and lint conclusions above come from reading the configs, not from running them.

@dillonliang224
dillonliang224 force-pushed the fix/iflytek-asr-0.2.1-official branch from b3ee178 to 06327dc Compare September 2, 2026 06:29
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review: fix(asr): align iFLYTEK integration for 0.2.1

Focused, low-risk change. The rename is applied consistently and the release metadata is coherent. Details below, with two things worth a second look before merge.

What I verified

The rename is complete. All 10 occurrences of ReconnectLimitReached are updated (reconnect_manager.py x2, extension.py x4, tests/test_reconnect_manager.py x2, tests/test_vendor_error.py x2). I grepped the whole repo for the old name outside the extension directory and for references to iflytek_asr_python from other packages -- no dangling references, and no dependent manifests pinning this extension. Nothing else needs updating.

The new name matches the package's own convention, which is the strongest argument for it: IFlytekProtocolError, IFlytekFinalizeTimeoutError, and IFlytekConfigurationError all carry the Error suffix, as does PEP 8's recommendation. Sibling ASR/TTS extensions follow the same pattern (RetryError, ResponseError, RealtimeError). Good cleanup.

One note on the PR description: it says the rename aligns with "the reviewed conversational agent integration," but I could not find ReconnectLimitReachedError anywhere else in this repo, so that rationale is not observable from the merged tree (presumably it refers to an unmerged branch). The internal-consistency argument above is the one a future reader will be able to check.

The external error contract is unchanged, which matters more than the class name. vendor_code is still "reconnect_exhausted" in _send_framework_error, and test_extension.py:352 / test_vendor_error.py:33 still assert it. Operators following the Monitoring section of PRODUCTION_READINESS.md (which alerts on reconnect exhaustion) keep working with no changes. Renaming that string would have been a silent breaking change for dashboards; good that it was left alone.

collections.abc.Mapping is the right move. typing.Mapping has been deprecated since 3.9 and the package sets requires-python = ">=3.10". Runtime behavior is identical -- the nine isinstance(..., Mapping) checks in protocol.py work the same, since collections.abc.Mapping has always been the actual runtime class that typing.Mapping aliased. The import placement also fits the file's existing alphabetical-by-module ordering, and the next_delay reflow is just black reacting to the longer name against the 80-column limit.

Version bump is consistent across manifest.json and pyproject.toml (both 0.2.1), with PRODUCTION_READINESS.md updated to match. The remaining 0.2.0 mentions in README.md and docs/README.*.md are correctly left alone -- they describe when the params nesting change landed, not the current version.

Two things worth considering

1. A public symbol was renamed in a patch release. ReconnectLimitReached was importable as iflytek_asr_python.reconnect_manager.ReconnectLimitReached. Any out-of-tree code catching it breaks at import -- and an ImportError inside an addon surfaces as a load failure, not an obvious rename. In practice the exposure looks minimal: __init__.py only does from . import addon, so the class was never re-exported at package top level, and the runtime loads this as an addon rather than as a library. So a patch bump is defensible. If you want belt-and-braces, a one-line alias keeps it free:

class ReconnectLimitReachedError(RuntimeError):
    pass


# Deprecated alias, retained for 0.2.x compatibility. Remove in 0.3.0.
ReconnectLimitReached = ReconnectLimitReachedError

Your call -- reasonable to skip if the extension is understood to have no external Python consumers. Worth an explicit decision rather than an implicit one.

2. Version parity has no test. manifest.json and pyproject.toml versions are kept in sync by hand, and drift between them is exactly the kind of thing that goes unnoticed until packaging. tests/test_package_contract.py already asserts manifest/property.json consistency, so it is the natural home for a few more lines:

def test_manifest_and_pyproject_versions_match() -> None:
    manifest = json.loads((PACKAGE_ROOT / "manifest.json").read_text())
    pyproject = tomllib.loads(
        (PACKAGE_ROOT / "pyproject.toml").read_text(encoding="utf-8")
    )
    assert manifest["version"] == pyproject["project"]["version"]

tomllib is stdlib on 3.11+; on 3.10 (the declared floor) it needs tomli, so a regex or a sys.version_info guard may be simpler than adding a dependency. Optional, and arguably a separate PR.

Test coverage

Adequate for the change -- this is a rename, and every touched call site is exercised by an existing test that was updated in lockstep (test_reconnect_manager.py asserts the exception is raised once the limit is hit; test_vendor_error.py and test_extension.py assert the resulting vendor_info.code). No new behavior, so no new tests needed.

I was not able to execute the suite in this environment (ten_runtime availability and the test runner were not confirmed), so my assessment of the rename's completeness comes from static analysis -- grep across the repo plus reading each call site -- not from a green run. Please confirm CI is green, particularly test_reconnect_manager.py, test_vendor_error.py, and test_extension.py.

Security, performance

Nothing to flag. No change to error redaction (_sanitize_text still applied on the same paths), no new dependencies, no network or credential handling touched. Zero runtime cost -- a class rename and an equivalent import.

Looks good to merge once the alias question above is settled.

@dillonliang224
dillonliang224 force-pushed the fix/iflytek-asr-0.2.1-official branch from 06327dc to d212914 Compare September 2, 2026 06:47
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Tight, well-scoped release PR. The rename is applied consistently, the alias keeps 0.2.x callers working, and adding a version-parity test is a real improvement over a manual sync step. One issue is worth fixing before merge; the rest are minor.

Main issue: the tomllib fallback depends on an undeclared package

try:
    import tomllib
except ModuleNotFoundError:
    import tomli as tomllib

tomllib landed in Python 3.11, but this package declares requires-python = ">=3.10" and the repo CI setup-python steps pin "3.10" almost everywhere. On 3.10 the fallback is the live path, and I could not find tomli declared anywhere:

  • tests/pyproject.toml declares only pytest>=8.4,<9.0 and ten-runtime-python
  • tests/bin/bootstrap runs pip install -r requirements.txt, which lists only websockets, pydantic, typing_extensions
  • no tomli reference anywhere under the extension directory

It may resolve today because pytest has historically carried a tomli; python_version < "3.11" marker, but I could not verify that for the pinned pytest 8.4 range from this environment. Leaning on a transitive dep of the test runner is fragile either way. Note the blast radius: an unresolved tomli fails at collection time, so it takes down the two pre-existing tests in this file too, not just the new one.

Three ways out, cheapest first:

  1. Drop the TOML dep entirely, since only one scalar is needed:
import re
text = (PACKAGE_ROOT / "pyproject.toml").read_text(encoding="utf-8")
match = re.search(r"^version\s*=\s*\"([^\"]+)\"", text, re.MULTILINE)
assert match is not None, "pyproject.toml is missing a version"
assert manifest["version"] == match.group(1)

The [project] table is the only one with a bare version key here (target-version under [tool.black] will not match an anchored ^version), so this is unambiguous and adds nothing to install.

  1. Declare it explicitly: add tomli>=2.0; python_version < "3.11" to dependencies in tests/pyproject.toml.
  2. tomllib = pytest.importorskip("tomllib"), which is honest but silently skips the check on exactly the version CI runs, defeating the purpose.

I would take option 1.

The deprecated alias is not actually deprecated

ReconnectLimitReached = ReconnectLimitReachedError

The CHANGELOG calls this a deprecated alias, but nothing warns, so a 0.2.x caller gets no signal before the 0.3.0 removal. They will just break. If the deprecation should carry weight, a module-level __getattr__ that emits DeprecationWarning on attribute access would do it. That would mean test_legacy_reconnect_limit_exception_name_is_preserved needs pytest.warns(DeprecationWarning), which is arguably the better test anyway: it pins the deprecation contract rather than an is identity.

Separately, worth asking whether the alias is needed at all. I grepped for ReconnectLimitReached outside this extension directory and found zero consumers. If the rationale is users who vendored the package, keep it and say so in the comment; otherwise it is surface area with a scheduled cleanup cost.

Smaller notes

  • Correctness of the rename is fine. ReconnectLimitReachedError subclasses RuntimeError, and the except ReconnectLimitReachedError clause still precedes except Exception in the reconnect loop, so classification order is preserved. vendor_code = "reconnect_exhausted" is unaffected. No wire-format or behavioral change, which is the right shape for a patch release. Since the alias points at the same object, except ReconnectLimitReached in third-party code keeps working.
  • The collections.abc.Mapping migration is complete. protocol.py was the only module importing Mapping from typing; the remaining from typing import Any imports in config.py, client.py, and extension.py are correct, since Any has no collections.abc equivalent.
  • Import placement: the tomllib block sits after import pytest, putting a stdlib import below a third-party one. Trivial, but isort or ruff may reorder it if either runs on tests.
  • Version strings: manifest.json, pyproject.toml, and PRODUCTION_READINESS.md are updated. The remaining 0.2.0 mentions in README.md and docs/README.*.md are correct as-is, since they describe when params nesting was introduced rather than the current version. Good call leaving them.
  • The new test does not cover PRODUCTION_READINESS.md, which is now a third manual sync point that this PR had to bump by hand. If it drifts, nothing catches it. Optional follow-up, not this PR.
  • CHANGELOG date 2026-09-02 matches today, and the entry accurately describes all three changes.

Conventions

Commit message follows the fix(scope): lowercase, present tense convention from AGENTS.md, and the branch name matches type/short-description. No AI attribution present. Naming the exception with an Error suffix aligns with PEP 8.

Test coverage

Reasonable for a patch release: the rename is exercised through test_vendor_error.py, exhaustion still asserts via pytest.raises, the alias is pinned, and version parity is newly guarded. The main gap is that the version-parity test is the one most likely to fail for an unrelated reason (the import above) rather than for real drift.

Nothing here is a security or performance concern: no I/O, auth, or hot-path changes.

@dillonliang224
dillonliang224 force-pushed the fix/iflytek-asr-0.2.1-official branch from d212914 to 10ef597 Compare September 2, 2026 07:06
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review of fix(asr): align iFLYTEK integration for 0.2.1

Focused, well-scoped change. The rename is applied consistently, the compatibility alias is the right mechanism, and adding a version-drift test alongside the version bump is a good instinct. A few things worth addressing before merge.

ISSUES

  1. Locale-dependent decoding of manifest.json (new test, and pre-existing)

In tests/test_package_contract.py, pyproject.toml is read with an explicit encoding but manifest.json is not:

manifest = json.loads((PACKAGE_ROOT / "manifest.json").read_text())
pyproject = (PACKAGE_ROOT / "pyproject.toml").read_text(encoding="utf-8")

Path.read_text() without encoding uses the locale default. manifest.json contains CJK content in the display_name / description locale blocks, so under a non-UTF-8 locale (LANG=C, LC_ALL=POSIX, or a Windows cp1252 default) this raises UnicodeDecodeError rather than failing an assertion. Worth passing encoding="utf-8" here, and on the two pre-existing read_text() calls in the same file while you are in it — the inconsistency within a single new function is what makes it stand out.

  1. The version regex is not scoped to the [project] table

    re.search(r"^version\s*=\s*"([^\"]+)"", pyproject, re.MULTILINE)

This is correct today — ^version will not match target-version = ["py310"] because of the line anchor — but it takes the first top-level-looking version key anywhere in the file, not specifically the one in [project]. If a future [tool.*] table gains its own version, the test could silently assert against the wrong value. Either anchor the search to the [project] section, or use tomllib.loads(...)["project"]["version"] — though note tomllib is 3.11+ while this package declares requires-python = ">=3.10", so that needs a tomli fallback or a guard. The regex is a defensible choice given that constraint; a short comment saying so would help the next reader.

  1. The new test does not cover the version string it was written to protect

docs/PRODUCTION_READINESS.md was bumped to 0.2.1 by hand, and that is exactly the kind of drift the new test exists to prevent — but it only compares manifest.json against pyproject.toml. Consider extending it to assert the PRODUCTION_READINESS.md version line, and that CHANGELOG.md has a heading for the current version. (The 0.2.0 mentions in the README files are feature-history statements — "starting with version 0.2.0, params must be nested" — and are correct to leave alone.)

SMALLER POINTS

Compatibility alias. ReconnectLimitReached = ReconnectLimitReachedError as a plain alias rather than a subclass is the right call — except, isinstance, and identity all keep working, and there is no risk of the two diverging. The "remove in 0.3.0" comment is good. If you want out-of-tree callers to actually notice, a module-level __getattr__ (PEP 562) emitting a DeprecationWarning on attribute access would make the deprecation machine-visible; skip it if the alias is purely belt-and-braces, since every in-repo caller is already migrated.

Alias test. assert ReconnectLimitReached is ReconnectLimitReachedError looks tautological but does guard the case where someone later "fixes" the alias into a separate subclass and quietly breaks except semantics for callers. Fine to keep.

Naming. The Error suffix matches this extension own existing convention (IFlytekProtocolError, IFlytekConfigurationError, IFlytekFinalizeTimeoutError), so this is a net consistency win even though the wider ai_agents/ tree is mixed (several ...Exception classes elsewhere).

Semver. Renaming a public exception is technically breaking; the alias is what makes 0.2.1 defensible rather than 0.3.0. Flagging that the alias is load-bearing for the patch-level choice, which is another reason not to drop it early.

collections.abc.Mapping. Correct change — typing.Mapping has been deprecated since 3.9, subscripting collections.abc.Mapping is fine at >=3.10, and all the isinstance and annotation uses in protocol.py work unchanged. This was the last typing-ABC import in the extension.

COVERAGE, PERFORMANCE, SECURITY

Test coverage is adequate for the change: the renamed exception is exercised on both the next_delay limit path (test_reconnect_manager.py) and the reconnect_exhausted vendor-code path (test_vendor_error.py). No performance implications. No security surface touched — no credential handling, network behavior, or logging changed, and _send_framework_error classification semantics are identical before and after.

Nothing here is a blocker beyond the encoding fix, which is a one-line change.

Align reconnect error naming and Python compatibility with the reviewed
conversational agent integration.

Update extension metadata, release documentation, and affected tests for the
0.2.1 package.
@dillonliang224
dillonliang224 force-pushed the fix/iflytek-asr-0.2.1-official branch from 10ef597 to 078cae2 Compare September 2, 2026 07:24
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Review of fix(asr): align iFLYTEK integration for 0.2.1

Basis: I read the full diff plus the surrounding code (extension.py, reconnect_manager.py, protocol.py, the package tests, and the docs/README set). Lint and test tooling was not runnable in my environment, so everything below comes from static reading rather than from executing the suite.

Overall this is a clean, well-scoped patch release. The rename is mechanical and complete, and the new package-contract test guards a real class of drift.

WHAT LOOKS CORRECT

  • The rename is complete. All six call sites in extension.py (import, the except clause, the limit_error construction, and the isinstance branch in _send_framework_error) are updated, and both touched tests follow. No stale references remain outside the intentional alias.
  • No consumer-visible behavior change, which is what a patch bump should mean. vendor_code is still "reconnect_exhausted", and test_vendor_error.py still asserts codes [1000, -1000] with the same vendor_info payload. Branch ordering in _send_framework_error also remains correct: ReconnectLimitReachedError is a RuntimeError and is checked after IFlytekProtocolError, so no branch gets shadowed.
  • The collections.abc.Mapping migration is safe. protocol.py uses Mapping both as a subscripted annotation (lines 123, 214, 271, 293) and in isinstance checks (189, 215, 272); collections.abc.Mapping supports both under requires-python >=3.10. It was also the last module in this package still importing Mapping from typing, so the package is now internally consistent.
  • encoding="utf-8" on every read_text() is a real correctness fix, not just tidying: it removes a dependency on the ambient locale.
  • Tying manifest.json, pyproject.toml, PRODUCTION_READINESS.md, and CHANGELOG.md together in CI catches exactly the drift that tends to bite on the next release.

MAIN DISCUSSION POINT: IS THE LEGACY ALIAS NEEDED?

I grepped for importers of ReconnectLimitReached. The only ones are extension.py and this package own tests, all updated in this PR. init.py only does "from . import addon", so the exception was never part of the package export surface, and TEN extensions load through the addon registry rather than being imported as a library by third parties. So there does not appear to be a 0.2.x caller that the alias protects.

Given that 04_conventions.md calls out YAGNI explicitly, I would lean toward dropping the alias along with test_legacy_reconnect_limit_exception_name_is_preserved, and letting the rename stand on its own. That also removes the 0.3.0 cleanup obligation: the comment is the only thing tracking it today, and nothing fails if it is forgotten. If you are keeping it because something out of tree imports it, that is a fair call, but it would help to note where. In that case the current form (a plain identity alias, so "except ReconnectLimitReached" still catches) is the right implementation.

NOTES ON THE NEW CONTRACT TEST

  1. Regex TOML parsing is brittle. test_package_versions_match hand-rolls a [project] section parser. It works on the current file, but the section-terminator lookahead (?=^[|\Z) ends the body early at any line beginning with a bracket, for example a nested array element inside a future multi-line classifiers or dependencies value. tomllib is stdlib only from 3.11 and this package targets >=3.10, so an unconditional import is not safe; either guard the import or simplify to a single search for the first ^version\s*= and drop the section extraction entirely.

  2. The CHANGELOG assertion does not check position. re.search with re.MULTILINE passes as long as a matching heading exists anywhere in the file. A future bump that inserts its entry in the wrong place, or that forgets a new entry while an older matching heading survives, would still pass. Anchoring to the first "## " heading would make it a real "the top entry describes this version" check.

  3. The PRODUCTION_READINESS assertion couples to prose: it depends on the trailing period after the backticked version. A harmless doc rewording would fail CI with a confusing message. Asserting on the backticked version alone is more robust. Also consider adding assertion messages to the two comparison asserts; the two "is not None" checks have them and the comparisons do not.

DOCS

The README files under docs/ and the top-level README.md still say 0.2.0, but those are historical statements about when params nesting became mandatory, so they should not be bumped. PRODUCTION_READINESS.md is the only file carrying a "this checklist applies to version X" stamp, and that is the one updated. Correct as-is; flagging it so a later reviewer does not try to fix it.

BEFORE MERGE

Per 04_conventions.md, task lint is strict (a single warning is fatal) and the format and lint hooks do not run for a commit made from a host shell. Worth confirming:

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

One specific thing to watch: pylint const-naming-style=UPPER_CASE against the module-level ReconnectLimitReached assignment. Pylint normally infers a class alias and applies class-naming-style (PascalCase, which passes), but that is worth confirming rather than assuming, and it becomes moot if the alias is dropped. Also worth running the package suite directly, since tests/ is excluded from pylint and the new contract test is therefore not exercised by lint:

sudo docker exec ten_agent_dev bash -c "cd /app/agents/ten_packages/extension/iflytek_asr_python && python3 -m pytest tests -q"

Commit message and branch name both follow the conventions (fix(asr): prefix, fix/ branch, body wrapped under 100 columns), so no commitlint risk that I can see.

Nothing here blocks except the alias question, which is a design call rather than a defect.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants