Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR introduces four major chat enhancements: ephemeral message TTL with channel-level settings and automatic background sweeping, an in-app help panel with markdown rendering replacing external links, an all-threads list panel for browsing channel conversations, and image zoom/pan controls in the attachment lightbox. Backend support includes message expiration persistence, thread metadata endpoints, and periodic cleanup loops. Changes
Sequence DiagramssequenceDiagram
participant Client as Frontend:<br/>MessagesApp
participant API as Backend:<br/>Routes
participant Store as Backend:<br/>Message Store
participant Hub as WebSocket Hub
Client->>API: POST /api/chat/messages<br/>(with channel TTL)
API->>Store: send_message(...,<br/>expires_at=now+ttl)
Store-->>API: message with expires_at
API-->>Client: message ACK
Note over Store: Background Loop<br/>(every 5 min)
Store->>Store: sweep_expired()
Store-->>API: [(msg_id, ch_id), ...]
loop For each expired message
API->>Hub: broadcast message_delete<br/>event to channel
Hub-->>Client: message_delete event
Client->>Client: remove message from UI
end
sequenceDiagram
participant User as User
participant Client as Frontend:<br/>MessagesApp
participant API as Backend:<br/>Routes
participant FS as Filesystem
User->>Client: Click "Chat Guide" button
Client->>Client: setShowHelp(true)
Client->>Client: Render HelpPanel
HelpPanel->>API: GET /api/docs/chat-guide
API->>FS: Read docs/chat-guide.md
FS-->>API: markdown content
API-->>HelpPanel: {markdown: "..."}
HelpPanel->>HelpPanel: Parse & render markdown<br/>to React nodes
HelpPanel-->>Client: Rendered HTML
User->>Client: Press Escape or click backdrop
Client->>Client: setShowHelp(false)
Client->>Client: Close HelpPanel
sequenceDiagram
participant User as User
participant Client as Frontend:<br/>MessagesApp
participant API as Backend:<br/>Routes
participant Store as Backend:<br/>Message Store
User->>Client: Click "All threads" button
Client->>Client: setShowAllThreads(true)
Client->>Client: Render AllThreadsList
AllThreadsList->>API: GET /api/chat/channels/{id}/threads
API->>Store: get_channel_threads(channel_id)
Store-->>API: [{id, content, author,<br/>reply_count, last_reply_at}, ...]
API-->>AllThreadsList: {threads: [...]}
AllThreadsList->>AllThreadsList: Render thread rows<br/>with timestamps
AllThreadsList-->>Client: Thread list UI
User->>AllThreadsList: Click a thread
AllThreadsList->>Client: onJumpToThread(parent_id)
Client->>Client: Close AllThreadsList,<br/>Open Thread Panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental UpdateAll issues from previous review have been fixed correctly:
Files Reviewed (2 files changed)
Reviewed by seed-2-0-pro-260328 · 146,198 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
desktop/src/apps/chat/AttachmentLightbox.tsx (1)
70-75:⚠️ Potential issue | 🟡 MinorAdd modal semantics to the lightbox.
This overlay behaves like a blocking dialog, but it is missing
aria-modal="true", so assistive tech can still traverse background content while the viewer is open.♿ Proposed fix
<div role="dialog" + aria-modal="true" aria-label="Image viewer" className="fixed inset-0 z-50 bg-black/80 flex items-center justify-center" onClick={onClose} onWheel={handleWheel}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/chat/AttachmentLightbox.tsx` around lines 70 - 75, The dialog overlay in AttachmentLightbox.tsx (the div with role="dialog" and handlers onClose and handleWheel) is missing aria-modal="true"; update that element to include aria-modal="true" so assistive tech treats it as a blocking modal while the lightbox is open. Ensure the attribute is added alongside role="dialog" and aria-label="Image viewer" on the same div.tinyagentos/routes/chat.py (1)
76-97:⚠️ Potential issue | 🟠 MajorTTL computation is still easy to miss on other message paths.
Expiry is derived only in this websocket branch and the main HTTP branch. The
/helpsystem-message path in this file returns before that logic runs, andtinyagentos/agent_chat_router.py:178-185still callssend_message(...)withoutexpires_at, so some messages in ephemeral channels will never disappear. This needs to be centralized behind a shared helper/service boundary instead of being repeated per route.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/routes/chat.py` around lines 76 - 97, The TTL/expiry computation for ephemeral channels is duplicated and missing on some code paths (websocket message branch, the /help system-message path and the call from agent_chat_router to msg_store.send_message), so extract the logic that reads channel settings and computes expires_at into a shared helper (e.g., compute_expires_at(channel_store, channel_id) or ChannelExpiryService.get_expires_at) that calls get_channel and returns the proper expires_at or None; then call that helper from the websocket message handler (where _ws_ttl/_ws_expires_at are currently computed), from the /help system-message path, and from the code that invokes send_message in agent_chat_router (so every call to send_message uses the helper to pass expires_at consistently).
🟡 Minor comments (7)
docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md-54-54 (1)
54-54:⚠️ Potential issue | 🟡 MinorUse canonical product capitalization for consistency.
Prefer
GitHub:<owner>/<repo>in the scheme description to match naming used elsewhere in the spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md` at line 54, Update the scheme description for the release_source field to use canonical product capitalization: change the example scheme from "github:<owner>/<repo>" to "GitHub:<owner>/<repo>" so the spec consistently uses "GitHub" everywhere (locate the table row for `release_source` and edit the string in that cell).docs/superpowers/plans/2026-04-18-framework-update-phase-1.md-1650-1653 (1)
1650-1653:⚠️ Potential issue | 🟡 MinorBuild instructions use a machine-specific absolute path.
cd /Volumes/NVMe/...is not portable for other developers or CI runners. Prefer repository-relative commands only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-18-framework-update-phase-1.md` around lines 1650 - 1653, The build step uses a machine-specific absolute path ("cd /Volumes/NVMe/Users/jay/Development/tinyagentos") which breaks portability; change it to a repository-relative command or remove the explicit cd and run from the repo root (e.g., use repo root resolution like git rev-parse --show-toplevel or relative paths) so CI/devs can run the script reliably, and keep the subsequent git add lines referencing "static/desktop" and "desktop/tsconfig.tsbuildinfo" as relative paths.docs/superpowers/plans/2026-04-18-framework-update-phase-1.md-1380-1413 (1)
1380-1413:⚠️ Potential issue | 🟡 Minor
onUpdatedis defined but never used in the FrameworkTab plan snippet.After a successful update start, the parent view won’t get notified, so sidebar/store indicators can stay stale until a full refresh.
Suggested change
async function doUpdate() { setSubmitting(true); - try { await startFrameworkUpdate(agent.name); await load(); } + try { + await startFrameworkUpdate(agent.name); + await load(); + onUpdated(); + } catch (e: any) { setErr(String(e)); } finally { setSubmitting(false); setConfirming(false); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-18-framework-update-phase-1.md` around lines 1380 - 1413, The component FrameworkTab receives onUpdated but never calls it; modify the doUpdate flow so the parent is notified when an update is successfully initiated: inside the doUpdate function (after awaiting startFrameworkUpdate(agent.name) and successfully reloading via load()), call onUpdated() (ensure it's invoked only on success, e.g., inside the try block after await load()); this uses existing symbols FrameworkTab, doUpdate, load, and startFrameworkUpdate to locate where to add the call.tests/test_chat_threads.py-241-241 (1)
241-241:⚠️ Potential issue | 🟡 MinorDrop the unused
appbinding in this test.Line 241 unpacks
appbut never uses it, which triggers lint noise and obscures intent.🔧 Proposed fix
- app, client = await _authed_thread_client(tmp_path) + _, client = await _authed_thread_client(tmp_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chat_threads.py` at line 241, The test unpacks an unused app from the coroutine result of _authed_thread_client, causing lint noise; change the unpack to discard the unused binding (e.g., replace "app, client = await _authed_thread_client(tmp_path)" with "_, client = await _authed_thread_client(tmp_path)") so only client is used while keeping the call to _authed_thread_client for setup.tinyagentos/chat/message_store.py-305-308 (1)
305-308:⚠️ Potential issue | 🟡 MinorOnly return rows that were actually soft-deleted.
This loop appends every selected id even if
soft_delete_message()returnsFalse. A concurrent manual delete or another sweep can therefore emit duplicatemessage_deleteevents for the same message.🧹 Proposed fix
- for row in rows: - await self.soft_delete_message(row[0]) - ids.append((row[0], row[1])) + for message_id, channel_id in rows: + if await self.soft_delete_message(message_id): + ids.append((message_id, channel_id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tinyagentos/chat/message_store.py` around lines 305 - 308, The loop currently appends every selected id to ids regardless of the result of soft_delete_message, causing duplicate events; update the loop in message_store.py so that after awaiting self.soft_delete_message(row[0]) you check its boolean return and only append (row[0], row[1]) to ids when soft_delete_message returned True (keep using the same rows variable and ids list and call to soft_delete_message to locate the code).desktop/src/apps/MessagesApp.tsx-175-180 (1)
175-180:⚠️ Potential issue | 🟡 MinorDon't collapse 30 days to
4w.
2592000seconds becomes4where, which underreports the configured TTL by two days. The header badge should preserve exact supported presets like30d.🕒 Proposed fix
function formatTTL(seconds: number): string { - if (seconds < 3600) return `${Math.round(seconds / 60)}m`; - if (seconds < 86400) return `${Math.round(seconds / 3600)}h`; - if (seconds < 604800 * 4) return `${Math.round(seconds / 86400)}d`; - return `${Math.round(seconds / 604800)}w`; + if (seconds % 86400 === 0) { + const days = seconds / 86400; + if (days % 7 === 0 && days < 30) return `${days / 7}w`; + return `${days}d`; + } + if (seconds % 3600 === 0) return `${seconds / 3600}h`; + return `${Math.round(seconds / 60)}m`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/MessagesApp.tsx` around lines 175 - 180, formatTTL currently collapses a 30-day TTL (2592000 seconds) into "4w"; update the formatTTL function to special-case known preset values so they render exactly (e.g., if seconds === 2592000 return "30d") before the existing range checks; reference the formatTTL function and the 2592000 seconds value when making the change so the header badge preserves supported presets like "30d".docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md-56-71 (1)
56-71:⚠️ Potential issue | 🟡 MinorReturn type annotation is incorrect; method returns
list[tuple], notlist[str].The docstring and signature say
list[str], but line 69 appends(row[0], row[1])tuples.Proposed fix
-async def sweep_expired(self) -> list[str]: - """Soft-delete messages past their expires_at. Returns list of deleted ids.""" +async def sweep_expired(self) -> list[tuple[str, str]]: + """Soft-delete messages past their expires_at. Returns list of (message_id, channel_id) tuples."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md` around lines 56 - 71, The return type annotation and docstring for sweep_expired are incorrect: the function actually returns a list of (id, channel_id) tuples. Update the async def sweep_expired signature to return list[tuple[str, str]] (or list[tuple[str, int]] if channel_id is numeric) and revise the docstring to describe "Returns list of (id, channel_id) tuples" so the type and documentation match the behavior invoked when appending (row[0], row[1]) and calling soft_delete_message.
🧹 Nitpick comments (8)
docs/superpowers/specs/2026-04-18-agent-persona-and-memory-deploy-design.md (1)
280-281: Tighten wording in the “Open questions” preface.“None blocking.” reads awkwardly. Consider “Nothing is blocking.” (or “No blocking items.”) for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-18-agent-persona-and-memory-deploy-design.md` around lines 280 - 281, Replace the awkward phrase "None blocking." in the "Open questions" preface with a clearer alternative such as "Nothing is blocking." or "No blocking items." — locate the preface containing the exact text "None blocking." and update that sentence to one of the suggested phrasings to improve clarity and tone.docs/superpowers/plans/2026-04-16-agent-resources-advanced.md (1)
129-130: Use repo-relative commands instead of machine-specific absolute paths.The hardcoded
/Volumes/NVMe/Users/jay/...paths are not reproducible for other devs/CI. Prefer repository-relative commands.Suggested doc tweak
-cd /Volumes/NVMe/Users/jay/Development/tinyagentos/desktop && npx tsc --noEmit 2>&1 | head -20 +cd desktop && npx tsc --noEmit 2>&1 | head -20-cd /Volumes/NVMe/Users/jay/Development/tinyagentos/desktop && npm test 2>&1 | tail -8 +cd desktop && npm test 2>&1 | tail -8Also applies to: 201-202, 395-396, 403-404
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-16-agent-resources-advanced.md` around lines 129 - 130, Replace the machine-specific hardcoded command string "cd /Volumes/NVMe/Users/jay/Development/tinyagentos/desktop && npx tsc --noEmit 2>&1 | head -20" (and the other occurrences with the same absolute path) with a repository-relative invocation; for example, change the CD step to use the repo root (e.g., "cd .", "cd $(git rev-parse --show-toplevel)" or simply run "npx tsc --noEmit 2>&1 | head -20" from repo-relative docs), ensuring all instances (the shown command and the similar occurrences mentioned) use relative paths so the docs/commands work on other developers' machines and CI.docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md (1)
105-110: Add a language tag to the fenced code block.This block is unlabeled; set it to
text(orbash) to satisfy linting and improve rendering consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md` around lines 105 - 110, The fenced code block containing the sample tag pattern starting with "pre-framework-update-<new-tag>-<utc-compact>" is unlabeled; update the opening triple-backtick to include a language tag (e.g., "text" or "bash") so it reads "```text" or "```bash" to satisfy the linter and ensure consistent rendering of that example block.tests/test_chat_threads.py (1)
243-253: Add status assertions for setup requests in the new endpoint test.The new test currently assumes channel/message creation succeeded; if setup fails, later JSON indexing errors make the failure harder to diagnose.
🔧 Proposed fix
ch_r = await client.post("/api/chat/channels", json={"name": "g", "type": "group", "description": "", "topic": "", "members": ["user", "tom"], "created_by": "user"}) + assert ch_r.status_code in (200, 201), ch_r.json() ch_id = ch_r.json()["id"] p1 = await client.post("/api/chat/messages", json={"channel_id": ch_id, "author_id": "user", "author_type": "user", "content": "parent one", "content_type": "text"}) + assert p1.status_code in (200, 201), p1.json() parent_id = p1.json()["id"] await client.post("/api/chat/messages", json={"channel_id": ch_id, "author_id": "user", "author_type": "user", "content": "reply", "content_type": "text", "thread_id": parent_id})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_chat_threads.py` around lines 243 - 253, The test setup is missing assertions on the responses from the POST calls so failures produce confusing JSON errors; after each client.post to "/api/chat/channels" and "/api/chat/messages" (responses held in ch_r and p1, and the subsequent message post) add assertions that response.status_code == 200 (or the expected status) before calling .json(), and optionally assert that ch_r.json().get("id") and p1.json().get("id") are present to ensure ch_id and parent_id exist; this ensures immediate, clear failures if channel or message creation fails.desktop/src/apps/chat/ChannelSettingsPanel.tsx (1)
181-185: Capture previous TTL explicitly before optimistic update.Using the closed-over
ephemeralTtlworks in simple cases, but capturingprevmakes rollback intent deterministic and clearer under async timing.🔧 Proposed fix
onChange={(e) => { + const prev = ephemeralTtl; const val = e.target.value === "" ? null : Number(e.target.value); setEphemeralTtl(val); - apply({ ephemeral_ttl_seconds: val }, () => setEphemeralTtl(ephemeralTtl)); + apply({ ephemeral_ttl_seconds: val }, () => setEphemeralTtl(prev)); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/apps/chat/ChannelSettingsPanel.tsx` around lines 181 - 185, The onChange handler uses the closed-over ephemeralTtl for rollback; capture the previous value into a local variable (e.g., const prev = ephemeralTtl) before doing the optimistic update, then call setEphemeralTtl(val) and call apply({ ephemeral_ttl_seconds: val }, () => setEphemeralTtl(prev)) so the rollback uses the captured prev value deterministically; update references in the ChannelSettingsPanel onChange handler and ensure prev's type matches ephemeralTtl (number | null).docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md (3)
30-35: Silent exception swallowing may hide real database errors.The bare
except Exception: passwill hide issues like database locks, disk full, or permission errors. Consider logging at debug level.Proposed improvement
try: await self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at REAL") await self._db.commit() -except Exception: - pass +except Exception as e: + # Column likely already exists; log for visibility + import logging + logging.getLogger(__name__).debug("expires_at migration skipped: %s", e)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md` around lines 30 - 35, The try/except block that runs self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at REAL") currently swallows all exceptions; update the error handling to catch the exception, log it at debug (or warning) with the exception details and context (e.g., include the SQL and table name) instead of silently passing, and only suppress the error for expected/benign cases (e.g., column already exists) by checking the exception message/type; reference the block around self._db.execute and await self._db.commit() to locate where to add logging and conditional suppression.
60-70: N+1 delete pattern could be slow with many expired messages.The current approach selects all expired messages then deletes them one-by-one. For channels with heavy ephemeral traffic, consider a batch update or wrapping in a transaction for atomicity.
Alternative: batch soft-delete
async def sweep_expired(self) -> list[tuple[str, str]]: """Soft-delete messages past their expires_at. Returns list of (message_id, channel_id) tuples.""" import time as _time now = _time.time() async with self._db.execute( "SELECT id, channel_id FROM chat_messages " "WHERE expires_at IS NOT NULL AND expires_at < ? AND deleted_at IS NULL", (now,), ) as cursor: rows = await cursor.fetchall() if not rows: return [] ids = [(row[0], row[1]) for row in rows] # Batch update instead of N individual calls await self._db.execute( "UPDATE chat_messages SET deleted_at = ? WHERE id IN (%s)" % ",".join("?" * len(ids)), (now, *[mid for mid, _ in ids]), ) await self._db.commit() return ids🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md` around lines 60 - 70, The sweep_expired implementation currently selects expired rows and calls self.soft_delete_message for each row (N+1 pattern); change it to collect ids from the SELECT, then perform a single batch UPDATE on chat_messages setting deleted_at = now WHERE id IN (...) using self._db.execute with parameterized placeholders (and call await self._db.commit()), return the list of (id, channel_id) tuples, and/or wrap the SELECT+UPDATE in a transaction to ensure atomicity; remove per-row calls to soft_delete_message or adjust soft_delete_message to operate on batches if it contains additional side effects.
228-247:SELECT parent.*withGROUP BYis non-standard SQL.SQLite allows
SELECT *withGROUP BY, but the values of non-aggregated columns are implementation-defined. Explicitly listing columns makes the query portable and its behavior explicit.Explicit column selection
- SELECT - parent.*, + SELECT + parent.id, + parent.channel_id, + parent.author_id, + parent.content, + parent.created_at, COUNT(reply.id) AS reply_count, MAX(reply.created_at) AS last_reply_at🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md` around lines 228 - 247, The SQL uses "SELECT parent.*" with a GROUP BY which is non-portable; replace "SELECT parent.*" with an explicit list of the chat_messages parent columns used by the code (e.g., id, thread_id, channel_id, user_id, content, created_at, updated_at, deleted_at — whatever columns exist and _parse expects) and then either include those non-aggregated columns in the GROUP BY or convert them to aggregates as appropriate so the query returns deterministic values; update the query string passed to self._db.execute (the block that binds (channel_id,)) and keep using cursor.description, rows, and _parse as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@desktop/src/apps/chat/AllThreadsList.tsx`:
- Around line 37-48: The effect in AllThreadsList can let an earlier fetch
resolve after channelId changes and call setThreads for the wrong channel;
modify the useEffect to create an AbortController for each fetch, pass
controller.signal into fetch(`/api/chat/channels/${channelId}/threads`, { signal
}), and call controller.abort() in the cleanup to cancel stale requests; also
adjust the .catch to ignore abort errors (check for e.name === 'AbortError' or e
instanceof DOMException) so aborted requests don't call setError, and only
update state (setThreads/setError/setLoading) from non-aborted responses.
In `@desktop/src/apps/chat/HelpPanel.tsx`:
- Around line 50-54: The help panel currently renders the guide source verbatim
using a <pre> wrapper when `!error && markdown !== null`; change this to render
parsed Markdown instead by replacing the raw <pre> output with a
Markdown-to-JSX/HTML renderer (e.g., react-markdown or your app's
MarkdownRenderer component) so headings, lists and links are properly structured
and clickable; keep or map existing styling (the className used on the <pre>) to
the renderer, ensure any HTML is sanitized or use a safe renderer option, and
update the conditional render in HelpPanel (the block that checks `!error &&
markdown !== null`) to render the markdown component fed with the `markdown`
string.
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 1307-1315: The PATCH handler for channel updates in
tinyagentos/routes/chat.py is not persisting ephemeral_ttl_seconds even though
ChannelSettingsPanel.tsx sends it and MessagesApp.tsx reads it; update the PATCH
/api/chat/channels/{channel_id} handler (the channel update function) to accept
ephemeral_ttl_seconds from the request payload, validate it, set
channel.settings['ephemeral_ttl_seconds'] (or the Channel model's
ephemeral_ttl_seconds field) accordingly, and save the channel persistently
before returning the updated channel JSON so the TTL chosen in
ChannelSettingsPanel.tsx survives reloads.
In `@docs/superpowers/plans/2026-04-16-agent-resources-advanced.md`:
- Around line 1-430: The plan file
docs/superpowers/plans/2026-04-16-agent-resources-advanced.md appears unrelated
to this chat Phase 2b PR; remove it from this PR by either moving the file to
the correct feature branch/PR or reverting the addition in this branch so the
chat-only PR stays focused. If this plan was meant here, update the PR
description to explicitly include the AgentsApp.tsx changes and reference key
symbols (AgentsApp.tsx, MEMORY_STEPS_MB, state vars
showAdvanced/advancedLoaded/systemRamMb/systemCpuCores) so reviewers can
validate scope; otherwise stage a git move/checkout to relocate the file and
commit to the appropriate agents/resources PR.
In `@docs/superpowers/plans/2026-04-16-guided-add-provider.md`:
- Around line 197-204: The ProviderCreate schema in the plan is out of sync with
the API model; update the plan's ProviderCreate to match the current contract
used by the route handler (tinyagentos/routes/providers.py) by replacing
name/priority/api_key_secret with the exact fields and types used there (e.g.,
display_name, api_key, auth_token, custom_headers, model, and any
optional/defaults or typing like Optional[str] or Dict[str,str]); ensure field
names, optionality, and defaults mirror the route's ProviderCreate/validation
logic and update any references in the plan to use the same symbol names so
add_provider handling remains compatible.
- Around line 434-451: The code currently swallows failures when calling
fetch("/api/secrets") and proceeds to save the provider, which can leave a
provider without credentials; update the secret-save logic around the fetch call
so failures are treated as blocking: check the fetch response.ok (or catch
network errors) and if not successful throw or return an error to the caller (do
not set apiKeySecret unless the request succeeded), and surface that error to
the UI (or require an explicit user confirmation before proceeding).
Specifically modify the block that builds secretName and calls
fetch("/api/secrets") so that non-2xx responses and exceptions cause early
exit/error (and avoid continuing to provider creation when apiKeySecret is
undefined).
- Around line 818-820: The plan contains a plaintext credential ("SSH:
jay@192.168.6.123 (password: alexander04)") — remove that secret immediately,
replace it with a redacted placeholder (e.g., "SSH: user@host (password:
REDACTED)" or instructions to use an SSH key/secrets manager), and rotate the
exposed password on the affected account; then purge the secret from Git history
using a history-rewrite tool (git filter-repo or BFG) and force-push the cleaned
branch, and notify stakeholders of the rotation. Ensure the change is applied to
the snippet that includes the SSH line and the command block "cd
/home/jay/tinyagentos && git pull origin feat/guided-add-provider && sudo
systemctl restart tinyagentos" so no plaintext credentials remain in current
files or history.
In `@docs/superpowers/plans/2026-04-18-agent-persona-and-memory-deploy.md`:
- Around line 1-13: The plan titled "Agent persona + memory deploy —
implementation plan" was added into a PR focused on "Chat Phase 2b polish",
which is out-of-scope; move this entire plan out of the current PR into its own
dedicated branch/PR (e.g., the feature branch referenced in the text) or convert
it into a clearly marked follow-up artifact by removing it from the change set
and adding a short cross-reference note in this PR that links to the separate
plan PR/issue; ensure the PR only contains Chat Phase 2b polish changes and that
the persona/memory spec reference is preserved in the separate work item for
reviewers.
- Around line 796-814: The DeployAgentRequest Pydantic model uses a mutable
default for fallback_models (fallback_models: list[str] = []), which should be
replaced with a per-instance factory; update the field to use Pydantic's
Field(default_factory=list) and ensure Field is imported, e.g., change the
fallback_models declaration in the DeployAgentRequest class to use
Field(default_factory=list) so each instance gets its own list.
- Around line 575-590: Decide and enforce a single archive API contract (sync or
async) and make all calls consistent: choose whether archive.record /
archive.query are coroutine functions; if you opt for async, change the caller
functions (e.g., the block using self.archive.record, all dual-write tasks and
smoke-checks that call archive.record/archive.query) to async defs, prepend
await to every archive.record/archive.query call, and use try/except with
logger.exception inside async context; if you opt for sync, remove all awaits
and ensure archive is a normal object with synchronous record/query methods and
update type hints and tests accordingly; update any references to
self.archive.record, archive.record, and archive.query across Task 2.1–2.4 to
match the chosen interface.
In `@docs/superpowers/plans/2026-04-18-framework-update-phase-1.md`:
- Around line 1-13: The file "Framework update — Phase 1 Implementation Plan"
belongs to a separate initiative and should be removed from this PR: either
delete or revert this file from the current branch and create a dedicated
branch/PR (e.g., feat/framework-update-phase-1) that contains the plan plus
related artifacts (references to framework_update.py,
docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md, and any
LXC/container helpers) or explicitly add a README link tying this PR to that
initiative; ensure the current PR only contains chat Phase 2b polish changes and
update the PR description to reference the separate framework-update PR if you
intend to keep the plan linked.
- Around line 859-873: The test test_bootstrap_sets_last_seen_at is too
permissive by accepting 401/403; change it so the authenticated path is
exercised and its observable effect is asserted: ensure the request uses a valid
auth setup (or the test fixture provides authenticated headers) so
resp.status_code == 200 and then assert agent["bootstrap_last_seen_at"] is not
None; alternatively split into two assertions/tests: one that sends no/invalid
auth and expects 401/403 and verifies bootstrap_last_seen_at stays None, and a
separate authenticated test that calls GET /api/openclaw/bootstrap?agent=atlas
(using the same app.state.config.agents entry) and asserts resp.status_code ==
200 and that the agent record was updated.
In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md`:
- Around line 56-57: The spec and implementation are out of sync: although the
spec defines service_name (used by systemctl stop/start) and install_script
(invoked via exec_in_container), the runner and install script still infer the
systemd unit from the framework ID; update the update path to pass the explicit
service_name through to exec_in_container and ensure the install script uses
that parameter for systemctl stop/start instead of deriving from framework ID.
Concretely, modify the runner code that calls exec_in_container to include the
service_name field from the update payload, and update the install_script
invocation/argument handling so the script reads service_name and uses it in the
systemctl stop/start commands (replace any logic that maps framework ID → unit
name).
- Around line 157-161: The timeout logic uses a strict > comparison against a
second-resolution deadline which can misclassify reconnects that land exactly on
the boundary; update the timeout check inside _wait_for_bootstrap_ping to use >=
when comparing the current timestamp to the deadline (so equality counts as
timed-out/handled appropriately), and make the same change to the other
occurrence referenced (the check around line 175) so both deadline comparisons
use >=; reference the functions/variables _wait_for_bootstrap_ping, started_at,
deadline, and _mark_failed when locating the checks to change.
In `@tinyagentos/app.py`:
- Around line 327-347: The ephemeral sweep task started with
asyncio.create_task(_ephemeral_sweep_loop(app)) is never tracked or cancelled,
causing a race on app.state.chat_messages and app.state.chat_hub during
shutdown; modify the lifespan logic to save the task reference (e.g., assign the
result of asyncio.create_task to app.state._ephemeral_sweep_task), ensure
_ephemeral_sweep_loop checks for cancellation (it can rely on
asyncio.CancelledError), and in the teardown after the yield cancel and await
the task (call app.state._ephemeral_sweep_task.cancel() and await it, handling
CancelledError) before closing stores so the loop cannot access closed
resources.
In `@tinyagentos/chat/message_store.py`:
- Around line 84-88: The migration currently swallows all exceptions when
running self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at
REAL"), which can hide real DB errors; change the except block to catch the
exception as e, detect SQLite's duplicate-column case (e.g.,
sqlite3.OperationalError with message containing "duplicate column" or "already
exists") and silently ignore only that, but re-raise any other exceptions so
corrupt/locked DB errors surface; ensure sqlite3 is imported and reference the
ALTER TABLE statement, self._db.execute, and the exception variable (e) in your
fix.
---
Outside diff comments:
In `@desktop/src/apps/chat/AttachmentLightbox.tsx`:
- Around line 70-75: The dialog overlay in AttachmentLightbox.tsx (the div with
role="dialog" and handlers onClose and handleWheel) is missing
aria-modal="true"; update that element to include aria-modal="true" so assistive
tech treats it as a blocking modal while the lightbox is open. Ensure the
attribute is added alongside role="dialog" and aria-label="Image viewer" on the
same div.
In `@tinyagentos/routes/chat.py`:
- Around line 76-97: The TTL/expiry computation for ephemeral channels is
duplicated and missing on some code paths (websocket message branch, the /help
system-message path and the call from agent_chat_router to
msg_store.send_message), so extract the logic that reads channel settings and
computes expires_at into a shared helper (e.g.,
compute_expires_at(channel_store, channel_id) or
ChannelExpiryService.get_expires_at) that calls get_channel and returns the
proper expires_at or None; then call that helper from the websocket message
handler (where _ws_ttl/_ws_expires_at are currently computed), from the /help
system-message path, and from the code that invokes send_message in
agent_chat_router (so every call to send_message uses the helper to pass
expires_at consistently).
---
Minor comments:
In `@desktop/src/apps/MessagesApp.tsx`:
- Around line 175-180: formatTTL currently collapses a 30-day TTL (2592000
seconds) into "4w"; update the formatTTL function to special-case known preset
values so they render exactly (e.g., if seconds === 2592000 return "30d") before
the existing range checks; reference the formatTTL function and the 2592000
seconds value when making the change so the header badge preserves supported
presets like "30d".
In `@docs/superpowers/plans/2026-04-18-framework-update-phase-1.md`:
- Around line 1650-1653: The build step uses a machine-specific absolute path
("cd /Volumes/NVMe/Users/jay/Development/tinyagentos") which breaks portability;
change it to a repository-relative command or remove the explicit cd and run
from the repo root (e.g., use repo root resolution like git rev-parse
--show-toplevel or relative paths) so CI/devs can run the script reliably, and
keep the subsequent git add lines referencing "static/desktop" and
"desktop/tsconfig.tsbuildinfo" as relative paths.
- Around line 1380-1413: The component FrameworkTab receives onUpdated but never
calls it; modify the doUpdate flow so the parent is notified when an update is
successfully initiated: inside the doUpdate function (after awaiting
startFrameworkUpdate(agent.name) and successfully reloading via load()), call
onUpdated() (ensure it's invoked only on success, e.g., inside the try block
after await load()); this uses existing symbols FrameworkTab, doUpdate, load,
and startFrameworkUpdate to locate where to add the call.
In `@docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md`:
- Around line 56-71: The return type annotation and docstring for sweep_expired
are incorrect: the function actually returns a list of (id, channel_id) tuples.
Update the async def sweep_expired signature to return list[tuple[str, str]] (or
list[tuple[str, int]] if channel_id is numeric) and revise the docstring to
describe "Returns list of (id, channel_id) tuples" so the type and documentation
match the behavior invoked when appending (row[0], row[1]) and calling
soft_delete_message.
In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md`:
- Line 54: Update the scheme description for the release_source field to use
canonical product capitalization: change the example scheme from
"github:<owner>/<repo>" to "GitHub:<owner>/<repo>" so the spec consistently uses
"GitHub" everywhere (locate the table row for `release_source` and edit the
string in that cell).
In `@tests/test_chat_threads.py`:
- Line 241: The test unpacks an unused app from the coroutine result of
_authed_thread_client, causing lint noise; change the unpack to discard the
unused binding (e.g., replace "app, client = await
_authed_thread_client(tmp_path)" with "_, client = await
_authed_thread_client(tmp_path)") so only client is used while keeping the call
to _authed_thread_client for setup.
In `@tinyagentos/chat/message_store.py`:
- Around line 305-308: The loop currently appends every selected id to ids
regardless of the result of soft_delete_message, causing duplicate events;
update the loop in message_store.py so that after awaiting
self.soft_delete_message(row[0]) you check its boolean return and only append
(row[0], row[1]) to ids when soft_delete_message returned True (keep using the
same rows variable and ids list and call to soft_delete_message to locate the
code).
---
Nitpick comments:
In `@desktop/src/apps/chat/ChannelSettingsPanel.tsx`:
- Around line 181-185: The onChange handler uses the closed-over ephemeralTtl
for rollback; capture the previous value into a local variable (e.g., const prev
= ephemeralTtl) before doing the optimistic update, then call
setEphemeralTtl(val) and call apply({ ephemeral_ttl_seconds: val }, () =>
setEphemeralTtl(prev)) so the rollback uses the captured prev value
deterministically; update references in the ChannelSettingsPanel onChange
handler and ensure prev's type matches ephemeralTtl (number | null).
In `@docs/superpowers/plans/2026-04-16-agent-resources-advanced.md`:
- Around line 129-130: Replace the machine-specific hardcoded command string "cd
/Volumes/NVMe/Users/jay/Development/tinyagentos/desktop && npx tsc --noEmit 2>&1
| head -20" (and the other occurrences with the same absolute path) with a
repository-relative invocation; for example, change the CD step to use the repo
root (e.g., "cd .", "cd $(git rev-parse --show-toplevel)" or simply run "npx tsc
--noEmit 2>&1 | head -20" from repo-relative docs), ensuring all instances (the
shown command and the similar occurrences mentioned) use relative paths so the
docs/commands work on other developers' machines and CI.
In `@docs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.md`:
- Around line 30-35: The try/except block that runs self._db.execute("ALTER
TABLE chat_messages ADD COLUMN expires_at REAL") currently swallows all
exceptions; update the error handling to catch the exception, log it at debug
(or warning) with the exception details and context (e.g., include the SQL and
table name) instead of silently passing, and only suppress the error for
expected/benign cases (e.g., column already exists) by checking the exception
message/type; reference the block around self._db.execute and await
self._db.commit() to locate where to add logging and conditional suppression.
- Around line 60-70: The sweep_expired implementation currently selects expired
rows and calls self.soft_delete_message for each row (N+1 pattern); change it to
collect ids from the SELECT, then perform a single batch UPDATE on chat_messages
setting deleted_at = now WHERE id IN (...) using self._db.execute with
parameterized placeholders (and call await self._db.commit()), return the list
of (id, channel_id) tuples, and/or wrap the SELECT+UPDATE in a transaction to
ensure atomicity; remove per-row calls to soft_delete_message or adjust
soft_delete_message to operate on batches if it contains additional side
effects.
- Around line 228-247: The SQL uses "SELECT parent.*" with a GROUP BY which is
non-portable; replace "SELECT parent.*" with an explicit list of the
chat_messages parent columns used by the code (e.g., id, thread_id, channel_id,
user_id, content, created_at, updated_at, deleted_at — whatever columns exist
and _parse expects) and then either include those non-aggregated columns in the
GROUP BY or convert them to aggregates as appropriate so the query returns
deterministic values; update the query string passed to self._db.execute (the
block that binds (channel_id,)) and keep using cursor.description, rows, and
_parse as before.
In `@docs/superpowers/specs/2026-04-18-agent-persona-and-memory-deploy-design.md`:
- Around line 280-281: Replace the awkward phrase "None blocking." in the "Open
questions" preface with a clearer alternative such as "Nothing is blocking." or
"No blocking items." — locate the preface containing the exact text "None
blocking." and update that sentence to one of the suggested phrasings to improve
clarity and tone.
In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md`:
- Around line 105-110: The fenced code block containing the sample tag pattern
starting with "pre-framework-update-<new-tag>-<utc-compact>" is unlabeled;
update the opening triple-backtick to include a language tag (e.g., "text" or
"bash") so it reads "```text" or "```bash" to satisfy the linter and ensure
consistent rendering of that example block.
In `@tests/test_chat_threads.py`:
- Around line 243-253: The test setup is missing assertions on the responses
from the POST calls so failures produce confusing JSON errors; after each
client.post to "/api/chat/channels" and "/api/chat/messages" (responses held in
ch_r and p1, and the subsequent message post) add assertions that
response.status_code == 200 (or the expected status) before calling .json(), and
optionally assert that ch_r.json().get("id") and p1.json().get("id") are present
to ensure ch_id and parent_id exist; this ensures immediate, clear failures if
channel or message creation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be7ace5a-5fdf-47a2-9fd3-3b4c92074663
📒 Files selected for processing (36)
desktop/src/apps/MessagesApp.tsxdesktop/src/apps/chat/AllThreadsList.tsxdesktop/src/apps/chat/AttachmentLightbox.tsxdesktop/src/apps/chat/ChannelSettingsPanel.tsxdesktop/src/apps/chat/HelpPanel.tsxdesktop/src/apps/chat/__tests__/AllThreadsList.test.tsxdesktop/src/apps/chat/__tests__/HelpPanel.test.tsxdesktop/src/lib/channel-admin-api.tsdesktop/tsconfig.tsbuildinfodocs/superpowers/plans/2026-04-16-agent-resources-advanced.mddocs/superpowers/plans/2026-04-16-guided-add-provider.mddocs/superpowers/plans/2026-04-18-agent-persona-and-memory-deploy.mddocs/superpowers/plans/2026-04-18-framework-update-phase-1.mddocs/superpowers/plans/2026-04-20-chat-phase-2b-2b-ephemeral-polish.mddocs/superpowers/specs/2026-04-18-agent-persona-and-memory-deploy-design.mddocs/superpowers/specs/2026-04-18-framework-update-phase-1-design.mddocs/superpowers/specs/2026-04-20-chat-phase-2b-2b-ephemeral-polish-design.mdstatic/desktop/assets/MCPApp-Bf_LfaaM.jsstatic/desktop/assets/MessagesApp-CkhTvSE9.jsstatic/desktop/assets/MessagesApp-D99HV6_a.jsstatic/desktop/assets/ProvidersApp-BMhQW6pS.jsstatic/desktop/assets/SettingsApp-DtCl5xI-.jsstatic/desktop/assets/chat-nY3Fd9ku.jsstatic/desktop/assets/main-ClVzsOcM.jsstatic/desktop/assets/tokens-C0Jn8tG6.jsstatic/desktop/assets/tokens-CX3OOjWL.cssstatic/desktop/assets/tokens-DbWedNtZ.cssstatic/desktop/chat.htmlstatic/desktop/index.htmltests/e2e/test_chat_phase2b2b.pytests/test_chat_docs.pytests/test_chat_ephemeral.pytests/test_chat_threads.pytinyagentos/app.pytinyagentos/chat/message_store.pytinyagentos/routes/chat.py
💤 Files with no reviewable changes (2)
- static/desktop/assets/MessagesApp-CkhTvSE9.js
- static/desktop/assets/tokens-DbWedNtZ.css
Comment on lines
+56
to
+57
| | `install_script` | Path of the script invoked via `exec_in_container` inside the agent's container. | | ||
| | `service_name` | Passed to `systemctl stop/start` by the install script. | |
There was a problem hiding this comment.
service_name is specified but not actually wired through the update path.
The spec says service_name drives systemctl stop/start, but the runner/script currently infer service from framework ID. That breaks multi-framework compatibility when service unit names diverge.
Proposed spec-level fix
-# Usage: taos-framework-update <framework> <tag> <asset_url>
+# Usage: taos-framework-update <framework> <service_name> <tag> <asset_url>
-FRAMEWORK="${1:?framework name required}"
-TAG="${2:?tag required}"
-URL="${3:?asset url required}"
+FRAMEWORK="${1:?framework name required}"
+SERVICE_NAME="${2:?service name required}"
+TAG="${3:?tag required}"
+URL="${4:?asset url required}"
-log "stopping ${FRAMEWORK}.service"
-systemctl stop "${FRAMEWORK}.service" || true
+log "stopping ${SERVICE_NAME}.service"
+systemctl stop "${SERVICE_NAME}.service" || true
-log "starting ${FRAMEWORK}.service"
-systemctl start "${FRAMEWORK}.service"
+log "starting ${SERVICE_NAME}.service"
+systemctl start "${SERVICE_NAME}.service" rc, stderr = await exec_in_container(container, [
manifest["install_script"],
manifest["id"],
+ manifest["service_name"],
latest["tag"],
latest["asset_url"],
], timeout=120)Also applies to: 146-151, 195-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md` around
lines 56 - 57, The spec and implementation are out of sync: although the spec
defines service_name (used by systemctl stop/start) and install_script (invoked
via exec_in_container), the runner and install script still infer the systemd
unit from the framework ID; update the update path to pass the explicit
service_name through to exec_in_container and ensure the install script uses
that parameter for systemctl stop/start instead of deriving from framework ID.
Concretely, modify the runner code that calls exec_in_container to include the
service_name field from the update payload, and update the install_script
invocation/argument handling so the script reads service_name and uses it in the
systemctl stop/start commands (replace any logic that maps framework ID → unit
name).
Comment on lines
+157
to
+161
| # Wait for the bridge to call bootstrap, bounded by the 120s window from | ||
| # started_at (NOT from now — the script itself may have taken most of it). | ||
| deadline = started_at + 120 | ||
| if not await _wait_for_bootstrap_ping(agent, deadline): | ||
| return await _mark_failed(agent, "bridge did not reconnect within 120s", snapshot=snap) |
There was a problem hiding this comment.
Bootstrap readiness check has a same-second false-timeout edge case.
Using strict > with second-level timestamps can miss a valid reconnect if bootstrap lands in the same second as started_at. Use >= (or compare against a pre-update baseline) to avoid spurious failures.
Also applies to: 175-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-18-framework-update-phase-1-design.md` around
lines 157 - 161, The timeout logic uses a strict > comparison against a
second-resolution deadline which can misclassify reconnects that land exactly on
the boundary; update the timeout check inside _wait_for_bootstrap_ping to use >=
when comparing the current timestamp to the deadline (so equality counts as
timed-out/handled appropriately), and make the same change to the other
occurrence referenced (the check around line 175) so both deadline comparisons
use >=; reference the functions/variables _wait_for_bootstrap_ping, started_at,
deadline, and _mark_failed when locating the checks to change.
Comment on lines
+327
to
+347
| async def _ephemeral_sweep_loop(app: FastAPI) -> None: | ||
| import asyncio as _asyncio | ||
| _store = app.state.chat_messages | ||
| _hub = getattr(app.state, "chat_hub", None) | ||
| while True: | ||
| try: | ||
| deleted = await _store.sweep_expired() | ||
| if _hub is not None: | ||
| for mid, cid in deleted: | ||
| await _hub.broadcast(cid, { | ||
| "type": "message_delete", | ||
| "seq": _hub.next_seq(), | ||
| "channel_id": cid, | ||
| "message_id": mid, | ||
| "deleted_at": __import__("time").time(), | ||
| }) | ||
| except Exception as _e: | ||
| logger.warning("ephemeral sweep failed: %s", _e) | ||
| await _asyncio.sleep(300) | ||
|
|
||
| asyncio.create_task(_ephemeral_sweep_loop(app)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== sweep loop task creation =="
rg -n "_ephemeral_sweep_loop|create_task\\(" tinyagentos/app.py
echo
echo "== explicit cancellation/await sites =="
rg -n "cancel\\(|CancelledError|await .*ephemeral" tinyagentos/app.pyRepository: jaylfc/tinyagentos
Length of output: 392
🏁 Script executed:
# Inspect the lifespan context manager structure around the code
sed -n '310,360p' tinyagentos/app.py | cat -nRepository: jaylfc/tinyagentos
Length of output: 2910
🏁 Script executed:
# Check if other long-running tasks are managed similarly
rg -B 5 -A 10 "create_task.*(_probe_framework_versions|_ensure_agent_image_present)" tinyagentos/app.pyRepository: jaylfc/tinyagentos
Length of output: 1597
🏁 Script executed:
# Look for any task tracking or shutdown logic
rg -n "app\.state\.|tasks|shutdown|lifespan" tinyagentos/app.py | head -30Repository: jaylfc/tinyagentos
Length of output: 1750
🏁 Script executed:
# Get the full lifespan function
rg -A 200 "async def lifespan\\(app: FastAPI\\)" tinyagentos/app.py | head -250Repository: jaylfc/tinyagentos
Length of output: 10083
🏁 Script executed:
# Get lines after 440 to find the yield and teardown
sed -n '440,520p' tinyagentos/app.py | cat -nRepository: jaylfc/tinyagentos
Length of output: 4539
🏁 Script executed:
# Find the yield statement in the lifespan function
rg -n "yield" tinyagentos/app.py | grep -A 5 -B 5 "241"Repository: jaylfc/tinyagentos
Length of output: 44
🏁 Script executed:
# Get a broader view to find where yield is
sed -n '500,600p' tinyagentos/app.py | cat -nRepository: jaylfc/tinyagentos
Length of output: 5768
🏁 Script executed:
# Get the teardown section after yield
sed -n '597,650p' tinyagentos/app.py | cat -nRepository: jaylfc/tinyagentos
Length of output: 2575
Track and cancel the ephemeral sweep task during lifespan shutdown.
Line 347 creates a long-running task that is never tracked or cancelled. During shutdown (after yield at line 597), stores like chat_messages are closed at line 634, but the task's infinite loop may still be accessing them via app.state.chat_messages and app.state.chat_hub. This creates a race condition where the task could attempt to use closed resources.
Store the task reference and cancel it in the teardown section:
🔧 Proposed fix
-from contextlib import asynccontextmanager
+from contextlib import asynccontextmanager, suppress
@@
- asyncio.create_task(_ephemeral_sweep_loop(app))
+ ephemeral_sweep_task = asyncio.create_task(_ephemeral_sweep_loop(app))
@@
- yield
+ yield
+ ephemeral_sweep_task.cancel()
+ with suppress(asyncio.CancelledError):
+ await ephemeral_sweep_task🧰 Tools
🪛 Ruff (0.15.10)
[warning] 343-343: Do not catch blind exception: Exception
(BLE001)
[warning] 347-347: Store a reference to the return value of asyncio.create_task
(RUF006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tinyagentos/app.py` around lines 327 - 347, The ephemeral sweep task started
with asyncio.create_task(_ephemeral_sweep_loop(app)) is never tracked or
cancelled, causing a race on app.state.chat_messages and app.state.chat_hub
during shutdown; modify the lifespan logic to save the task reference (e.g.,
assign the result of asyncio.create_task to app.state._ephemeral_sweep_task),
ensure _ephemeral_sweep_loop checks for cancellation (it can rely on
asyncio.CancelledError), and in the teardown after the yield cancel and await
the task (call app.state._ephemeral_sweep_task.cancel() and await it, handling
CancelledError) before closing stores so the loop cannot access closed
resources.
Comment on lines
+84
to
+88
| try: | ||
| await self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at REAL") | ||
| await self._db.commit() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Don't swallow non-duplicate migration failures.
Catching every exception here and continuing means a locked/corrupt DB can boot without the expires_at column, and the first insert using it will fail later at runtime. Only ignore SQLite's duplicate-column case and re-raise everything else.
🛠️ Proposed fix
try:
await self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at REAL")
await self._db.commit()
- except Exception:
- pass
+ except Exception as exc:
+ if "duplicate column name: expires_at" not in str(exc).lower():
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| await self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at REAL") | |
| await self._db.commit() | |
| except Exception: | |
| pass | |
| try: | |
| await self._db.execute("ALTER TABLE chat_messages ADD COLUMN expires_at REAL") | |
| await self._db.commit() | |
| except Exception as exc: | |
| if "duplicate column name: expires_at" not in str(exc).lower(): | |
| raise |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 87-88: try-except-pass detected, consider logging the exception
(S110)
[warning] 87-87: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tinyagentos/chat/message_store.py` around lines 84 - 88, The migration
currently swallows all exceptions when running self._db.execute("ALTER TABLE
chat_messages ADD COLUMN expires_at REAL"), which can hide real DB errors;
change the except block to catch the exception as e, detect SQLite's
duplicate-column case (e.g., sqlite3.OperationalError with message containing
"duplicate column" or "already exists") and silently ignore only that, but
re-raise any other exceptions so corrupt/locked DB errors surface; ensure
sqlite3 is imported and reference the ALTER TABLE statement, self._db.execute,
and the exception variable (e) in your fix.
- PATCH /api/chat/channels/{id} now accepts ephemeral_ttl_seconds (persist)
- channel_store: set_ephemeral_ttl with validation (<= 30 days, >= 0, null allowed)
- HelpPanel: render markdown as JSX (headers/lists/links/code/bold) instead of raw <pre>
- AllThreadsList: AbortController on fetch to prevent stale responses overwriting current channel
- remove 4 unrelated plans + 2 unrelated specs that were force-added from other branches
(these belonged on their own PRs; includes an earlier credential leak that is now removed)
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
Bundles 4 independent features from the original Phase 2b-2 polish bucket, minus read receipts (dropped — Slack/Discord don't do them; unread badges cover the use case).
Ephemeral messages (Signal/WhatsApp style)
In-app help panel
All-threads list
Lightbox zoom + pan
Test plan
Summary by CodeRabbit
Release Notes