fix(text-output): install smbprotocol at module level and fix anonymize crash#1178
Conversation
ksaurabhAparavi
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR removes concurrent GLiNER calls by running predictions sequentially, moves smbprotocol dependency installation to module import in endpoint.py (simplifying IGlobal.beginGlobal to a no-op), updates Endpoint.validateConfig to call self.connect() and widen exception logging, and expands text_output services.json to list SMB and anonymize parameters and explicit transform properties. ChangesNode stability and initialization fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete. |
…ze crash - Move depends() to module level in endpoint.py so smbprotocol is always installed regardless of OPEN_MODE. - Fix STATUS_HEAP_CORRUPTION in anonymize by removing the ThreadPoolExecutor from glinerRecognizer.predict(); GLiNER/PyTorch is not thread-safe and concurrent calls on the same model corrupt the native heap. Fixes rocketride-org#1165
ksaurabhAparavi
force-pushed
the
fix/RR-1165-anonymize-heap-and-smbprotocol
branch
from
a5e4e7c to
b9bd389
Compare
ksaurabhAparavi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/text_output/endpoint.py (1)
25-37:⚠️ Potential issue | 🔴 CriticalFix import order so
depends()runs before SMB imports innodes/src/nodes/text_output/endpoint.py.
depends(.../requirements.txt)is currently called at line 37, afterimport smbclient(line 27) andfrom smbprotocol.exceptions import SMBOSError(line 28), so the module can still fail to import before dependency bootstrap runs—move thedepends()call above the SMB imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nodes/src/nodes/text_output/endpoint.py` around lines 25 - 37, The bootstrap call depends(...) must run before importing SMB modules to ensure requirements are installed first; move the depends(os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt') invocation so it appears before any imports of smbclient and from smbprotocol.exceptions import SMBOSError (i.e., place the depends() call immediately after importing os and any other stdlib modules it needs), then keep the smbclient and SMBOSError imports after that change so that the smbclient import happens only after dependencies have been bootstrapped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nodes/src/nodes/text_output/endpoint.py`:
- Around line 77-83: The generic Exception handler is swallowing connection
probe failures from self.connect() during validation, allowing invalid SMB
configs to pass; update the except Exception block (around self.connect() in the
validation flow / method that calls self.connect()) to treat connection errors
as failures by logging them with engLib.error (not engLib.warning) and
re-raising the exception (or raising a ValidationError) so validateConfig
actually fails on probe errors instead of continuing silently. Ensure ValueError
handling remains as-is and only broad exceptions are escalated.
---
Outside diff comments:
In `@nodes/src/nodes/text_output/endpoint.py`:
- Around line 25-37: The bootstrap call depends(...) must run before importing
SMB modules to ensure requirements are installed first; move the
depends(os.path.dirname(os.path.realpath(__file__)) + '/requirements.txt')
invocation so it appears before any imports of smbclient and from
smbprotocol.exceptions import SMBOSError (i.e., place the depends() call
immediately after importing os and any other stdlib modules it needs), then keep
the smbclient and SMBOSError imports after that change so that the smbclient
import happens only after dependencies have been bootstrapped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24a69f13-b4c2-418b-80e6-da0043042ecd
📒 Files selected for processing (4)
nodes/src/nodes/anonymize/glinerRecognizer.pynodes/src/nodes/text_output/IGlobal.pynodes/src/nodes/text_output/endpoint.pynodes/src/nodes/text_output/services.json
| if not syntaxOnly: | ||
| # Do not try to connect as validation is run by Platform | ||
| # where SMB share may not be available | ||
| # self.connect() | ||
| pass | ||
| self.connect() | ||
|
|
||
| except ValueError as e: | ||
| engLib.error(e) | ||
| except Exception as e: | ||
| engLib.warning(e) |
There was a problem hiding this comment.
Do not swallow connection probe failures as warnings in validation.
Line 78 intentionally validates credentials/connectivity via self.connect(), but Line 82-83 catches all exceptions and logs only engLib.warning, which can let invalid SMB config pass validateConfig.
Suggested fix
except ValueError as e:
engLib.error(e)
except Exception as e:
- engLib.warning(e)
+ engLib.error(e)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not syntaxOnly: | |
| # Do not try to connect as validation is run by Platform | |
| # where SMB share may not be available | |
| # self.connect() | |
| pass | |
| self.connect() | |
| except ValueError as e: | |
| engLib.error(e) | |
| except Exception as e: | |
| engLib.warning(e) | |
| if not syntaxOnly: | |
| self.connect() | |
| except ValueError as e: | |
| engLib.error(e) | |
| except Exception as e: | |
| engLib.error(e) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@nodes/src/nodes/text_output/endpoint.py` around lines 77 - 83, The generic
Exception handler is swallowing connection probe failures from self.connect()
during validation, allowing invalid SMB configs to pass; update the except
Exception block (around self.connect() in the validation flow / method that
calls self.connect()) to treat connection errors as failures by logging them
with engLib.error (not engLib.warning) and re-raising the exception (or raising
a ValidationError) so validateConfig actually fails on probe errors instead of
continuing silently. Ensure ValueError handling remains as-is and only broad
exceptions are escalated.
1 participant
Summary
depends()to module level inendpoint.pysosmbprotocolinstalls regardless ofOPEN_MODE.STATUS_HEAP_CORRUPTIONin anonymize by removing theThreadPoolExecutorfromglinerRecognizer.predict()— GLiNER/PyTorch is not thread-safe and concurrent calls on the same model corrupt the native heap.Testing
./builder test) — relying on GitHub Actions; not runnable in the contributor's local shell (engine build / Maven / torch unavailable). Static checks (compile, no conflict markers) pass.Linked Issue
Fixes #1165
Summary by CodeRabbit
Bug Fixes
Refactoring
Configuration