feat(ai-clone): backend persona-chat endpoint + Telegram plugin (v0.1)#8437
feat(ai-clone): backend persona-chat endpoint + Telegram plugin (v0.1)#8437choguun wants to merge 22 commits into
Conversation
Spec source: PLAN.md Track 2 (AI clone) + spectrum-ts reference. Recommends unified TypeScript bridge on spectrum-ts self-hosted over PLAN.md's split (Python Telegram/WhatsApp + TS iMessage). Reuses existing persona engine + app API key auth unchanged.
Per user direction: self-hosted from day one, keep maintainable from existing codebase, honest current design. Drop spectrum-ts proposal. Stick with per-provider Python FastAPI plugins modeled on omi-slack-app. iMessage is local-only (sqlite poll of chat.db + osascript send). Resolve all 4 open questions in the spec.
8 vertical-slice tasks: backend endpoint + capability, shared persona client, three per-provider plugins (Telegram/WhatsApp/iMessage), Flutter Clone screen, Chat Tools manifest. Each task lands as its own commit with green unit tests. Plugin tasks parallelizable post-T-002.
- backend/utils/apps.py: app_can_persona_chat(app) — 1-line wrapper around
app_has_action(app, 'persona_chat'), gates the new endpoint.
- backend/models/integrations.py: PersonaChatRequest Pydantic model with
text (min_length=1) and optional context dict.
- backend/routers/integration.py: POST /v2/integrations/{app_id}/user/persona-chat
with app API key auth (Bearer omi_dev_...), per-(app,user) rate limit
(mirrors existing check_rate_limit_inline pattern), app lookup +
enabled-for-user check + persona_chat capability gate, then streams
the reply via execute_chat_stream (same generator the chat UI uses).
- backend/tests/unit/test_persona_chat_endpoint.py: 14 tests covering
capability gate (5), request model (3), and endpoint auth/404/403/200
behavior (6). All green.
Backend persona-chat endpoint + capability gate shipped at 670585871. 14 unit tests green. Next: T-002 (shared persona_client module).
plugins/_shared/persona_client.py - async chat() that POSTs to
/v2/integrations/{app_id}/user/persona-chat with Bearer app API key
auth, reads the SSE stream via httpx_sse.EventSource, joins the
chunks into a single reply string. Returns '' on timeout/connect
error and logs at ERROR. Raises httpx.HTTPStatusError on 4xx/5xx so
the caller decides retry policy.
Three plugins (T-003/T-005/T-006) will import this verbatim.
11 unit tests in plugins/_shared/test/test_persona_client.py:
- success: concat, auth header, URL, JSON body
- SSE parsing: comments ignored, empty stream -> ''
- errors: 401/403/500 raise HTTPStatusError; timeout/connect error
return '' and log.
Shared persona_client shipped at 4b4b35b0a. 11 unit tests green. Next: T-003 (Telegram plugin skeleton + setup).
plugins/omi-telegram-app/ scaffolded per spec:
- main.py FastAPI app, /health, /setup, /webhook (no auto-reply yet)
- telegram_client.py async wrapper for api.telegram.org (setWebhook,
getMe, sendMessage)
- simple_storage.py JSON-file persistence for users + pending_setups
(mirrors omi-slack-app pattern)
- persona_client.py re-export of plugins/_shared/persona_client
- requirements.txt, Dockerfile, Procfile, runtime.txt, README.md
Setup flow:
- POST /setup accepts bot_token + omi_uid + persona_id + omi_dev_api_key
+ public_base_url, registers the webhook with Telegram (with secret_token
header verification), returns {deep_link, bot_username, setup_token}.
- POST /webhook verifies X-Telegram-Bot-Api-Secret-Token, handles /start
handshake (binds chat_id to user), nudges regular messages from known
chats with auto_reply disabled, silently 200s on unknown chats.
15 unit tests in plugins/omi-telegram-app/test/test_main.py:
- /health (1)
- /setup: deep link, setWebhook, getMe, pending_setups persistence,
failure path (5)
- /webhook: secret header validation, unknown chat silence, /start
handshake stores mapping + confirms, regular message nudge, regular
message from unknown chat no-reply (6)
- simple_storage round-trip for users, pending_setups, auto_reply
toggle (3)
T-004 will wire the persona dispatch loop into /webhook.
Telegram plugin skeleton + setup flow shipped at 386dc38ce. 15 unit tests green. Next: T-004 (Telegram auto-reply - wire persona dispatch into /webhook).
main.py /webhook handler now:
- Safety filters: skip groups/supergroups/channels, skip bot senders
(own-message), skip non-text payloads (voice/photo/sticker out of scope v1).
- For known private chat with auto_reply_enabled: calls _persona_chat()
(the shared plugins/_shared/persona_client), then sends the reply via
telegram_client.send_message.
- Empty reply (timeout/connect error) -> logged, no send (Telegram 200 OK).
- HTTPStatusError from persona -> logged, no crash, no send.
- Unexpected exception -> caught, logged, no crash (webhook MUST 200).
New endpoint POST /toggle with {chat_id, enabled} -> flips the
auto_reply_enabled flag for that chat, returns 404 if unknown. Wired
to be called by Chat Tools in T-008.
12 new unit tests in plugins/omi-telegram-app/test/test_auto_reply.py:
- Dispatch: persona -> sendMessage (1), empty reply no-send (1),
HTTPStatusError no-crash (1).
- Safety: group / supergroup / channel skipped (3), bot sender
skipped (1), no-text skipped (1), auto_reply disabled still
nudges (1).
- /toggle: enable, disable, unknown chat 404 (3).
Telegram auto-reply dispatch + /toggle shipped at e44bd0fe9. 12 new tests, total 52 green. Telegram clone is feature-complete for v1. Next: T-005 (WhatsApp plugin - mechanical copy of Telegram with Meta payload parsing).
Critical: - C1: don't log webhook secret at startup (was leaking first 8 chars) - C2: rate-limit the disabled-auto-reply nudge to once per _NUDGE_COOLDOWN_SECONDS (default 4h, configurable via env). Added last_nudge_at to user record + simple_storage.should_nudge / mark_nudged helpers. - C3: atomic file writes in simple_storage via tmp+fsync+os.replace. Cleans up .tmp on failure. Warnings: - W1: secrets.compare_digest for webhook secret constant-time compare. - W2: removed unused 'context' field from PersonaChatRequest. The plugin signature still accepts context= but the backend now ignores it. A future task can add a Message.context field if the persona engine wants to use it. - W3: moved Message/MessageSender/MessageType imports to module level in routers/integration.py. - W4: updated webhook handler docstring to reflect three paths (was 'two'). - W5: narrowed except clauses to (httpx.HTTPError, asyncio.TimeoutError, json.JSONDecodeError, KeyError). Removed the bare 'except Exception' that would have swallowed KeyboardInterrupt. - W6: telegram_client.send_message now truncates text > 4096 chars with a trailing U+2026 ellipsis and logs a warning. - W7: removed stale 'T-004' comment from module docstring (T-004 is done). - W8: covered with test (bare /start -> 200, no sendMessage). Tests added (test_fixes.py, 13 new): - Nudge cooldown: first nudge sends, second within window doesn't, after cooldown does. Helper tests for should_nudge. - Atomic writes: _save uses os.replace + cleans .tmp on success and failure. - Reply truncation: short text passes through, > 4096 truncated with ellipsis. - Bare /start: silently 200, no sendMessage. - Malformed JSON body: 200, no crash. - Non-dict JSON body: 200, no crash. Suggestions applied: - S1: Message.id is now 'integration-persona-chat:<random 12 chars>' so concurrent requests don't share an id. - S8: update_auto_reply now raises KeyError on unknown chat_id instead of returning bool. Caller (main.py toggle) already checks existence first. Deferred (documented in plan/SUGGESTIONS): - S2: bot token in URL path (Telegram's documented format; HTTPS terminator log risk accepted for v1). - S3: setup token TTL (one-shot; cleanup task deferred). - S5: MessageType.integration_prompt enum (no consumer yet). - S6: /toggle auth (chat_id is a long random string). - S7: load_storage at import time (defer to FastAPI lifespan). - S9: CHANGELOG.json entry (lands with T-007 desktop screen). Total: 65 tests green (14 backend + 11 persona_client + 40 telegram). 14 new tests since the review.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
14 issues found across 21 files
Confidence score: 2/5
backend/routers/integration.pycurrently passes a DB dict fromget_app_by_id_dbintoexecute_chat_stream, which expects anAppmodel and can raiseAttributeErrorwhen callingapp.is_a_persona(); the same area also introducesintegration:personawithout guaranteedRATE_POLICIESregistration, risking runtime failures under traffic—convert/validate toAppand register (or guard) the rate-limit policy before merging.plugins/omi-telegram-app/main.pyexposes/setupand/togglestate-changing endpoints without authz checks, so unauthorized callers could reconfigure integrations in production—add authentication/authorization middleware or request signing before merge.plugins/omi-telegram-app/telegram_client.pylogs rawhttpx.HTTPError, which can include full request URLs and leak the Telegram bot token into logs—sanitize exception logging (or log structured safe fields only) before merging.- Input and runtime hardening gaps in
backend/models/integrations.pyandplugins/omi-telegram-app/main.py/Dockerfile(unboundedPersonaChatRequest.text, crash-prone env float parsing, and root container user) increase DoS and operational risk—add max-length validation, safe env parsing defaults, and run the container as a non-root user to de-risk release.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/omi-telegram-app/main.py">
<violation number="1" location="plugins/omi-telegram-app/main.py:93">
P1: `/setup` and `/toggle` endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| setup_token: str | ||
|
|
||
|
|
||
| @app.post("/setup", response_model=SetupResponse) |
There was a problem hiding this comment.
P1: /setup and /toggle endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/omi-telegram-app/main.py, line 93:
<comment>`/setup` and `/toggle` endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.</comment>
<file context>
@@ -0,0 +1,339 @@
+ setup_token: str
+
+
+@app.post("/setup", response_model=SetupResponse)
+async def setup(req: SetupRequest):
+ """Register the user's bot and return a one-time deep link for the user to click."""
</file context>
14 issues found by cubic; 12 fixed here, 2 deferred with rationale.
Critical (P1, blocks runtime):
1. **integration:persona not in RATE_POLICIES** (routers/integration.py:736)
Added the policy to utils/rate_limit_config.py with 60/hour ceiling —
matches integration:memories. Without this, every request KeyErrors.
2. **DB dict passed to execute_chat_stream expecting App model**
(routers/integration.py:739)
get_app_by_id_db returns a Firestore dict, but execute_chat_stream
calls app.is_a_persona() (a method on the Pydantic App class). Without
coercion, every request AttributeErrors. Fixed by:
- Coercing app_dict to App(**app_dict) after capability + enabled checks
- Adding ActionType.PERSONA_CHAT = 'persona_chat' to models/app.py
(the enum was rejecting 'persona_chat' as an invalid action)
- Wrapping the coercion in try/except so malformed Firestore docs
return 502 instead of crashing
3. **Bot token leak in error logs** (telegram_client.py:76)
httpx.HTTPStatusError.__str__ includes the full request URL — which
contains the bot token. Split the except clause so we log only the
status code, not the full exception repr.
Defensive hardening (P2):
4. **Dockerfile: non-root user + clean apt-get** (Dockerfile)
Added groupadd/useradd for the 'omi' user, USER omi before CMD, and
removed the empty 'apt-get install' step (no system deps required).
5. **NUDGE_COOLDOWN_SECONDS crash on malformed float** (main.py:58)
Wrapped float(os.getenv(...)) in try/except so a malformed env value
logs a warning and falls back to the default instead of crashing
service startup.
6. **Dead re-export creates circular-import risk**
(plugins/omi-telegram-app/persona_client.py)
The file does 'from persona_client import chat' inside itself; main.py
imports from the shared module directly. The re-export was never
triggered and was a footgun for anyone who did 'import persona_client'
from the plugin dir. Deleted.
7. **PersonaChatRequest.text unbounded** (models/integrations.py:51)
Added max_length=8192 (covers WhatsApp's 65k cap with margin; larger
than Telegram's 4096; comfortable for the persona engine).
8. **README.md inaccuracies** (plugins/omi-telegram-app/README.md)
- 'Auto-reply is future work' — wrong, it's implemented.
- Missing /toggle endpoint in the endpoints list.
- Webhook secret restart behavior was ambiguous; now says 'MUST be
set in production' explicitly.
Performance (P2):
9. **Shared httpx.AsyncClient in telegram_client** (telegram_client.py)
Module-level client with connection pooling. Was creating a new client
per Telegram API call — repeated TLS handshakes under load. Tests
patched _get_client to inject the mock client.
Code clarity (P2):
10. **_split_lines preserves blank lines** (plugins/_shared/persona_client.py)
Multi-line SSE data (rare but legitimate — code blocks, lists) used
to have blank lines filtered out. Now preserves them. Added a test.
Deferred (documented in commit + PR):
11. **/setup and /toggle auth** (main.py:93) — requires design decision
(request signing? session token? HMAC of a per-user secret generated
at handshake?). Out of scope for v0.1; will be a follow-up issue.
12. **Accept: text/event-stream redundant header** (persona_client.py:80)
Harmless; informational only. Skipped per the 'cosmetic' guidance.
Tests:
- New SSE blank-lines preservation test (test_blank_lines_in_sse_data_are_preserved).
- All 14 backend tests + 12 persona_client tests + 40 telegram plugin tests
remain green (66 total, up from 65).
Verified: branch has same 5 pre-existing collection errors as main
(test_prompt_caching et al — pre-existing, unrelated to this PR).
Cubic review fixes — pushed as commit
|
AIDLC state, spec, and plan files were being committed as part of the loop protocol, but they're scratch state for the agent workflow, not project source. Local-only from now on (the .aidlc/ directory still exists in the worktree, just not in git). The plugin README.md, backend CHANGELOG, AGENTS.md, and other project .md files stay tracked — those are real documentation.
There was a problem hiding this comment.
2 issues found across 15 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/omi-telegram-app/main.py">
<violation number="1" location="plugins/omi-telegram-app/main.py:93">
P1: `/setup` and `/toggle` endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ing tests test_persona_chat_endpoint.py registered module stubs via direct sys.modules[name] = mod assignment, which overwrote the real modules already imported by other backend tests. When pytest collected the full backend/unit tree, downstream tests failed with import errors (database.redis_db.r, database.cache, etc. pointing to MagicMocks). Switch to sys.modules.setdefault — we only stub modules that haven't been loaded yet, so we never clobber. My persona_chat tests still get their stubs (run first in collection), and other tests get the real modules. Verified: branch now has the same 5 pre-existing test collection errors as main. No regression.
14 issues found by cubic; 12 fixed here, 2 deferred with rationale.
Critical (P1, blocks runtime):
1. **integration:persona not in RATE_POLICIES** (routers/integration.py:736)
Added the policy to utils/rate_limit_config.py with 60/hour ceiling —
matches integration:memories. Without this, every request KeyErrors.
2. **DB dict passed to execute_chat_stream expecting App model**
(routers/integration.py:739)
get_app_by_id_db returns a Firestore dict, but execute_chat_stream
calls app.is_a_persona() (a method on the Pydantic App class). Without
coercion, every request AttributeErrors. Fixed by:
- Coercing app_dict to App(**app_dict) after capability + enabled checks
- Adding ActionType.PERSONA_CHAT = 'persona_chat' to models/app.py
(the enum was rejecting 'persona_chat' as an invalid action)
- Wrapping the coercion in try/except so malformed Firestore docs
return 502 instead of crashing
3. **Bot token leak in error logs** (telegram_client.py:76)
httpx.HTTPStatusError.__str__ includes the full request URL — which
contains the bot token. Split the except clause so we log only the
status code, not the full exception repr.
Defensive hardening (P2):
4. **Dockerfile: non-root user + clean apt-get** (Dockerfile)
Added groupadd/useradd for the 'omi' user, USER omi before CMD, and
removed the empty 'apt-get install' step (no system deps required).
5. **NUDGE_COOLDOWN_SECONDS crash on malformed float** (main.py:58)
Wrapped float(os.getenv(...)) in try/except so a malformed env value
logs a warning and falls back to the default instead of crashing
service startup.
6. **Dead re-export creates circular-import risk**
(plugins/omi-telegram-app/persona_client.py)
The file does 'from persona_client import chat' inside itself; main.py
imports from the shared module directly. The re-export was never
triggered and was a footgun for anyone who did 'import persona_client'
from the plugin dir. Deleted.
7. **PersonaChatRequest.text unbounded** (models/integrations.py:51)
Added max_length=8192 (covers WhatsApp's 65k cap with margin; larger
than Telegram's 4096; comfortable for the persona engine).
8. **README.md inaccuracies** (plugins/omi-telegram-app/README.md)
- 'Auto-reply is future work' — wrong, it's implemented.
- Missing /toggle endpoint in the endpoints list.
- Webhook secret restart behavior was ambiguous; now says 'MUST be
set in production' explicitly.
Performance (P2):
9. **Shared httpx.AsyncClient in telegram_client** (telegram_client.py)
Module-level client with connection pooling. Was creating a new client
per Telegram API call — repeated TLS handshakes under load. Tests
patched _get_client to inject the mock client.
Code clarity (P2):
10. **_split_lines preserves blank lines** (plugins/_shared/persona_client.py)
Multi-line SSE data (rare but legitimate — code blocks, lists) used
to have blank lines filtered out. Now preserves them. Added a test.
Deferred (documented in commit + PR):
11. **/setup and /toggle auth** (main.py:93) — requires design decision
(request signing? session token? HMAC of a per-user secret generated
at handshake?). Out of scope for v0.1; will be a follow-up issue.
12. **Accept: text/event-stream redundant header** (persona_client.py:80)
Harmless; informational only. Skipped per the 'cosmetic' guidance.
Tests:
- New SSE blank-lines preservation test (test_blank_lines_in_sse_data_are_preserved).
- All 14 backend tests + 12 persona_client tests + 40 telegram plugin tests
remain green (66 total, up from 65).
Verified: branch has same 5 pre-existing collection errors as main
(test_prompt_caching et al — pre-existing, unrelated to this PR).
Two issues flagged by cubic on re-review of PR BasedHardware#8437: P2 (routers/integration.py) — Internal exception details leaked in 502 response when App model parsing fails. The previous fix returned the full Pydantic ValidationError message in detail, exposing internal field names ('capabilities', 'external_integration.actions.0.action') and data shape to anyone hitting the endpoint. Now: log the full exception server-side, return generic 'App data is malformed' to the client. P1 (main.py) — /toggle endpoint had no auth; chat_id alone is scrapable from Telegram update payloads. Now requires bot_token in the request body and verifies (constant-time via secrets.compare_digest) that it matches the stored token for that chat_id. Bot tokens are real secrets (calling setWebhook with the wrong token fails at Telegram), so this raises the bar from 'knows chat_id' to 'knows chat_id AND bot_token'. /setup already had implicit auth via bot_token verification round-trip (Telegram rejects bad tokens). Documented in the endpoint comment. Tests: - 3 existing toggle tests updated to pass bot_token - 2 new tests: wrong bot_token returns 403, missing bot_token returns 422 Total: 68 tests green (was 66).
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/omi-telegram-app/main.py">
<violation number="1" location="plugins/omi-telegram-app/main.py:93">
P1: `/setup` and `/toggle` endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Second cubic review — both issues fixed (commit
|
Cubic's third pass on PR BasedHardware#8437 flagged a chat_id enumeration risk: returning 404 for unknown chat_id vs 403 for wrong bot_token let attackers probe which chat_ids were registered. Both cases now return 403 with a generic 'Invalid chat_id or bot_token' message. The endpoint no longer leaks the existence of a registered chat_id. Test updated: test_toggle_unknown_chat_returns_404 -> now expects 403. 68 tests still green.
Third cubic review — chat_id enumeration fix (commit
|
Git-on-my-level
added
needs-maintainer-review
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the large first slice here — the direction is interesting and the tests show a lot of care around the Telegram flow. I’m requesting changes because I found a backend/client contract break and an auth boundary that needs maintainer/security review before this can be considered mergeable.
Blocking issues:
-
The Telegram/shared client currently cannot call the new backend endpoint successfully.
persona_chat_via_integrationrequiresuidas a query parameter, butplugins/_shared/persona_client.pyposts only to/v2/integrations/{app_id}/user/persona-chatwith{"text": ...}._dispatch_auto_reply()hasuser["omi_uid"], but it never passes it through. In production this should 422 before reaching the persona stream. Please make the contract explicit, e.g. add auidargument to the shared client, send it asparams={"uid": uid}, update the Telegram call site, and add a cross-component test that would fail if the backend-required parameters and client request drift again. -
The new persona-chat route is a sensitive impersonation surface: it streams replies as a user’s persona based on a caller-supplied
uid. Right now the authorization check verifies only the app API key forapp_idand then trustsuidif that app is enabled for the user. For AI-clone auto-reply, that boundary should be tighter than the existing generic integration endpoints: the caller should not be able to choose an arbitrary enabled user just by holding an app-level key. Please bind the request to a user-scoped credential/session or otherwise prove the API key is authorized for that exactuidbefore invokingexecute_chat_stream. -
The tests are currently too mocked/permissive to catch (1): the backend tests call
?uid=..., while the shared client tests assert a URL withoutuid, so both sides pass independently while the feature is broken end-to-end. Please add a contract/integration test around the plugin client calling the actual FastAPI route shape, and keep the auth/user-binding case covered.
Because this is a broad AI-clone feature touching user-data/persona behavior, dependency/runtime files, and a new Telegram service, it should also get human maintainer product/security review after the above is addressed. I did not formally approve under the maintainer automation policy.
… contract)
Three blocking issues from Git-on-my-level's review:
1. **Plugin client never sent uid** (would 422 every request in prod)
The backend route declares 'uid: str' as a query parameter (FastAPI
extracts it from the URL). The shared persona_client.chat() POSTed
only a JSON body, missing the query param, so every persona request
would have failed with 422 in production.
Fix: chat() now takes uid as a required kwarg and sends it as
params={'uid': uid}. Telegram dispatch passes user['omi_uid'].
2. **Auth bypass — caller could pick any enabled uid** (impersonation)
The old check was verify_api_key(app_id, api_key) which only proved
the caller holds a valid app-level key. Then the endpoint trusted
the URL uid. Anyone with a valid app key could impersonate any user
who had enabled the persona app.
Fix:
- Added verify_api_key_for_uid(app_id, uid, api_key) in utils/apps.py
that additionally checks the key's stored uid matches.
- The persona-chat route uses this stricter check.
- create_api_key_for_app now stamps 'uid' on the api_keys doc.
- Legacy keys (no uid field) are rejected — sensitive endpoints
require the new key shape.
- Existing 7+ integration endpoints continue to use the looser
verify_api_key (backward compat).
3. **Tests didn't catch contract drift** (the bug from #1)
New file: plugins/_shared/test/test_contract.py with 4 tests that
pin the URL path, query-param shape, and uid placement from BOTH
sides simultaneously. If either side drifts, a test fails immediately.
Also added test_sends_uid_as_query_param in test_persona_client.py
that asserts the client sends params={'uid': uid} explicitly.
Tests: 73 green (was 68). 4 new contract tests + 1 new uid-param test.
Verified: same 5 pre-existing collection errors as main.
Maintainer review fixes — all three blocking issues addressed (commit
|
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/omi-telegram-app/main.py">
<violation number="1" location="plugins/omi-telegram-app/main.py:93">
P1: `/setup` and `/toggle` endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.</violation>
</file>
<file name="backend/routers/apps.py">
<violation number="1" location="backend/routers/apps.py:1976">
P2: Legacy API keys lack the new 'uid' field, so they may break when the persona-chat auth path enforces key ownership verification.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # Stamp the uid on the key so sensitive endpoints (e.g. persona-chat) | ||
| # can verify the key was issued by this exact user, not just by anyone | ||
| # who happens to hold an app-level key. | ||
| 'uid': uid, |
There was a problem hiding this comment.
P2: Legacy API keys lack the new 'uid' field, so they may break when the persona-chat auth path enforces key ownership verification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/apps.py, line 1976:
<comment>Legacy API keys lack the new 'uid' field, so they may break when the persona-chat auth path enforces key ownership verification.</comment>
<file context>
@@ -1965,7 +1965,16 @@ def create_api_key_for_app(app_id: str, uid: str = Depends(auth.get_current_user
+ # Stamp the uid on the key so sensitive endpoints (e.g. persona-chat)
+ # can verify the key was issued by this exact user, not just by anyone
+ # who happens to hold an app-level key.
+ 'uid': uid,
+ }
create_api_key_db(app_id, data)
</file context>
Two more P2 issues from cubic's re-review of the maintainer-fix commit: 1. Legacy keys without 'uid' field (created before this PR) would 403 on the persona-chat endpoint. Fix: when verify_api_key_for_uid finds a key without a 'uid' field, fall back to the parent app's owner uid. This preserves the auth model for existing installations (the key owner == app owner == the developer who created it). New keys stamped with 'uid' skip the fallback. 2. verify_api_key and verify_api_key_for_uid had duplicated prefix-strip + hash + lookup logic — drift risk in security code. Fix: extracted _lookup_api_key() helper as the single source of truth. Both functions now call it. Tests added (test_persona_chat_endpoint.py): - test_returns_403_when_key_uid_mismatches — strict check rejects when key's uid != URL uid - test_auth_uses_strict_verify_not_loose — endpoint never calls the loose verify_api_key on the persona-chat path (regression guard against the impersonation bypass being re-introduced) 75 tests green (was 73).
Cubic pass-2 fixes (commit
|
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for iterating quickly on the earlier backend/client contract and user-binding concerns. I re-reviewed the current head (f041851a) and I’m still requesting changes because there is a remaining token-leak issue in the Telegram setup error path.
Blocking issue:
plugins/omi-telegram-app/main.pystill logs and returns raw Telegramhttpxexceptions from/setup:logger.error("set_webhook failed: %s", e)/detail=f"Telegram setWebhook failed: {e}"logger.error("getMe failed: %s", e)/detail=f"Telegram getMe failed: {e}"
telegram_client.set_webhook() and telegram_client.get_me() call URLs of the form https://api.telegram.org/bot{bot_token}/.... For httpx.HTTPStatusError, the exception string includes the request URL, so a bad token, bad webhook URL, or Telegram-side 4xx/5xx can leak the user’s Telegram bot token into service logs and into the HTTP 502 response body. The send_message() path was sanitized, but the setup path needs the same treatment.
Suggested fix: catch httpx.HTTPStatusError separately for both setWebhook and getMe, log only safe structured fields such as status code (not str(e)), and return a generic 502 detail that does not include the exception string. For non-status HTTP errors, also avoid echoing str(e) back to clients unless it is guaranteed not to contain request URLs or secrets. Please add a regression test that simulates a Telegram HTTPStatusError with a bot-token URL and asserts the token is not present in logs or response JSON.
Non-blocking cleanup while touching this area: plugins/_shared/README.md still documents chat(app_id, api_key, omi_base, text, ...) and the example omits the now-required uid= argument, so the docs are stale after the contract fix.
Given this is still a broad AI-clone/persona feature touching user impersonation behavior, bot/API-key storage, dependency/runtime files, and a new public Telegram service, it should remain under human product/security maintainer review after this fix. I did not formally approve under the maintainer automation policy.
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
AI Clone backend foundation + Telegram persona-chat (Track 2 first slice). Approving as feature.
… dead code Two issues from the maintainer's re-review of commit f041851: 1. **Blocking: /setup error path leaked the Telegram bot token.** httpx.HTTPStatusError.__str__ includes the full request URL, which contains the bot token. The old code logged str(e) and returned it in the HTTPException detail. Fix in main.py: - /setup set_webhook + getMe: catch HTTPStatusError separately, log only e.response.status_code, return generic 502 detail. - _dispatch_auto_reply (same leak pattern for persona chat HTTP errors): same split-catch + status-code-only logging. - Generic httpx.HTTPError catch: log type(e).__name__ (no str(e)). - asyncio.TimeoutError: aligned to type(e).__name__ for consistency. 2. **Non-blocking: plugins/_shared/README.md was stale.** The chat() signature requires 'uid' (added in commit 7f334b7) but the README omitted it. Updated signature, added the auth-boundary rationale, fixed example usage, updated test count (11 -> 13) and added a reference to the new test_contract.py. Regression coverage (verified — tests fail on buggy code, pass on fix): - test_setup_token_leak.py (new, 4 tests): setWebhook fail + getMe fail + non-status error; asserts bot token absent from response body and all log records. - test_auto_reply.py::TestDispatchErrorPathDoesNotLeakSecrets (new, 2 tests): persona HTTPStatusError + ConnectError; asserts api_key absent from logs. Cleanups (from sub-agent review): - Removed dead _setup_dispatch_with_error helper in test_auto_reply.py (seeded storage that no test read; unused 'error' parameter). - Aligned asyncio.TimeoutError logging to use type(e).__name__ (consistency with the other two exception branches). 81 tests pass (was 75).
Maintainer re-review fixes — pushed (
|
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up here. I re-reviewed the current head (1dd180f) and the previously blocking Telegram token-leak path appears addressed: /setup now catches httpx.HTTPStatusError separately for setWebhook/getMe, logs only safe status/type information, returns generic 502 details, and includes a regression test for token-bearing Telegram URLs. I also verified the focused regression/shared-client tests pass once the async pytest plugin is installed.
One remaining change before I’d consider this implementation-review-clean:
- The new plugin/shared tests use
@pytest.mark.asyncio, but the added standalone plugin requirements do not includepytest-asyncio, and the README’s advertised test command does not mention installing it. In a clean venv withplugins/omi-telegram-app/requirements.txtpluspytest, the focused tests fail with “async def functions are not natively supported” / unknownpytest.mark.asyncio. After installingpytest-asyncio, the same focused set passes (34 passed). Please add/document the test dependency in the appropriate plugin dev/test requirements path so the tests are reproducible from the instructions.
Commands run in an isolated review venv:
python -m pip install -r plugins/omi-telegram-app/requirements.txt pytest
python -m pytest plugins/omi-telegram-app/test/test_setup_token_leak.py plugins/omi-telegram-app/test/test_fixes.py plugins/_shared/test/test_persona_client.py plugins/_shared/test/test_contract.py -q
# -> 15 failed, 19 passed; async pytest plugin missing
python -m pip install pytest-asyncio
python -m pytest plugins/omi-telegram-app/test/test_setup_token_leak.py plugins/omi-telegram-app/test/test_fixes.py plugins/_shared/test/test_persona_client.py plugins/_shared/test/test_contract.py -q
# -> 34 passedSeparately, this still needs human product/security maintainer review before merge because it is a broad AI-clone/persona surface that can reply on a user’s behalf, stores/transmits Telegram bot tokens and Omi API keys, adds runtime/dependency files, and changes backend auth/capability behavior. I’m not formally approving under the maintainer automation policy.
The new async tests added in commit 1dd180f require pytest-asyncio, but it was not declared in any requirements file or documented in either README. Maintainer Git-on-my-level confirmed: in a clean venv with just the plugin's requirements.txt + pytest, the focused tests fail with "async def functions are not natively supported" (15 failed, 19 passed). After pip install pytest-asyncio, the same set passes (34 passed). Fix: - New plugins/omi-telegram-app/requirements-dev.txt with pytest>=8.0 and pytest-asyncio>=0.23, plus a comment explaining which test files actually use async (test_auto_reply.py::TestDispatchErrorPathDoesNotLeakSecrets and test_fixes.py::TestReplyTruncation — test_setup_token_leak.py does NOT need pytest-asyncio, as a sub-agent review caught). - New plugins/_shared/requirements-dev.txt that also lists httpx>=0.27 and httpx-sse>=0.4, so the shared test command 'pip install -r requirements-dev.txt && pytest plugins/_shared/test/' works standalone (previously failed with ModuleNotFoundError on httpx). - Updated plugins/omi-telegram-app/README.md Tests section: install BOTH requirements.txt and requirements-dev.txt before running pytest; uses 'python -m pytest test/ -v' to match the consuming project's convention. - Updated plugins/_shared/README.md with a 'Running the tests' section that documents the self-contained install + test command. Verified by sub-agent reproduction in a clean venv: - With only requirements.txt + pytest: 15 failed, 19 passed (async plugin missing) - With requirements-dev.txt also installed: 34 passed, 0 failed - Full plugin suite: 48 passed
Round 2 follow-up — pytest-asyncio dependency declared (
|
| Step | Result |
|---|---|
pip install -r requirements.txt && pip install pytest && pytest <4 files> (no pytest-asyncio) |
15 failed, 19 passed — exact match to your report |
pip install -r requirements-dev.txt && pytest <same 4 files> |
34 passed, 0 failed |
cd plugins/omi-telegram-app && pip install -r requirements.txt -r requirements-dev.txt && pytest test/ -v (full plugin suite, following new README exactly) |
48 passed |
Stack
- PR head:
e212580f0 - 81 tests green overall (unchanged from round 1)
- No code changes — pure dep + docs
Thanks for catching this; should have caught it in the previous round.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/omi-telegram-app/main.py">
<violation number="1" location="plugins/omi-telegram-app/main.py:93">
P1: `/setup` and `/toggle` endpoints mutate integration state without any authentication or authorization checks, allowing unauthorized configuration changes.</violation>
</file>
<file name="backend/routers/apps.py">
<violation number="1" location="backend/routers/apps.py:1976">
P2: Legacy API keys lack the new 'uid' field, so they may break when the persona-chat auth path enforces key ownership verification.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ic P2) Cubic re-review on commit e212580 flagged a P2 in plugins/_shared/requirements-dev.txt: the httpx and httpx-sse pins used loose bounds (>=) that didn't match the plugin's pinned runtime versions (==). A developer installing the dev reqs could end up with httpx 0.28 in their test env while production runs 0.27.2, risking silent behavior drift between tests and runtime. Fix: - httpx>=0.27 -> httpx==0.27.2 (matches plugins/omi-telegram-app/requirements.txt) - httpx-sse>=0.4 -> httpx-sse==0.4.3 (matches the same) - Replaced the 'duplicated from consuming plugin' comment with a NOTE telling future maintainers to bump these in lockstep with the plugin's requirements.txt in the same PR. Single-file change, no code or test impact. 81 tests still pass.
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-reviewed current head 7e9b4fb after the dev-requirements/docs update.
The implementation-review blocker I raised is now addressed: the plugin and shared test instructions both declare pytest-asyncio, and the dev requirements include the async test plugin plus the shared runtime test deps. I verified the focused plugin/shared suite in a clean isolated venv with the documented requirements:
python -m pip install -r plugins/omi-telegram-app/requirements.txt \
-r plugins/omi-telegram-app/requirements-dev.txt \
-r plugins/_shared/requirements-dev.txt
python -m pytest plugins/omi-telegram-app/test/ plugins/_shared/test/ -q
# 65 passedThe earlier token-leak, uid contract, and test-reproducibility issues appear covered by regression tests now. I’m not seeing another implementation blocker in this pass.
I’m still not formally approving because this PR remains a broad AI-clone/persona-chat feature that can reply on behalf of a user, stores/transmits Telegram bot tokens and Omi API keys, adds dependency/runtime files, and changes backend auth/capability behavior. It still needs human product/security maintainer review before merge, but from this automated maintainer pass the latest fixes are a positive signal.
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Backend+plugin feature: ai-clone persona-chat + Telegram — approve only (re-eval new sha).
3 participants
Summary
First slice of the AI Clone feature, Track 2. Omi can now respond to people on the user's behalf in Telegram, using their persona (memories, recent conversations, tone). The user pastes a bot token, clicks a deep link to bind their Telegram chat to their Omi account, and toggles auto-reply on — every subsequent DM gets a reply from the persona engine.
This PR delivers the backend foundation + first platform (Telegram). Three more platforms + UI + Chat Tools land in subsequent cycles.
What's in this PR
Backend
POST /v2/integrations/{app_id}/user/persona-chat— single-turn persona chat driven by a 3rd-party integration. Auth by app API key (omi_dev_...), rate-limited per (app, user), gated by a newpersona_chatcapability. Streams the reply via the existingexecute_chat_stream— same generator the chat UI uses.PersonaChatRequestPydantic model.app_can_persona_chat(app)capability check (1-line wrapper overapp_has_action(app, 'persona_chat')).Shared
plugins/_shared/persona_client.py— asyncchat()that POSTs to the new endpoint and joins the SSE stream into a single reply string. Returns""on timeout/connect error, raiseshttpx.HTTPStatusErroron 4xx/5xx. Three downstream plugins will reuse this verbatim.Telegram plugin (
plugins/omi-telegram-app/)Mirrors the existing
plugins/omi-slack-app/shape (FastAPI + simple_storage + client wrapper). No new framework, no new abstractions.GET /healthPOST /setup— register a bot token, return a one-time deep link (https://t.me/<bot>?start=<token>)POST /webhook— handles/start <token>handshake, then dispatches every private chat message to the persona when auto-reply is onPOST /toggle— flip auto-reply on/off (called by Chat Tools, lands in a follow-up)Safety (auto-reply)
Operational hygiene
tmp + fsync + os.replace)secrets.compare_digest)…+ warning logexcept Exception)Test plan
65 unit tests, all green. Covers:
Verified no regression to existing backend tests — branch has the same 5 pre-existing collection errors as
main.Deploy notes
Plugin env vars (see
plugins/omi-telegram-app/README.md):TELEGRAM_WEBHOOK_SECRET— required in prod (random if unset, breaks webhook across restarts)OMI_BASE_URL— defaults tohttps://api.omi.meNUDGE_COOLDOWN_SECONDS— defaults to14400(4h)STORAGE_DIR— defaults to/app/dataon Railway (per-instance JSON persistence)The bot token is stored in
users_data.jsonunderSTORAGE_DIR. The file is gitignored.Out of scope (follow-ups)
/toggleendpoint is already there; just needs wiring)Review checklist
main)black --line-length 120 --skip-string-normalization)plugins/omi-slack-app/patterns — no new framework