Skip to content

fix(lint): unblock develop's lint pipeline#1092

Merged
Brutus5000 merged 2 commits into
developfrom
fix/lint-toolchain-drift
Jun 11, 2026
Merged

fix(lint): unblock develop's lint pipeline#1092
Brutus5000 merged 2 commits into
developfrom
fix/lint-toolchain-drift

Conversation

@Brutus5000

@Brutus5000 Brutus5000 commented Jun 11, 2026

Copy link
Copy Markdown
Member

Summary

Two Lint workflow steps have been red on develop for months even though the source code hasn't changed since the last green run (commit e36de71a, 2026-01-29). Both reds are toolchain drift via unpinned/loosely-pinned external tools, not real issues in the repo:

  • pipenv-verify — workflow does pip install pipenv (no version pin). pipenv verify recomputes a SHA-256 of Pipfile and compares it to _meta.hash.sha256 in Pipfile.lock. The current pipenv normalizes the Pipfile differently from the version that produced the lock, so the hashes diverge even though Pipfile is unchanged. Fixed by regenerating Pipfile.lock under current pipenv. Dependency versions get bumped to current point releases as a side effect (e.g. sqlalchemy 2.0.43 → 2.0.50, aiohttp 3.12.15 → 3.14.1, cryptography 46.0.1 → 48.0.1).
  • mypy — workflow uses jpetrucciani/mypy-check@master (unpinned action, latest mypy at run time). QDataStreamProtocol.pack_message builds a bytearray and passes it to pack_block(block: bytes). Older mypy implicitly accepted that; current mypy raises [arg-type]. Fixed by widening the signature to bytes | bytearray and casting back to bytes at return so the declared return type still holds for both inputs.

Greenlet follow-up

The first lock regeneration silently dropped greenlet, which broke the full Test workflow (ValueError: the greenlet library is required to use this function. No module named 'greenlet') on every test that opens an async DB session. Root cause: older pipenv eagerly resolved SQLAlchemy's conditional greenlet marker; current pipenv only pulls it in when the asyncio extra is requested. Fixed by declaring the dependency explicitly:

sqlalchemy = {version = ">=2.0.0", extras = ["asyncio"]}

so the lock carries greenlet regardless of how pipenv interprets the underlying marker. Verified locally: SQLAlchemy async dialect now loads with no greenlet error.

Verification

  • pipenv verify → "Pipfile.lock is up-to-date."
  • mypy --disable-error-code=annotation-unchecked --follow-untyped-imports --strict-equality --warn-redundant-casts --warn-unused-ignores server/ main.py → "Success: no issues found in 75 source files"
  • pytest tests/unit_tests/test_protocol.py → 16 passed
  • Integration tests run without the greenlet ValueError

Test plan

  • Lint workflow goes fully green (isort, flake8, mypy, pipenv-verify)
  • Test workflow stays green (no greenlet error, no behavior change from pack_block widening)

Follow-up suggestion (out of scope)

Both reds will recur as the tools keep moving. Worth pinning pipenv in pipenv-verify and replacing jpetrucciani/mypy-check@master with a tag, so toolchain bumps land via explicit PR rather than silently breaking CI on unrelated branches.

🤖 Generated with Claude Code

Two pre-existing lint reds on develop, caused by toolchain drift since
the last green run (2026-01-29). Develop's source is otherwise byte-
identical to the last passing commit.

- pipenv-verify: regenerate Pipfile.lock under current pipenv. The lock
  was produced by an older pipenv whose Pipfile-hashing differs from
  what `pipenv verify` computes today, even though Pipfile is unchanged.
  Lock now matches `pipenv verify` cleanly. Dependency versions are
  bumped to current point releases as a side effect.
- mypy: widen `QDataStreamProtocol.pack_block` to accept `bytes |
  bytearray`. Newer mypy releases (pulled via the unpinned
  jpetrucciani/mypy-check@master action) no longer accept passing a
  `bytearray` to a `bytes` parameter, and `pack_message` builds its
  argument as a `bytearray`. Cast back to `bytes` at return so the
  declared return type still holds for both inputs.

`pipenv verify`, `mypy server/ main.py`, and `tests/unit_tests/test_protocol.py`
are clean locally.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The pack_block method in the QDataStream protocol handler now accepts both bytes and bytearray input types. The implementation converts bytearray to bytes before prefixing the payload with a 4-byte big-endian length field.

Changes

pack_block input type flexibility

Layer / File(s) Summary
pack_block signature and implementation
server/protocol/qdatastream.py
pack_block now accepts bytes | bytearray input and converts bytearray to bytes before packing the length prefix.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A rabbit hops in to expand the way,
Bytes and bytearray can now play,
No conversion needed by the caller's hand,
One simple change across the land! 🐰✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'fix(lint): unblock develop's lint pipeline' is vague and does not clearly indicate the specific technical changes made. While it references 'lint', it does not specify that this involves updating QDataStreamProtocol to accept bytearray or fixing pipenv/mypy issues. Consider a more specific title like 'fix: resolve mypy and pipenv lint pipeline issues' or 'fix: widen QDataStreamProtocol.pack_block to accept bytearray' to better communicate the actual changes to the codebase.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/lint-toolchain-drift

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

The regenerated lock from the previous commit dropped `greenlet`. Older
pipenv resolved SQLAlchemy's conditional `greenlet` marker eagerly;
current pipenv only includes it when the `asyncio` extra is requested.
Without it, every test that touches an async DB session fails with
`ValueError: the greenlet library is required to use this function`.

Declare the dependency explicitly via `sqlalchemy[asyncio]` so the
lock carries `greenlet` regardless of how pipenv interprets the
underlying marker.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Brutus5000 Brutus5000 merged commit 3c500c0 into develop Jun 11, 2026
10 checks passed
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.

1 participant