feat(teams): port TeamsAuthCertificate config shape (#58)#65
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a Python Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the TeamsAuthCertificate dataclass to the Teams adapter to align with upstream configuration shapes, while maintaining the existing startup validation that prevents certificate-based authentication. The reviewer raised a concern regarding the inconsistency introduced by converting TeamsAuthCertificate to a dataclass while TeamsAuthFederated remains a TypedDict, noting that this change breaks support for dictionary literals in configuration. I recommend addressing this inconsistency by either reverting to TypedDict for TeamsAuthCertificate or migrating TeamsAuthFederated to a dataclass to ensure a uniform API.
| @dataclass | ||
| class TeamsAuthCertificate: |
There was a problem hiding this comment.
The conversion of TeamsAuthCertificate from a TypedDict to a dataclass introduces an inconsistency with other authentication configuration shapes in this file, such as TeamsAuthFederated (line 39), which remains a TypedDict. Additionally, this is a breaking change for any existing code that passes a dictionary literal to the certificate field of TeamsAdapterConfig. To maintain a consistent API and support dictionary literals (which is common for configuration shapes ported from TypeScript interfaces), consider keeping TeamsAuthCertificate as a TypedDict. If you retain the TypedDict structure, ensure it is defined with total=False if it will be used for mode detection when narrowing from a Mapping[str, Any], as per the project's typing standards. If a dataclass is preferred, TeamsAuthFederated should also be converted for consistency.
References
- When using cast to narrow a Mapping[str, Any] into a TypedDict, ensure that the TypedDict is a superset (total=False) that admits all possible keys, especially when performing duck-typing with .get() for mode detection.
There was a problem hiding this comment.
Noted — the TypedDict vs dataclass inconsistency with TeamsAuthFederated is deliberately deferred as bikeshed. No consumer usage to unify around, and TeamsAuthFederated isn't re-exported from __init__.py. Tracking as a potential follow-up; not blocking this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_teams_coverage.py (1)
1491-1503: Consolidate duplicate certificate smoke coverage.Line 1491-Line 1503 overlaps with
tests/test_teams_extended.py’s basic “raises ValidationError when certificate is set” check. Keep one smoke test and leave exact-message/minimal-shape assertions in one place to reduce duplication.As per coding guidelines: "No two tests should verify the same thing. Duplicates inflate test counts without catching more bugs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_teams_coverage.py` around lines 1491 - 1503, The test test_certificate_raises_validation_error in tests/test_teams_coverage.py duplicates the same certificate-validation check already present in tests/test_teams_extended.py; remove or replace this duplicate so only one smoke-level existence check remains here and keep the detailed "raises ValidationError when certificate is set" assertion (including exact message/minimal-shape checks) in tests/test_teams_extended.py; locate the duplicate by the test name test_certificate_raises_validation_error and the use of TeamsAdapter, TeamsAdapterConfig, and TeamsAuthCertificate and either delete the block from tests/test_teams_coverage.py or reduce it to a single minimal smoke assertion that does not repeat the full message/assertion logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_teams_coverage.py`:
- Around line 1491-1503: The test test_certificate_raises_validation_error in
tests/test_teams_coverage.py duplicates the same certificate-validation check
already present in tests/test_teams_extended.py; remove or replace this
duplicate so only one smoke-level existence check remains here and keep the
detailed "raises ValidationError when certificate is set" assertion (including
exact message/minimal-shape checks) in tests/test_teams_extended.py; locate the
duplicate by the test name test_certificate_raises_validation_error and the use
of TeamsAdapter, TeamsAdapterConfig, and TeamsAuthCertificate and either delete
the block from tests/test_teams_coverage.py or reduce it to a single minimal
smoke assertion that does not repeat the full message/assertion logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b71798a-6b4b-4361-9079-5cec642cb241
📒 Files selected for processing (6)
CHANGELOG.mdsrc/chat_sdk/adapters/teams/__init__.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/adapters/teams/types.pytests/test_teams_coverage.pytests/test_teams_extended.py
Addresses bot review feedback on PR #65 — the same startup-throw check existed in both test_teams_coverage.py and test_teams_extended.py. Kept the detailed version, removed the duplicate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review verdict: LGTM. Refreshed and reviewed latest head fb2b43c against main. No issues found in the TeamsAuthCertificate config shape/export changes. Verification: targeted Teams certificate/adapter coverage passed (175 passed across teams coverage/extended/adapter slices) and ruff passed on touched files. Formal GitHub approval is blocked because the authenticated account owns this PR. |
Ports the upstream `TeamsAuthCertificate` interface (adapter-teams/src/types.ts:3-10) as a Python dataclass and re-exports it from `chat_sdk.adapters.teams`. Updates the adapter's startup throw to match upstream (adapter-teams/src/config.ts:13-18) verbatim, including the camelCase `appPassword` reference. Pure config-shape parity — upstream does not implement cert auth either. Refs #58. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses bot review feedback on PR #65 — the same startup-throw check existed in both test_teams_coverage.py and test_teams_extended.py. Kept the detailed version, removed the duplicate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
patrick-chinchill
force-pushed
the
feat/teams-cert-58
branch
from
fb2b43c to
53b2948
Compare
1 participant
Summary
TeamsAuthCertificateinterface from upstream (adapter-teams/src/types.ts:3-10)certificatefield toTeamsAdapterConfigwith@deprecated-equivalent docstringcertificateis set — matchesadapter-teams/src/config.ts:13-18Refs #58. Queued under
## Unreleasedfor bundled0.4.26.2.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests