Mikhail Putilovskij
c9072d51ea
7 documents covering stack, integrations, architecture, structure, conventions, testing, and concerns.
235 lines
16 KiB
Markdown
235 lines
16 KiB
Markdown
# Codebase Concerns
|
||
|
||
**Analysis Date:** 2026-04-01
|
||
|
||
---
|
||
|
||
## Tech Debt
|
||
|
||
### Telegram adapter not merged to main
|
||
|
||
- Issue: The entire `adapter/telegram/` directory exists only in the `feat/telegram-adapter` branch (worktree at `.worktrees/telegram/`). `main` has no Telegram adapter at all.
|
||
- Files: `.worktrees/telegram/adapter/telegram/` and remote branch `origin/feat/telegram-adapter`
|
||
- Impact: Running `python -m adapter.telegram.bot` from `main` fails with ImportError. Tests referencing `adapter/telegram/` (e.g., `tests/adapter/test_forum_db.py`) only exist in the worktree and are absent from `main`.
|
||
- Fix approach: Merge `feat/telegram-adapter` into `main` after final manual QA pass. The branch is ahead of main by 5 commits (`a1b7a14` being the most recent).
|
||
|
||
### Divergent core/handlers between main and feat/telegram-adapter
|
||
|
||
- Issue: `feat/telegram-adapter` removed platform-awareness from `core/handlers/chat.py` and `core/handlers/message.py` — the `_command()` and `_start_command()` helpers that format Matrix `!cmd` vs Telegram `/cmd` prompts were deleted. The branch hardcodes `/start` everywhere.
|
||
- Files: `core/handlers/chat.py`, `core/handlers/message.py` (differ between branches)
|
||
- Impact: If the Matrix adapter relies on these platform-aware helpers being in `main`'s version of core, merging `feat/telegram-adapter` will break Matrix `!start` prompt text for unauthenticated users.
|
||
- Fix approach: Before merging, decide which version of `core/handlers/` is canonical. The Matrix adapter in `main` currently passes because `main` still has the platform-aware helpers.
|
||
|
||
### SQLiteStore uses blocking I/O in async context
|
||
|
||
- Issue: `core/store.py` `SQLiteStore` methods are declared `async` but perform synchronous blocking `sqlite3.connect()` calls without `asyncio.to_thread` or `aiosqlite`.
|
||
- Files: `core/store.py` lines 46–73
|
||
- Impact: Each database call blocks the asyncio event loop. Under any concurrent load (e.g., two Matrix users sending messages simultaneously) this will cause visible latency spikes and potential event loop starvation.
|
||
- Fix approach: Replace `sqlite3` calls with `aiosqlite` or wrap each call in `asyncio.to_thread()`.
|
||
|
||
### Telegram adapter has its own separate SQLite database layer
|
||
|
||
- Issue: `adapter/telegram/db.py` is a fully independent SQLite database (file: `lambda_bot.db`) with its own schema (`tg_users`, `chats`). Meanwhile, `core/store.py` has `SQLiteStore` with a KV schema (`lambda_matrix.db` for Matrix). The two stores are incompatible and do not share data.
|
||
- Files: `.worktrees/telegram/adapter/telegram/db.py`, `core/store.py`
|
||
- Impact: There is no unified storage layer. Chat state is split across two databases. A user's Telegram chats cannot be seen from Matrix and vice versa (even conceptually). Violates the "single core" architecture principle from CLAUDE.md.
|
||
- Fix approach: This is a fundamental design gap. Either extend `StateStore` to support the Telegram-specific data model, or accept separate stores as intentional for the prototype stage and document the constraint.
|
||
|
||
### MockPlatformClient hardcoded throughout — no production path wired
|
||
|
||
- Issue: Both `adapter/matrix/bot.py` and `.worktrees/telegram/adapter/telegram/bot.py` instantiate `MockPlatformClient()` directly. `PLATFORM_MODE` is defined in `.env.example` but is never read or acted upon anywhere in the codebase.
|
||
- Files: `adapter/matrix/bot.py` line 71, `sdk/mock.py`, `sdk/interface.py`
|
||
- Impact: There is no runtime switch to connect a real SDK. Switching to production requires code changes, not configuration.
|
||
- Fix approach: Add a factory function in `sdk/` that reads `PLATFORM_MODE` and returns either `MockPlatformClient` or a real `PlatformClient`. Both bot entrypoints should use this factory.
|
||
|
||
### MatrixRuntime type annotation leaks MockPlatformClient
|
||
|
||
- Issue: `adapter/matrix/bot.py` `MatrixRuntime.platform` is typed as `MockPlatformClient` (not `PlatformClient`). `build_event_dispatcher` and `build_runtime` signatures also use `MockPlatformClient` as the parameter type.
|
||
- Files: `adapter/matrix/bot.py` lines 46, 54, 67
|
||
- Impact: The isolation promise ("replace only `sdk/mock.py` when real SDK arrives") is broken — the bot layer is coupled to the mock concrete type, not the Protocol.
|
||
- Fix approach: Change type annotations to `PlatformClient` from `sdk.interface`.
|
||
|
||
---
|
||
|
||
## Known Bugs / Open Issues
|
||
|
||
### Telegram forum: global commands visible inside topic context
|
||
|
||
- Issue: Telegram shows the full bot command menu (including `/chats`, `/new`, `/settings`) even when the user is inside a forum topic. The code blocks `switch` and `new_chat` callbacks inside topics but the commands themselves still appear in the UI.
|
||
- Files: `.worktrees/telegram/adapter/telegram/handlers/forum.py`, `.worktrees/telegram/adapter/telegram/bot.py`
|
||
- Impact: Users can tap `/settings` or `/chats` inside a topic and get confusing behavior.
|
||
- Tracked: Issue `#15` — `Telegram forum topics: remaining UX and synchronization gaps`
|
||
|
||
### Telegram forum: `/new <name>` inside linked topic does not rename the Telegram topic
|
||
|
||
- Issue: Running `/new <name>` inside a forum topic that is already linked to a chat renames the internal chat record but does not call `edit_forum_topic` to rename the actual Telegram topic.
|
||
- Files: `.worktrees/telegram/adapter/telegram/handlers/forum.py`
|
||
- Impact: Topic name in Telegram goes out of sync with internal chat name.
|
||
- Tracked: Issue `#15`
|
||
|
||
### Matrix: `handle_invite` hardcodes `chat_id = "C1"` for all new rooms
|
||
|
||
- Issue: `adapter/matrix/handlers/auth.py` `handle_invite()` always assigns `chat_id = "C1"` regardless of how many rooms the user already has. If a user invites the bot into a second room before using `!new`, both rooms get `C1`.
|
||
- Files: `adapter/matrix/handlers/auth.py` line 26
|
||
- Impact: Two rooms mapped to the same `chat_id` causes routing collisions.
|
||
- Fix approach: Call `next_chat_id(store, user_id)` here instead of hardcoding `"C1"`.
|
||
|
||
### Matrix: `remove_reaction` uses non-standard `undo` field
|
||
|
||
- Issue: `adapter/matrix/reactions.py` `remove_reaction()` sends a `"undo": True` field in the reaction event body. This is not part of the Matrix spec for reaction redaction. The correct approach is to redact the original reaction event via `client.room_redact()`.
|
||
- Files: `adapter/matrix/reactions.py` lines 56–68
|
||
- Impact: Reaction "undo" will silently fail on compliant homeservers.
|
||
|
||
### Matrix: E2EE not supported (blocked by `python-olm`)
|
||
|
||
- Issue: `matrix-nio` E2EE requires `python-olm`, which fails to build on macOS/ARM. No encrypted DM support.
|
||
- Files: `adapter/matrix/bot.py`
|
||
- Impact: The bot cannot operate in encrypted rooms. Users who have DM encryption enforced cannot use the Matrix bot.
|
||
- Status: Documented as a known infrastructure constraint in `docs/reports/2026-04-01-surfaces-progress-report.md`. Needs a separate infrastructure task.
|
||
|
||
---
|
||
|
||
## Security Considerations
|
||
|
||
### SQLite database files not in .gitignore
|
||
|
||
- Risk: `lambda_bot.db` and `lambda_matrix.db` are present in the working tree (shown in `git status`) but not listed in `.gitignore`. These files may contain user data including chat content and display names.
|
||
- Files: `lambda_bot.db`, `lambda_matrix.db`, `.gitignore`
|
||
- Current mitigation: Files are currently untracked (not yet staged) but nothing prevents them from being accidentally committed.
|
||
- Recommendation: Add `*.db` or specific filenames to `.gitignore` immediately.
|
||
|
||
### Auth flow is auto-confirmed in mock — no real validation exists
|
||
|
||
- Issue: `core/auth.py` `confirm()` automatically sets `state = "confirmed"` and generates a fake `platform_user_id`. There is no real verification step, no code exchange, no token validation.
|
||
- Files: `core/auth.py` lines 39–48
|
||
- Impact: The auth layer is decorative for the prototype. Any user who sends `!start` or `/start` is immediately authenticated. If the real SDK auth requires a different flow (e.g., OAuth, code), the current `AuthManager` interface may not match.
|
||
- Current mitigation: Acceptable for mock stage. Must be re-evaluated before production use.
|
||
|
||
### Matrix room metadata stored without access control
|
||
|
||
- Issue: `adapter/matrix/store.py` stores room metadata keyed by `room_id`. Any call that can supply an arbitrary `room_id` can read or overwrite another user's room metadata.
|
||
- Files: `adapter/matrix/store.py`, `adapter/matrix/room_router.py`
|
||
- Impact: In the current single-process bot this is not exploitable. If the store is ever shared across processes or users, room metadata can be poisoned.
|
||
|
||
---
|
||
|
||
## Fragile Areas
|
||
|
||
### `core/chat.py` scan-by-suffix fallback is O(N) and collision-prone
|
||
|
||
- Issue: `ChatManager.get()` when called without `user_id` scans all `chat:*` keys and matches by suffix (e.g., `":C1"`). If two users both have a chat named `C1` (which is always the case), this returns the first one found, non-deterministically.
|
||
- Files: `core/chat.py` lines 76–82
|
||
- Impact: Functions like `rename` and `archive` that call `chat_mgr.get(chat_id)` without `user_id` will operate on the wrong user's chat in a multi-user scenario.
|
||
- Fix approach: Audit all callers and always pass `user_id`. The scan-by-suffix fallback should be removed or explicitly guarded.
|
||
|
||
### `adapter/matrix/handlers/chat.py` chat_id counter races under concurrency
|
||
|
||
- Issue: `make_handle_new_chat` calls `chat_mgr.list_active()` and uses `len(chats) + 1` to compute a new `chat_id`. This is not atomic. Two concurrent `!new` commands from the same user can produce the same `chat_id`.
|
||
- Files: `adapter/matrix/handlers/chat.py` line 17
|
||
- Impact: Duplicate `chat_id` values (`C2`, `C2`) for the same user, leading to state corruption.
|
||
- Fix approach: Use `next_chat_id()` from `adapter/matrix/store.py` which increments an atomic counter in the store. The `next_chat_id()` function already exists but is not used here.
|
||
|
||
### `conftest.py` contains a fragile stdlib `platform` module workaround
|
||
|
||
- Issue: `conftest.py` patches `sys.modules` to remove the Python stdlib `platform` module so local `platform/` (which no longer exists — renamed to `sdk/`) doesn't shadow it. The comment still refers to `platform/` but the directory was renamed to `sdk/` in commit `41660fe`.
|
||
- Files: `conftest.py` lines 1–13
|
||
- Impact: The workaround is now a no-op (there is no `platform/` package to shadow) but adds confusion. The comment is incorrect. If someone creates a `platform/` directory again, unexpected behavior can return.
|
||
- Fix approach: Remove the `sys.modules` patching entirely since `sdk/` does not conflict with stdlib. Update the comment.
|
||
|
||
### Forum onboarding `chat_shared` constructs a fake `Chat` object
|
||
|
||
- Issue: `adapter/telegram/handlers/forum.py` handles `chat_shared` by constructing `Chat(id=..., type="supergroup", is_forum=True)` and passing it to `_complete_group_link()`. The `is_forum=True` is hardcoded — the real value from Telegram is not verified. This means the check `if getattr(forwarded_chat, "is_forum", None) is False` in the forwarding fallback path is bypassed entirely.
|
||
- Files: `.worktrees/telegram/adapter/telegram/handlers/forum.py` lines 162–168
|
||
- Impact: A user could link a regular supergroup (without Topics enabled) via `chat_shared`, which would succeed in linking but fail when the bot tries to create forum topics.
|
||
|
||
---
|
||
|
||
## Gaps Between CLAUDE.md and Actual Code
|
||
|
||
### CLAUDE.md says `platform/` — code uses `sdk/`
|
||
|
||
- CLAUDE.md architecture diagram shows `platform/interface.py` and `platform/mock.py`
|
||
- Actual code uses `sdk/interface.py` and `sdk/mock.py` (renamed in commit `41660fe`)
|
||
- Files: `CLAUDE.md` (project instructions), `sdk/interface.py`, `sdk/mock.py`
|
||
- Also: Agent config files at `.claude/agents/core-developer.md` still reference `platform/` throughout
|
||
- Impact: New contributors reading CLAUDE.md will look for a `platform/` directory that does not exist.
|
||
|
||
### CLAUDE.md lists `core/handlers/` sub-handlers that partially do not exist
|
||
|
||
- CLAUDE.md lists handler modules but the actual `core/handlers/` only has: `start.py`, `message.py`, `chat.py`, `settings.py`, `callback.py`
|
||
- No `voice.py` handler exists; voice is handled as a fallback inside `core/handlers/message.py` (returns stub response)
|
||
- No `payment.py` handler exists; `PaymentRequired` dataclass is defined in `core/protocol.py` but never dispatched
|
||
- Files: `core/protocol.py` (PaymentRequired defined), `core/handlers/` (no payment or voice handlers)
|
||
|
||
### CLAUDE.md workflow describes `@reviewer` agent but agent file references old patterns
|
||
|
||
- `.claude/agents/core-developer.md` still says "Твоя зона — `core/` и `platform/`"
|
||
- The old Haiku/Sonnet researcher-developer workflow is captured in `docs/workflow-backup-2026-04-01.md`, but `.claude/agents/` configs were not updated to match
|
||
|
||
### `tests/adapter/test_forum_db.py` is untracked on main
|
||
|
||
- This test file exists in the working tree (visible in `git status`) but is not committed to `main`. It tests `adapter/telegram/db.py` which also does not exist on `main`.
|
||
- Files: `tests/adapter/test_forum_db.py`
|
||
- Impact: Running `pytest tests/` from main currently includes this test, which imports `adapter.telegram.db`. This import succeeds only because the test auto-reloads the module from an untracked file. This is fragile — if the file is deleted, tests silently pass with fewer tests counted.
|
||
|
||
---
|
||
|
||
## Missing Critical Features
|
||
|
||
### No streaming response support in adapters
|
||
|
||
- Both adapters use `platform.send_message()` (sync) not `platform.stream_message()` (streaming)
|
||
- `sdk/interface.py` defines `stream_message` returning `AsyncIterator[MessageChunk]`
|
||
- No adapter sends a typing indicator before the response arrives and then streams chunks
|
||
- Impact: User experience with slow AI responses will show nothing until the full response is ready
|
||
- Files: `core/handlers/message.py` line 28, `sdk/interface.py` lines 83–88
|
||
|
||
### No webhook/push notification handling
|
||
|
||
- `sdk/interface.py` defines `WebhookReceiver` Protocol with `on_agent_event()`
|
||
- `sdk/mock.py` has `register_webhook_receiver()` and `simulate_agent_event()`
|
||
- Neither bot entrypoint registers a `WebhookReceiver`
|
||
- Impact: Push notifications from the platform (task completions, background jobs) cannot reach the user
|
||
- Files: `sdk/interface.py` lines 95–97, `adapter/matrix/bot.py`, no registration present
|
||
|
||
### Telegram adapter uses InMemoryStore for core state
|
||
|
||
- `.worktrees/telegram/adapter/telegram/bot.py` calls `InMemoryStore()` for the `EventDispatcher`'s state
|
||
- All `core/` state (auth, chat metadata in the KV layer) is lost on bot restart
|
||
- `adapter/telegram/db.py` SQLite is used only for Telegram-specific data
|
||
- Impact: On restart, authenticated users are logged out; core chat context is wiped
|
||
- Files: `.worktrees/telegram/adapter/telegram/bot.py` line 46
|
||
|
||
### No multi-user isolation in Matrix store
|
||
|
||
- `adapter/matrix/store.py` keys are global (`matrix_room:ROOMID`, `matrix_user:USERID`)
|
||
- There is no namespace or tenant isolation
|
||
- Impact: At scale, any key collision would corrupt state. For a single-user prototype this is acceptable, but it is an architectural constraint to document before expanding scope.
|
||
|
||
---
|
||
|
||
## Test Coverage Gaps
|
||
|
||
### No tests for `adapter/telegram/` in main test suite
|
||
|
||
- `tests/adapter/` on main only contains `matrix/` tests and the untracked `test_forum_db.py`
|
||
- All Telegram adapter tests live in the worktree at `.worktrees/telegram/tests/`
|
||
- Files: `tests/adapter/` (missing `telegram/` subdirectory on main)
|
||
- Risk: Merging `feat/telegram-adapter` without also merging its tests leaves Telegram untested on main
|
||
- Priority: High
|
||
|
||
### No tests for `core/handlers/callback.py` confirm/cancel real behavior
|
||
|
||
- `core/handlers/callback.py` `handle_confirm` and `handle_cancel` return stub text with `action_id`
|
||
- No test verifies that a real confirmation flow (dispatch → confirm → side effect) works end to end
|
||
- Files: `core/handlers/callback.py`, `tests/core/test_dispatcher.py`
|
||
- Priority: Medium
|
||
|
||
### No tests for `adapter/matrix/handlers/auth.py` multi-room invite scenario
|
||
|
||
- The hardcoded `C1` bug (see Known Bugs section) is not caught by any test
|
||
- Files: `adapter/matrix/handlers/auth.py`, `tests/adapter/matrix/test_dispatcher.py`
|
||
- Priority: Medium
|
||
|
||
---
|
||
|
||
*Concerns audit: 2026-04-01*
|