Skip to content

fix: replace assert with runtime guard in Slack block_kit _rich_text_list_block - #64071

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/slack-block-kit-assert
Closed

fix: replace assert with runtime guard in Slack block_kit _rich_text_list_block#64071
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/slack-block-kit-assert

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Replace assert cur is not None in plugins/platforms/slack/block_kit.py _rich_text_list_block() with an explicit if cur is None: continue guard.

Why

assert statements are stripped by python -O, silently removing the invariant check. While this particular assert is technically unreachable (the first loop iteration always enters the if key != cur_key block and sets cur), using assert for runtime invariant checking in production code is fragile.

Follows the same pattern as PRs #56736, #56866, #61983, #62001, #62006, #62659, #64063.

Test Plan

  • Existing Slack tests pass
  • python -O -c "from plugins.platforms.slack.block_kit import _rich_text_list_block" succeeds

…uilder

assert statements are stripped when Python runs with -O flag. Replace
the assert cur is not None in _rich_text_list_block() with an explicit
if/continue guard. The assert is technically unreachable (first loop
iteration always sets cur), but using assert for invariant checking in
production code is fragile under optimization.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins platform/slack Slack app adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages P3 Low — cosmetic, nice to have labels Jul 14, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

This PR replaces an assert with a runtime guard in Slack block_kit _rich_text_list_block. Small, targeted fix.

Please verify:

  • The runtime guard handles the edge case that the assert was catching
  • Error message is user-friendly

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (token read-only)

PR 64071: fix: replace assert with runtime guard in Slack block_kit _rich_text_list_block

Correctness

  • Replaces assert cur is not None (which disappears with python -O) with an explicit runtime guard (if cur is None: continue).
  • Maintains the same logical protection while being optimization-safe.
  • Comment explains the intent clearly.

No issues found.


Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment (token read-only)

PR 64071: fix: replace assert with runtime guard in Slack block_kit _rich_text_list_block

Correctness

  • Replaces assert cur is not None (which disappears with python -O) with an explicit runtime guard (if cur is None: continue).
  • Maintains the same logical protection while being optimization-safe.
  • Comment explains the intent clearly.

No issues found.


Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

Changes

Slack block_kit: assert cur is not None replaced with explicit guard + continue. Prevents silent drop under python -O.

Assessment

  • Defensive fix. The comment explains the rationale well.
  • No debug artifacts.

Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused defensive-cleanup proposal.

Problems

  • cur is already guaranteed non-None on every loop iteration: cur_key starts as None (plugins/platforms/slack/block_kit.py:196), while key is always a tuple (:198), so the construction at :199-207 runs first. The only parser call site also appends parsed items before calling _list_block() (:442-477).
  • If this invariant were ever violated, the new continue would silently omit a Slack list item. The current optimized-mode path would instead fail at cur["elements"] (:209), exposing the broken internal contract.

Suggested changes

  • Keep the assertion or omit this change rather than adding a silent content-dropping recovery branch.

Automated hermes-sweeper review.

elements.append(cur)
cur_key = key
assert cur is not None
if cur is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cur is structurally assigned before this point: key is always a tuple while cur_key begins as None. If a future refactor broke that invariant, continue would silently drop this list item; please retain the assertion or avoid adding this no-op recovery branch.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 16, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Merged via #69317 — your commit was cherry-picked onto current main with your authorship preserved in git history: your assert-to-runtime-guard fix was cherry-picked directly.

Thanks for the contribution!

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

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/slack Slack app adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants