Code quality round: lint and type gates adopted, wire contract typed end to end - #16
Merged
Conversation
Eric's IDE review round, finding 1 (Ruff I001 import sorting) - resolved by adopting the rulebook, not just the one line: ruff config in pyproject (line-length 100, py310, isort rules; agent-skills excluded - that layer is tool-managed by agent-memory and style fixes for it belong upstream), ruff added to the dev extra. - 103 auto-fixes: canonical import blocks, Optional[X] -> X | None and other py310+ modernizations (ruff 0.16 defaults) - 3 nested async-with blocks in tests combined (SIM117) - 2 deliberate blanket catches in the host justified with noqa + rationale (any handler failure must become an envelope reply; log-only async sink) - PYI034 (-> Self) suppressed with a revisit note: typing.Self needs python >= 3.11, the package floors at 3.10 - .idea/ added to .gitignore ruff check clean; tests 30/30. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round, finding 2. The (headers, body) signature is the function contract (TypedLambdaFunction mirror) and stays; a handler that does not need headers underscore-prefixes the parameter per Python convention - the demo now teaches that idiom, while declarative_echo keeps 'headers' because it uses it. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round, findings 3+4 (reportMissingTypeArgument on bare dict; untyped body parameter) - resolved by adopting the type checker and typing the contract, not just the flagged lines: - new exported Body type: the payload universe of the standard wire format as a recursive alias (None | bool | int | float | str | bytes | list[Body] | dict[str, Body]); Handler is now Callable[[dict[str, str], Body], Any] and both are exported - demo and README teach the typed signature - every bare generic parameterized (dict[str, str] headers, dict[str, Any] config, dict[str, Semaphore], Task[EventEnvelope], Token[TraceInfo | None]) - executor call restructured so the contextvars relay is typeable; msgpack's bytes | None returns asserted/cast at the boundaries - genuine test fixes at standard level: optional trace access guarded, bound port discovered via the public runner.addresses (not site._server), stack assertion narrowed - basedpyright pinned in pyproject: standard mode + reportMissingParameterType as error on the public surface (src/examples); tests stay at standard via an executionEnvironments override; agent-skills excluded (tool-managed) Gates: basedpyright 0 errors, ruff clean, tests 30/30. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's ruling: tests are held to the same bar - the per-directory exemption is removed and every test signature is annotated (handlers take (dict[str, str], Body), fixtures yield AsyncIterator[str], pytest fixtures typed tmp_path: Path / monkeypatch: pytest.MonkeyPatch). The Body annotation forced honest narrowing where a handler pokes into the payload (isinstance(body, dict) before .get). Vestigial fixture parameter removed. Gates: basedpyright 0 errors repo-wide, ruff clean, tests 30/30. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round: argparse Namespace attributes are untyped Any and the checker's os.path overloads smear str | bytes into the path handling. The parsed arguments are now pinned to their real types where they leave the parser (app: str, port: int | None, host: str, config: str | None), so dirname/join/load_config all check cleanly. Gates: ruff clean, basedpyright 0 errors, tests 30/30, CLI usage smoke ok. Co-Authored-By: Claude Code <noreply@anthropic.com>
… builder Eric's IDE review round, client.py findings: - _get_session had the async keyword without any await, and its return type narrowed poorly (ClientSession | None): now a plain method using local narrowing; it is only ever called from the running event loop inside request/send - _build_event never touched self: now a module-level helper - declined, with rationale: "aiohttp not in project requirements" is an IDE environment issue (declared in pyproject [project.dependencies]; point the IDE interpreter at .venv); "traceparent" is the literal W3C header name, not a typo Gates: ruff clean, basedpyright 0 errors, tests 30/30. Co-Authored-By: Claude Code <noreply@anthropic.com>
The project venv is uv-managed (uv venv + uv pip install -e '.[dev]'); document it with the three gates (pytest, ruff, basedpyright) and the PyCharm wiring - interpreter type uv on the project .venv, and the Package-requirements-file setting pointed at pyproject.toml so the requirements inspection reads [project.dependencies]. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round, config.py findings:
- redundant '\}' escape removed from the ${...} reference regex (closing
brace needs no escape outside a character class)
- the substitution callback's local no longer shadows the outer 'resolved'
- app_config()/load_config() use local narrowing so their AppConfig return
type checks cleanly (global-name narrowing is weak, same pattern as the
client session accessor)
- declined: "yaml not in project requirements" is the same IDE
requirements-source setting as aiohttp (PyYAML is declared in pyproject)
Gates: ruff clean, basedpyright 0 errors, tests 30/30.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round, envelope.py findings (all six): - from_map decodes each optional field via a raw_* local and an if-statement guard: statement-level narrowing that every checker follows (the ternary form defeated PyCharm's), the id field is no longer looked up twice, and constructor defaults carry the absent cases - wire semantics unchanged (absent == nil, defaults preserved), pinned by the golden-vector tests - raw_annotations avoids shadowing the 'annotations' name that 'from __future__ import annotations' introduces at module scope - __repr__ formats the optional 'to' explicitly instead of !r on None Gates: ruff clean, basedpyright 0 errors, tests 30/30 (incl. conformance vectors - decode behavior proven unchanged). Co-Authored-By: Claude Code <noreply@anthropic.com>
Follow-up to the envelope decode cleanup: PyCharm does not narrow 'Any | None' through an 'is not None' guard, so the four conversion locals (raw_id/raw_status/raw_exec_time/raw_round_trip) now declare what they really are - Any, a msgpack-decoded value - at the source. Runtime guards unchanged; ruff, basedpyright and PyCharm all agree. Gates: ruff clean, basedpyright 0 errors, tests 30/30. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round, server.py findings: - except (CompactFormatError, ValueError) collapsed to ValueError - CompactFormatError IS a ValueError, the tuple was redundant - the async log-only sink uses log.exception() - and chasing that smell uncovered a real gap: both custom formatters ignored exc_info, so any traceback-carrying log would have silently dropped its traceback. Text formatter appends the formatted traceback; JSON formatter carries an "exception" field. Pinned by two new formatter tests (engine pattern + traceback rendering). - _log_async_outcome and handle_health are @staticmethod (no self) - declined: removing async from handle_health - aiohttp handlers must be coroutines, the keyword is the framework contract (comment added) Gates: ruff clean, basedpyright 0 errors, tests 32/32. Co-Authored-By: Claude Code <noreply@anthropic.com>
…ync sink Eric's IDE review round: PyCharm's "Too broad exception clause" on the log-only sink. The catch is deliberate - a drop-n-forget event has no requester to answer, so anything a task raises is logged with its traceback and never re-raised. Marked with PyCharm's suppression id (PyBroadException), rationale comment kept. (No ruff marker needed - BLE001 only fires on a bound exception.) Gates: ruff clean, basedpyright 0 errors, tests 32/32. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round: the protected-member warnings (_set_trace / _reset_trace imported by tests) exposed a missing public API. New trace_context(trace_id, trace_path, cid=None) context manager establishes a trace context around a block - for batch jobs and tests whose PostOffice calls should carry a trace - mirroring the node package's exported runWithTrace. Exported and documented in the README. Also: unused handler parameters across the test registries take the underscore convention (same contract ruling as the demo). Gates: ruff clean, basedpyright 0 errors, tests 32/32. Co-Authored-By: Claude Code <noreply@anthropic.com>
… raises Eric's IDE review round, test_envelope.py: - the vectors path uses pathlib (Path(__file__).parent / ...) - os.path overloads smeared str | bytes through dirname/join - the composite 'assert standard and compact' split into two assertions - the compact-vector pytest.raises block narrowed to exactly one throwing call (b64decode moved out) so a decode error cannot masquerade as the expected CompactFormatError - declined: 'packb' is the msgpack API name, not a typo Gates: ruff clean, basedpyright 0 errors, tests 32/32. Co-Authored-By: Claude Code <noreply@anthropic.com>
Eric's IDE review round, SonarQube for IDE finding in test_server.py: the composite 'is not None and >= 0' assertion split into two - a failure now names which half broke, and the second assert type-narrows exec_time. The sweep confirms no other composite assertions remain in the project. Gates: ruff clean, basedpyright 0 errors, tests 32/32. Co-Authored-By: Claude Code <noreply@anthropic.com>
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.
What
Eric's screenshot-driven IDE/Sonar review round, resolved as durable tooling rather than
one-line fixes — 15 commits:
103 auto-fixes incl.
Optional[X]->X | None) and basedpyright (standard mode +reportMissingParameterTypeas error, unit tests included - every test signature isannotated).
agent-skills/is excluded from both: that layer is tool-managed byagent-memory and style fixes for it belong upstream.
.idea/gitignored. README gaineda Development section (uv environment recipe, the three gates, PyCharm wiring).
Bodytype (theMsgPack payload universe) and
Handler = Callable[[dict[str, str], Body], Any]; everybare generic parameterized; demo and README teach the typed signature; unused contract
parameters take the underscore convention.
tracebacks (
exc_infoignored) - fixed in both text and JSON form and pinned by newtests (suite 30 -> 32); the protected-member warnings exposed a missing public API -
new
trace_context()context manager (the noderunWithTracetwin) so tests and batchcallers stop importing
_set_trace/_reset_trace;pytest.raisesblocks narrowed toa single throwing call; pathlib replaces
os.pathoverload smearing; checker-prooflocal-narrowing in the singleton accessors, session accessor, and envelope decode;
redundant exception-class catch collapsed; composite assertions split (Sonar S9073).
asyncstays on the aiohttp health handler (handlers mustbe coroutines - framework contract); the broad catch in the log-only async sink is a
justified suppression (
PyBroadException+ rationale);traceparent/packbare realnames, not typos.
Why
The wrappers are reference implementations for the polyglot initiative, so the first IDE
findings were answered with a shared rulebook - one config that the IDE, the CLI and the
future CI all read - rather than chasing warnings one by one. Typing the function contract
(
Body,Handler,dict[str, str]headers) turns the wire format's documentation intochecked code, and the round surfaced two genuine defects (dropped tracebacks, a missing
public trace API) that screenshots of style warnings would never have justified on their
own.
Verification
conformance vectors included - decode behavior proven unchanged through the refactors).
Co-Authored-By: Claude Code noreply@anthropic.com