Browse Source

Ask for typed access over dynamic lookups

Record what the upload bug taught: cast(Any, ...) plus a getattr default
turns a wrong field name into an empty string, so the failure surfaces as
a message blaming the server instead of an AttributeError at the call.
chattolib is fully typed — read the fields directly and let tests touch
the real types.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Paul-Dieter Klumpp 1 week ago
parent
commit
0257dbf854
1 changed files with 32 additions and 0 deletions
  1. 32 0
      AGENTS.md

+ 32 - 0
AGENTS.md

@@ -15,6 +15,38 @@ PYTHONPATH=/tmp/hermes uv run --with pyyaml pytest -q
 `gateway.config` needs `pyyaml`, which is not among our dev dependencies. If a
 checkout already exists somewhere, point at that path instead of cloning again.
 
+## Typed access
+
+**Work against the declared types. Do not reach for values dynamically when a
+typed attribute exists.**
+
+`chattolib` ships dataclasses with `py.typed` for everything it returns —
+`Message`, `Asset`, `AssetUpload`, `Room`, `User`. Read their fields directly, so
+a wrong name is a loud `AttributeError` at the first call and a type checker can
+see it before that:
+
+```python
+upload_id = upload.upload_id            # yes
+upload_id = str(getattr(cast(Any, upload), "id", ""))   # no
+```
+
+The second form cost us every single file upload. `AssetUpload` has no `id`
+field — it is called `upload_id` — but `cast(Any, ...)` disables the check and
+the `""` default turns the mismatch into an empty string. The result was an
+error message blaming the server (`CreateUpload returned no upload ID`) for a
+name we got wrong ourselves, and it survived in production because it never
+raised.
+
+So: no `cast(Any, ...)` on a value whose type is known, and no `getattr` with a
+fallback default to paper over a field you are not sure about — look the field up
+in `vendor/common/chattolib/types.py` instead. `getattr` is fine for genuinely
+optional or duck-typed things (a handler that may not be set, a payload from an
+untyped source), not for dodging a type.
+
+Tests should exercise real library types too, not mocks shaped like them. The
+upload bug was invisible because the tests replaced `_upload_asset` wholesale
+with an `AsyncMock`, so no test ever touched a real `AssetUpload`.
+
 ## Capabilities
 
 **When you extend what the adapter can do, keep the startup banner current.**