fix: lazy import cryptography — broken install without extras#4
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the version to 0.0.1a2 and moves the cryptography import into the encrypt_token and decrypt_token functions to handle it as an optional dependency. The review feedback suggests improving the user experience by wrapping these imports in try-except blocks to provide clear instructions on how to install the missing dependency.
| """ | ||
| import base64 | ||
|
|
||
| from cryptography.hazmat.primitives.ciphers.aead import AESGCM |
Contributor
There was a problem hiding this comment.
When handling optional dependencies like cryptography, it is a best practice to provide a descriptive error message that guides the user on how to resolve the missing dependency (e.g., by installing the crypto extra).
Suggested change
| from cryptography.hazmat.primitives.ciphers.aead import AESGCM | |
| try: | |
| from cryptography.hazmat.primitives.ciphers.aead import AESGCM | |
| except ImportError: | |
| raise ImportError( | |
| "The 'cryptography' package is required for Slack token encryption. " | |
| "Install it with 'pip install chat-sdk[crypto]'." | |
| ) from None |
| """ | ||
| import base64 | ||
|
|
||
| from cryptography.hazmat.primitives.ciphers.aead import AESGCM |
Contributor
There was a problem hiding this comment.
Providing a clear error message here, similar to encrypt_token, ensures that users are informed about the missing optional dependency when attempting to decrypt tokens.
Suggested change
| from cryptography.hazmat.primitives.ciphers.aead import AESGCM | |
| try: | |
| from cryptography.hazmat.primitives.ciphers.aead import AESGCM | |
| except ImportError: | |
| raise ImportError( | |
| "The 'cryptography' package is required for Slack token decryption. " | |
| "Install it with 'pip install chat-sdk[crypto]'." | |
| ) from None |
Closed
5 tasks
patrick-chinchill
pushed a commit
that referenced
this pull request
May 22, 2026
Independent review surfaced three actionable items: 1. **Math/escape ordering** (review #1) -- `_strip_math_regions` ran BEFORE `_strip_escape_sequences`, so input like `\$opener *unclosed text closer\$` had its two escaped `$`s paired as math, eating the `*` opener inside. Reordered: escape strip first, then math strip. Verified the original bug repro now correctly produces a closing `*`. 2. **Empty task items** (review #5) -- `r"^\[([ xX])\]\s+(.*)"` required at least one whitespace AFTER `]`, so `- [ ]` with no trailing content silently fell through to plain text. Loosened to `r"^\[([ xX])\](?:\s+(.*))?$"` -- trailing whitespace+content is now optional. `- [ ]` and `- [x]` (no trailing) both produce `listItem(checked=False/True, children=[])`. 3. **Strikethrough test strengthened** (review #7) -- the existing `test_escaped_tilde_does_not_form_strikethrough` only asserted the absence of a `delete` node. A buggy impl that dropped the tildes entirely would have passed. Added a content-shape assertion that `~~not strike~~` appears in the text leaves. Reviewer's #3 (the `r"a *b\* * c"` case) was investigated and determined NOT to be a bug -- the trailing `*` between two spaces is not a valid CommonMark closer (whitespace-flanked), so italic correctly stays open. Pre-fix behavior happened to balance by ignoring flanking; post-fix is CommonMark-correct. Pre-existing edge cases #4 (`\\*text*`) and #6 (link-text unescape implicit) are documented in the existing code; not blockers. Tests added: - `test_empty_unchecked_task_item_no_content` and `_checked_` variant - `test_remend_escaped_dollar_does_not_pair_with_unescaped_dollar` (the exact #1 repro) - `test_remend_escaped_dollar_does_not_create_phantom_math_region` - Strengthened `test_escaped_tilde_does_not_form_strikethrough` 3,625 pass / 1 pre-existing failure unrelated.
patrick-chinchill
pushed a commit
that referenced
this pull request
May 23, 2026
Closes the PR #101 review #4 finding (chatgpt-codex P2) properly rather than deferring. `\\*foo*` (escaped backslash + real italic) now parses correctly as text(`\`) + emphasis(`foo`); previously the fixed-width `(?<!\\)` lookbehind couldn't distinguish odd vs even backslash runs, so the `*` was wrongly treated as escaped. Approach: a `_protect_escapes` pre-pass runs at the start of `_parse_inline`, replacing every `\X` (X in `_ESCAPABLE_PUNCT`) with `<SENTINEL>X` where SENTINEL = U+0001 (a control codepoint that never legitimately appears in chat markdown). The inline regex lookbehinds now check for the sentinel instead of `\`, which trivially handles any number of preceding backslashes: - `\*` -> `<SENTINEL>*`. Regex lookbehind sees SENTINEL, skips. - `\\*` -> `<SENTINEL>\*`. The `*` is preceded by `\` (not SENTINEL), so the lookbehind passes and emphasis opens correctly. - `\\\*` -> `<SENTINEL>\<SENTINEL>*`. The `*` is preceded by SENTINEL, lookbehind skips. Correct (3 backslashes = escaped backslash + escaped asterisk = both literal). The complementary restore functions handle the two emission modes: - `_restore_escapes(text)`: SENTINEL pairs -> literal X (text leaves and image alt -- the backslash is dropped because the escape resolved to a literal char). - `_restore_escapes_as_literal_pair(text)`: SENTINEL pairs -> `\X` (inline code -- CommonMark preserves backslashes inside code spans). Changes: - New helpers `_protect_escapes`, `_restore_escapes`, `_restore_escapes_as_literal_pair`. ~50 LOC. - `_INLINE_PATTERNS` lookbehinds changed from `(?<!\\)` to `(?<!\x01)`; emphasis-star/_under add `\x01` to the exclusion sets; content groups for link/image use `(?:[^\]\x01]|\x01.)+?` to consume sentinel pairs without terminating early. - `_parse_inline` calls `_protect_escapes` on first entry and `_restore_escapes` at emission sites (text leaves, image alt, link URL/title). Inline code uses `_restore_escapes_as_literal_pair`. - Recursive calls into link text / emphasis content pass a `_already_protected=True` kwarg so the pre-pass doesn't run twice. - Removed `_unescape_punct` (no longer used); updated docstring references to point at `_restore_escapes` instead. - Removed the limitation comment added in 9a2027f. Tests added: - `test_escaped_backslash_followed_by_real_emphasis` (the reviewer's exact repro — `\\*real italic*` now produces text + emphasis) - `test_triple_backslash_before_star_is_fully_escaped` 3,636 pass / 1 pre-existing unrelated failure. No existing tests broke (escape behaviour preserved, inline code preservation preserved, image alt unescape preserved).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Installing
chat-sdkwithout[crypto]extra crashed on import becauseadapters/__init__.pyeagerly imports all adapters, which importsslack/crypto.py, which importscryptographyat module level. Fixed by making the import lazy inside the encrypt/decrypt functions. Also bumps to0.0.1a2for TestPyPI re-publish.