Audit fixes 2026-07-02: SIM-1..7 - #6
Merged
Merged
Conversation
Bare zip() at geogrid.py fetch_loop()/sample() and route.py's _fetch_depth_profile() paired external OpenTopoData results with requested points positionally, with no guard against a partial/malformed API response. A length mismatch would silently misalign results instead of raising, and the misaligned depths get persisted into the geogrid cache used for grounding/routing checks. Add strict=True at all zip() sites (plus test_heading_wander.py for consistency) so a mismatch raises loudly instead of corrupting data. In geogrid.py, materialize zip(..., strict=True) into a list before writing to self._elev so a mismatch is caught before any partial write, keeping the cache fully untouched on failure rather than half-populated. Add regression tests covering both the sync sample() path and the async fetch_loop() path with a short fetcher response, asserting the cache stays empty and misses stay queued for retry.
…yncClient SignalKWriter.connect() (reached via SignalKSink.open() -> SinkChain.open() -> runner.py) called urllib.request.urlopen(req, timeout=10) synchronously inside an async coroutine, freezing the event loop for up to 10s during the auth login POST. The rest of the codebase avoids blocking I/O in async paths (geogrid.py uses asyncio.to_thread; get_self_position() below already uses httpx.AsyncClient). Migrate the login POST to httpx.AsyncClient, preserving the 10s timeout and raise-on-failure semantics (non-2xx now raises httpx.HTTPStatusError instead of urllib.error.HTTPError; callers already catch broad Exception). Drop the now-unused urllib.request import. Add a regression test mocking httpx's transport + websockets.connect to verify connect() performs the login via httpx and never touches urllib.
pyproject.toml only set target-version/line-length, so ruff ran the default E4,E7,E9,F rule set — E501 (line-too-long) was never enforced despite a configured 100-char limit, and B905 (zip without strict=, the SIM-1 bug class) wasn't caught either. Add `[tool.ruff.lint] select = ["E","F","B","UP","SIM","C4"]` and fix the ~79 resulting violations: - `ruff check --fix` handled UP017 (datetime.UTC alias), UP035/UP006/ UP045/UP041/UP037/UP034 (typing-import/annotation modernization), SIM114 (merge if/elif arms with identical bodies), and the earlier B905 hit already fixed by SIM-1. - Manual fixes: E501 line wraps across metadata tables, config.py, runner.py, engine.py, etc.; SIM105 try/except/pass -> contextlib. suppress() in control.py, sinks/signalk.py, signalk_writer.py (x2), test_geogrid.py; SIM108 if/else -> ternary in synthetic_ais.py; SIM102 nested-if collapse in web/api.py; the remaining B905 in passage.py's `zip(leg, leg[1:])` pairwise walk switched to itertools.pairwise (strict=True is wrong there since the two sequences are intentionally offset by one). ruff check . is now clean under the broadened ruleset; full suite (190 tests) still green.
Second half of SIM-3: no static type checking existed despite the codebase having full type-hint coverage. Add mypy>=1.10 to dev deps and a `mypy` CI step (after ruff, before pytest). Config (non-strict start, per spec): mypy_path=src, explicit packages/package-bases + a py.typed marker for the src/ layout (yey.boats.simulator sits under implicit-namespace parents yey/, yey.boats/), ignore_missing_imports=true. Tighten iteratively later. Fix the 12 errors mypy surfaced: - config.py: rename shadowed loop variable (k, v reused across two for-loops made mypy infer a too-narrow type for the second use). - web/api.py: explicit dict[str, object]/dict[str, str] annotations for changes/errors (were inferred dict[str, str] from first use, rejecting the later int/bool values). - geogrid.py: assert self._cache_path is not None in _load() (only called when set); float() the already-None-checked corner values before the bilinear interpolation so mypy gets concrete floats. - signalk_writer.py: annotate self._ws: Any (websockets' connect() return type varies; was inferred as None-only from __init__). - runner.py: annotate ais_source: Any (AISStreamSource/ SyntheticAISSource are duck-typed, no shared base); assert writer.token is not None before use (true by this point — set by connect(), which the caller already awaited). - signalk_command.py: narrow token: str | None -> str (the only caller passes a non-None token by the time it gets here). mypy is green (0 errors); ruff still clean; full suite (190 tests) still passing.
Route.load_depth_profile()/_fetch_depth_profile() (per-leg OpenTopoData prefetch into a JSON cache) and Route.depth_at() (its reader) have zero production call sites — no runner/engine code calls load_depth_profile, and GeoGrid (lazy per-tick fetch + persistent cache) supersedes this whole path for depth-based routing/grounding checks. The only consumer was tests/test_route_depth.py. Confirmed via repo-wide grep before deleting: no other module reads Route._depth_profile or calls these methods. Delete Route.load_depth_profile(), Route.depth_at(), the module-level _fetch_depth_profile() helper, the now-unused _depth_profile dataclass field (and the sys/httpx/field imports that only served this cluster), tests/test_route_depth.py, resources.depth_cache_path() (orphaned with it — path helper for the same dead cache), and its lone test in test_resources.py. Suite green (188 tests, down from 190 — the 2 removed depth-profile tests); ruff and mypy still clean.
…lhost config.py defaults signalk_username/password to admin/admin (fine for localhost dev), but the GHCR publish pipeline makes shared/lab deployments plausible, and a stale default password would go unnoticed there. Add Settings.warn_if_insecure_credentials(): prints a startup WARNING to stderr when signalk_password is still "admin" and signalk_host is not "localhost", pointing the operator at SIGNALK_PASSWORD. Wired into Settings.from_env() (the actual startup path, via cli.build_settings) and Controller.apply_config() (live web-admin reconfigure can also point at a non-localhost host). Add tests covering: warns for non-localhost+default password, silent for localhost+default, silent for non-localhost+custom password, and warns via the from_env() startup path.
The job started the container detached (&), slept 45s, and printed a
message — it asserted nothing, so a container that crash-looped or
exited during startup still showed a green check.
Run the container with `docker run -d` (capturing its id) instead of
backgrounding via &, then after the 45s settle:
- fail unless `docker inspect -f '{{.State.Running}}'` reports true
- fail unless `docker logs` contains "[sink] active:" — the marker
SinkChain.open() prints once a sink is live (adapters/failover.py),
verified locally (venv run) to appear ~2s after start, well within
the existing 45s window, followed by steady per-tick output.
Always dump `docker logs` first for debugging, and clean up the
container on both the success and failure paths.
All deps were >=-only with no lockfile, so CI/dev reproducibility depended on whatever resolved at install time. Generate and commit uv.lock (uv lock; 33 packages resolved). Switch the CI `test` job to astral-sh/setup-uv + `uv sync --frozen --extra dev`, running ruff/mypy/pytest via `uv run` and the wheel build via `uv build --wheel` (replacing `pip install build && python -m build`). pyproject.toml dependency bounds stay loose for wheel consumers per spec — only CI/dev installs are pinned via the lockfile. Verified locally: `uv sync --frozen --extra dev` installs cleanly from the lockfile, `uv run pytest -q` (192 passed), `uv run ruff check .` and `uv run mypy` both clean, `uv build --wheel` succeeds. Document the uv-based dev setup in CONTRIBUTING.md (uv sync --frozen as the preferred path, plain pip as a fallback, `uv lock` to regenerate after a pyproject.toml dependency change).
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.
Summary
Fixes for findings SIM-1 through SIM-7 from the 2026-07-02 codebase audit
(
docs/audit-2026-07-02/specs/simulator.md). One commit per finding (SIM-3landed as two commits: ruff config+fixes, then mypy).
zip()withoutstrict=at 3 sites could silentlymisalign OpenTopoData elevation results with requested (lat,lon) cells,
corrupting the persisted geogrid depth cache used for grounding/routing.
Added
strict=Trueeverywhere, made the geogrid writes atomic (materializethe zip into a list before writing, so a mismatch can't leave a
partially-written cache), and added regression tests for both the sync
sample()and asyncfetch_loop()paths.SignalKWriter.connect()'s auth login usedurllib.request.urlopen(..., timeout=10)synchronously inside anasync def, freezing the event loop for up to 10s. Migrated tohttpx.AsyncClient,preserving the timeout and raise-on-failure semantics. Added a regression
test mocking httpx's transport +
websockets.connect.E4,E7,E9,Frule set (noE501, no B905). Broadened to
select = ["E","F","B","UP","SIM","C4"]andfixed the ~79 resulting violations. Added mypy (non-strict start) to dev
deps + a CI step, added a
py.typedmarker +[tool.mypy]config for thesrc/ layout, and fixed the 12 surfaced type errors.
Route.load_depth_profile()/_fetch_depth_profile()/Route.depth_at()prefetch cluster (zeroproduction callers — GeoGrid supersedes it), its test file, and the
orphaned
resources.depth_cache_path().Settings.warn_if_insecure_credentials()— a loudstartup WARNING when
signalk_passwordis still"admin"andsignalk_host != "localhost", wired intoSettings.from_env()andController.apply_config(). Added tests for all four combinations.actually running (
docker inspect ... State.Running) and reached steadystate (
[sink] active:marker indocker logs) instead of just sleepingand printing a message.
uv.lock(33 packages) and switched the CItestjob touv sync --frozen+uv runfor ruff/mypy/pytest, plusuv build --wheel. Documented the uv-based dev setup in CONTRIBUTING.md.Verification
Docker-smoke logic was verified locally by running the actual CLI
(
yey-boats-sim --sink stdout --no-failover) outside Docker and confirmingthe
[sink] active: stdoutmarker appears ~2s after start, well within thejob's 45s window — the sandboxed environment doesn't have a running Docker
daemon to exercise the CI job's
docker build/docker runend-to-end.Test plan
pytest -q— 192/192 passingruff check .— clean under broadened rulesetmypy— clean (non-strict start)uv sync --frozen --extra dev/uv build --wheel— verified locallybe exercised locally without a Docker daemon)