feat(chat): text chat interface alongside voice - #478
Conversation
Hocsman
changed the title
isair
left a comment
isair
left a comment
There was a problem hiding this comment.
Re-posting my earlier review as inline comments so each point is anchored to the code it concerns (the previous blob review is withdrawn).
Overall this is a strong contribution: voice and text correctly share one dialogue memory behind a single query lock, IPC parsing is defensive, input-side redaction is right, Qt threading is done properly, and specs/README/llm_contexts were all updated accurately. Four defects need fixing before merge (see inline), the rest are suggestions.
Non-blocking, not anchored inline: the spec nits in chat_window.spec.md (a showEvent is mentioned but not implemented; "single-line input box" contradicts the multi-line QPlainTextEdit), and three test gaps worth closing (the voice-waits-for-text query_lock() path including release on engine exception; the real broken-pipe recovery closure in _submit_chat_subprocess — the current test re-implements a simplified copy without the try/except; and the shutting-down rejection branch of submit_text_query). Happy to merge promptly once the four inline defects land.
|
|
||
| # Cancel the in-flight chat query (drop the reply), NOT request_stop | ||
| # which would tear down the whole voice assistant. | ||
| daemon.cancel_active_chat_query() |
There was a problem hiding this comment.
Blocking — Stop is a no-op in subprocess mode, and the "stopped" reply still appears. This calls cancel_active_chat_query() in the desktop app's process, but in subprocess mode the query runs in the daemon process, whose _chat_cancel_event is a different module instance — nothing is cancelled. The UI resets the thinking indicator, then the late __CHAT__: complete arrives and _on_complete appends the reply anyway.
Suggested fix: route cancellation over the same stdin IPC as query submission (e.g. a __CHAT_CANCEL__ line handled in stdin_monitor), and drop a complete that arrives after a local Stop. The spec's Cancellation section should describe whichever behaviour ships.
| _notify_chat("complete", None, callbacks=callbacks, use_ipc=use_ipc) | ||
| return | ||
|
|
||
| if is_stop_requested(): |
There was a problem hiding this comment.
Blocking — shutdown races an in-flight chat worker. This guard rejects new submissions once stop is requested, but a worker that started just before stop is unprotected: the shutdown finally runs the final diary update and db.close() without acquiring _chat_query_lock, so a worker still inside run_reply_engine can hit a closed SQLite connection or mutate dialogue memory during the final diary pass.
Suggested fix: in the shutdown path, acquire _chat_query_lock with a bounded timeout (or briefly join the worker) before the final diary update and db.close().
| # on the Qt main thread, never here on the log reader thread | ||
| # (Qt widgets must be created on the GUI thread). | ||
| if line.startswith(CHAT_IPC_PREFIX): | ||
| self._chat_ipc_signals.line_received.emit(line) |
There was a problem hiding this comment.
Blocking — chat reply text leaks into the general log viewer. After routing the __CHAT__: line here, execution falls through to self.log_signals.new_log.emit(line), so the complete event — which carries the full assistant reply, potentially echoing user PII — is rendered in the log window. That undermines the PR's own "IPC carries redacted content" invariant.
| ollama_runtime_ownership=self._ollama_runtime_ownership, | ||
| ) | ||
|
|
||
| def show_runtime_status(self) -> None: |
There was a problem hiding this comment.
Blocking — Runtime Status freezes the tray for up to 5 s. This runs on the Qt main thread and synchronously calls check_ollama_server() (blocking requests.get, 5 s timeout), and loads settings twice along the way. If Ollama is down or the socket hangs, the whole tray UI stalls.
Suggested fix: collect the snapshot on a worker thread and populate the dialog via a signal — the same marshalling pattern this PR already uses for chat signals. At minimum lower the timeout and reuse a single cfg load.
| self.chat_window.raise_() | ||
| self.chat_window.activateWindow() | ||
|
|
||
| def _set_chat_daemon_available(self, available: bool) -> None: |
There was a problem hiding this comment.
Nit: _set_chat_daemon_available is never called (all call sites use _set_chat_daemon_status directly) and it duplicates the mapping in ChatWindow.set_daemon_available. Suggest removing it before someone picks the wrong one.
| from .intent_judge import IntentJudge, create_intent_judge, warm_up_chat_model | ||
| from .intent_judge import ( | ||
| IntentJudge, | ||
| _is_low_power_mode_enabled, |
There was a problem hiding this comment.
Nit: this imports a private helper (_is_low_power_mode_enabled) from intent_judge. Suggest promoting it to a public name — or better, a config-adjacent helper, since it only reads cfg.low_power_mode — so neither module reaches into the other's internals. Similarly, the lazy from ..daemon import query_lock in _dispatch_query deserves a one-line comment noting it avoids the daemon/listener import cycle.
| layout.setSpacing(8) | ||
|
|
||
| # Transcript (read-only) | ||
| self.transcript_widget = QPlainTextEdit() |
There was a problem hiding this comment.
Nit: the transcript grows unbounded over a long session (the window is only hidden on close, never destroyed). Suggest setMaximumBlockCount with a few thousand lines to bound memory.
Superseded by the inline review with line-anchored comments.
…cise full inference pipeline The warmup was sending an empty /api/generate request, which loads the model into memory but does not trigger the full chat-inference pipeline (JIT compilation, KV-cache allocation). The first real /api/chat call with a large prompt (e.g. intent judge system prompt + transcript) could therefore exceed even generous timeouts on laptop GPUs. Switch to a minimal /api/chat with a system + user message pair and num_predict=1, matching the approach already used by OpenAICompatibleBackend.warm_up(). Closes isair#537
… to exercise full inference pipeline" This reverts commit ce9e198.
## Summary Splits the single `/review-pr` skill into two tiers with different token costs: ### `/review-pr` (lightweight) - Single-pass review with a checklist — no sub-agents spawned - Covers correctness, security, performance, maintainability, completeness in one pass - Cheaper and faster; suitable for routine reviews ### `/review-pr-ultra` (heavyweight) - Multi-agent adversarial pipeline: 5 parallel specialist agents + verifier - Same structure as the old `/review-pr` but higher token cost - Use for high-risk changes, security-sensitive areas, or deep-dive requests ### Both skills - Ask for user confirmation before posting to GitHub (Step 5) - Post inline comments as a single review via `POST /reviews` with a `comments` array — not individual `POST /comments` calls (avoids the separate-thread issue from isair#550) - Include verdict-to-event mapping and explicit warnings against the wrong endpoint Closes the skill-update request from the isair#550 review workflow.
Add a text chat window as a first-class sibling to the voice path, closing isair#35. Voice and text share one conversation: the chat window submits through jarvis.daemon.submit_text_query which runs the reply engine on a worker thread with tts=None and the daemon's global DialogueMemory, so a follow-up typed in the chat window continues a voice discussion. Text never speaks. Daemon (src/jarvis/daemon.py): - submit_text_query: fire-and-forget worker that reuses the shared dialogue memory, cfg, and db exposed as new _global_cfg / _global_db globals. One-query-at-a-time lock shared with voice so they cannot race the memory. Per-call callbacks (on_start/on_token/on_tool_call/on_complete/ on_busy) for bundled mode; __CHAT__: JSON events on stdout for subprocess mode. Redaction runs before the start event so the IPC stream never carries raw user text. - handle_chat_query_stdin_line + extended stdin monitor: the desktop app writes __CHAT_QUERY__: lines to the daemon's stdin in subprocess mode; the monitor parses them and routes to submit_text_query(use_ipc=True). Bare SHUTDOWN / EOF semantics unchanged. Monitor now starts on any non-TTY non-bundled stdin, not just Windows. Desktop app (src/desktop_app/): - chat_window.py: PyQt6 QMainWindow (transcript, input, send, stop), shared theme, ChatSignals bridge marshalling daemon-worker callbacks onto the Qt main thread. Enter sends, Shift+Enter for newline. Stop button reuses daemon.request_stop(). Closing hides; does not stop the daemon. Accepts an injectable submit_fn so subprocess mode writes to the daemon's stdin while bundled mode calls the daemon directly. - app.py: 💬 Chat… tray action, lazy show_chat, subprocess stdin-bridge closure, _dispatch_chat_ipc routing __CHAT__: events to the chat window's signals (creating the window lazily so events that arrive before the user opens chat are not lost). Specs & docs: - chat_window.spec.md: full contract (one conversation, daemon entry point, callbacks, IPC, privacy, MVP scope, Phase 2/3 boundaries). - desktop_app.spec.md: ChatWindow added to the windows table and package structure; subprocess IPC section documents the chat lines. - docs/llm_contexts.md: text-chat entry note on context #1 (no new LLM context; planner/router/enrichment/digests run unchanged). - README.md: Text Chat added to Features; the 'voice-only' limitation removed. - CLAUDE.md: chat_window.spec.md registered in the Spec File Registry. Tests: - tests/test_chat_submission.py (11): tts=None + shared memory, callback ordering, None-on-failure, optional callbacks, one-at-a-time busy rejection, __CHAT__: IPC event shape, no-secret-leak invariant, __CHAT_QUERY__: stdin handler (valid / non-chat / malformed). - tests/test_chat_window.py (16): window structure, send dispatch, user echo + input clear + empty guard, on_complete/on_busy slots, stop routes to request_stop, close does not stop daemon, submit_fn injection, desktop app IPC dispatch + subprocess stdin bridge. - tests/test_diary_import.py: fixed a pre-existing test-isolation bug where unconditional sys.modules mocking of PyQt6 at collection time corrupted real Qt classes for every later PyQt6-dependent test. Now attempts a real import first and only falls back to MagicMock when the module is genuinely unavailable. Closes isair#35
.agents/ and AGENTS.md hold local agent-runtime config that is not part of the Jarvis project. Ignore them so they never land in feature commits.
closeEvent only accepted the event without calling hide(), so the window could stay visible when close was triggered programmatically. Add an explicit hide() so the tray's lazy single-instance contract holds and a reply that lands while hidden still reaches the same transcript. Also add a regression test that pins the transcript to the bottom after many appends, so the latest reply stays in view.
Opening the chat window used to show a blank transcript even after a long voice conversation, because the in-memory transcript and the daemon's hot window were never reconciled. The first showEvent now replays the daemon's recent turns (already redacted at memory-insert time) so the user sees context, not a void. Seeding runs once per instance, so re-opening never duplicates turns, and is skipped when the daemon accessor is unavailable. Adds jarvis.daemon.get_hot_window_messages() as the privacy-safe accessor the window reads through.
68d2352 to
e2c08ad
Compare
Hocsman
changed the title
submit_text_query ran redact() on the caller's thread after acquiring the shared query lock but before the worker's try/except, so a redaction failure (or a broken redact import) leaked the lock and orphaned the per-query cancel event. Every subsequent submission was then rejected as busy forever. Move the redact import + call inside the worker's try so the existing finally releases the lock and complete(None) fires on failure. Add a regression test that monkeypatches redact to raise and asserts the lock is released and a follow-up submission is accepted. Also from the adversarial PR review: - treat numpad Enter (Key_Enter) the same as Return for sending - fix spec: the input box is multi-line, not single-line - add cross-path tests proving voice query_lock() blocks while text holds _chat_query_lock, and text is rejected as busy while voice holds it
075622a to
2d40388
Compare
|
@Hocsman There are conflicts now. When it's ready could you re-request the GitHub review from me please and I'll get on it asap. |
Two files disagreed, both about the Ollama warmup, and both were a genuine collision of intents rather than one side being stale. develop moved the warmup from `/api/generate` to `/api/chat`, because an empty generate ping loads the weights without exercising the inference pipeline, so JIT compilation and KV-cache allocation still happened on the first real call and could time it out. This branch had meanwhile made `keep_alive` a parameter, so low power mode can ask for a one minute residency instead of holding the model for half an hour. Keeping either side alone would have silently dropped the other's fix. The resolution takes develop's endpoint and body, including `temperature: 0.0`, and threads this branch's caller-supplied `keep_alive` through it in place of the hardcoded "30m". The spec entry is merged the same way.
**Stop was a no-op in subprocess mode.** The query runs in the daemon, whose module globals are a different instance from the desktop app's, so calling `cancel_active_chat_query()` locally set a flag nobody reads. The UI reset the indicator, then the late `complete` arrived and appended the reply anyway. Cancellation now travels the same stdin pipe as submission (`__CHAT_CANCEL__`), and the window marks the exchange abandoned so the reply is declined when it lands. Both halves are needed: cancellation cannot unwind a request already inside the engine, so the local guard is what actually keeps the answer out of the transcript, and the IPC line is what stops the engine wasting the rest of the turn. **Shutdown raced an in-flight chat worker.** The guard rejected new submissions after a stop request, but a worker that started a moment earlier was still inside `run_reply_engine` holding the connection that the final diary pass uses and `db.close()` shuts. `wait_for_chat_worker` acquires the query lock with a bounded timeout before both, and proceeds anyway on timeout: quitting a moment late beats hanging the quit. **Chat replies leaked into the general log viewer.** The line was routed to the chat window and then fell through to `new_log.emit`, so the `complete` event — the whole assistant reply, potentially echoing what the user typed — was rendered in a window outside the redaction invariant this PR maintains. Chat IPC is now carved out; diary IPC still reaches the viewer, which is what it is for. **Runtime Status froze the tray for up to five seconds.** `check_ollama_server` is a blocking request with a five second timeout, run on the Qt main thread, stalling every menu precisely when the user reached for diagnostics because something looked wrong. The snapshot is collected on a worker thread and rendered through a signal, the same marshalling this PR already uses for chat events. The spec's Cancellation section now describes the behaviour that ships, including why it takes three places and not one, and the IPC section documents the cancel line and the log carve-out.
# Conflicts: # docs/llm_contexts.md # src/jarvis/llm/llm.spec.md # tests/test_diary_import.py # tests/test_voice_listener.py
Unit-marked Qt tests aborted headless CI with SIGABRT (exit 134) because QApplication([]) was constructed with no display and no QT_QPA_PLATFORM. Set the offscreen platform as a default inside the fixture so the real Qt classes can run in CI; machines with a display are unaffected (setdefault).
test_review_478_defects.py had no unit markers, so pytest -m unit deselected every regression test for the four blocking review defects. Mark the file unit (its Qt tests are now headless-safe), and rework the runtime-status test to exercise show_runtime_status's worker-thread dispatch instead of calling the snapshot collector synchronously on the asserting thread, which could not catch a regression.
warm_up_chat_model passes keep_alive unconditionally for Ollama parity, but OpenAICompatibleBackend.warm_up never gained the parameter, so every warmup on an openai_compatible provider raised TypeError, was swallowed, and silently reported failure (first-query latency regression). Accept and ignore keep_alive; pin with unit tests covering both the backend method and the shared helper.
The chat IPC carve-out only filtered __CHAT__: lines, but run_reply_engine unconditionally printed the full reply to stdout, which subprocess mode forwards to the desktop app's log viewer - a surface outside the chat redaction invariant. Add a quiet mode to run_reply_engine and set it from the chat worker, so text-chat replies reach the transcript via callbacks/IPC only. Pin with a quiet=True contract test and the shutdown-rejection branch test (complete(None), engine never called, lock stays free).
A typed chat query must see the same rolling conversation context as a voice follow-up. Pin the chain end to end: the engine injects the shared DialogueMemory's recent turns (written by the audio path) verbatim into the model prompt ahead of the current query. Combined with the existing daemon contract test (engine receives the global memory instance), this proves audio context flows into text chat in both bundled and subprocess mode.
isair
left a comment
isair
left a comment
There was a problem hiding this comment.
Approving current head: all review feedback addressed (headless CI fix, defect regressions in CI, warmup keep_alive regression, log-viewer carve-out, voice-context test) and both checks are green.
## Summary Builds on the text chat interface (#478): the chat window is now a single conversation styled like an SMS thread, with a rewind control on sent messages and a themed runtime status dialog. ## Chat window: one conversation, SMS style - **Single conversation, like a text-message thread with one contact.** No session list, no "new session" button, nothing written to disk. Voice and text keep sharing the daemon's single dialogue memory, so a follow-up typed in the chat window continues a voice discussion. - **SMS look.** Portrait, phone-like window with a contact header (avatar, "Jarvis", and an Online / Typing presence line). Your messages are right-aligned amber bubbles, Jarvis's replies are left-aligned dark bubbles, each with a muted timestamp and no role prefixes. - **Rewind button on every sent message.** Rolls the conversation back to that message (transcript and daemon memory), then re-submits the same text so a fresh reply is generated through the normal complete path. Disabled while a query is in flight or the daemon is stopped. - **Tray label** is now `💬 Chat` (no ellipsis); the subprocess chat control closure only writes the `__CHAT_REWIND__:` line. ## Fast-stop removal The `Force Quit (no saving)` tray action is removed. Stop Listening already stops the listener immediately and then saves the diary + knowledge graph, so the skip-diary path added nothing. The recently-added `skip_diary_update` argument is removed end-to-end: `request_stop()`, `stop_daemon()`, the `SHUTDOWN_SKIP_DIARY` stdin command, and the shutdown skip flag are gone. The daemon's shutdown block always runs the final diary save. ## Daemon + IPC - The rewind path stays: `rewind_chat_to_user` (bundled) / `__CHAT_REWIND__:` (subprocess), guarded by the shared query lock. - The daemon-side session API (`new_chat_session`, `set_chat_messages`, `__CHAT_NEW_SESSION__` / `__CHAT_RESTORE__:` handlers) remains as a tested daemon capability but is no longer used by the desktop UI, so the feature can return without daemon changes. ## Tests Unit coverage for the SMS layout (bubble alignment, presence line, single conversation accumulation, timestamps), rewind, the lock guard, and the rewind IPC handler; session tests replaced, fast-stop tests removed. Full unit suite: 2259 passed, 4 pre-existing macOS-environment failures (`test_macos_crash_report.py` runs its darwin-only tests unconditionally on Windows). ## Spec & docs `chat_window.spec.md` rewritten to the single-conversation SMS design; `desktop_app.spec.md`, `docs/llm_contexts.md`, and the README updated to match.
3 participants
Summary
Adds a text chat interface alongside the existing voice path, plus a set of desktop runtime-quality-of-life improvements that landed on this branch. Voice remains the primary modality; text is a first-class sibling that shares the same conversation (the daemon's global dialogue memory), the same tools, and the same diary.
Text chat interface
ChatWindow(desktop_app.chat_window): transcript + single-line input (Enter sends, Shift+Enter newline) + Send + Stop. Styled via the shared theme. Opened from a💬 Chat...tray entry; created lazily and kept alive for the session.jarvis.daemon):submit_text_query(text)— fire-and-forget; runsrun_reply_enginewithtts=Noneand the shared dialogue memory on a worker thread. One query at a time via a shared voice+text lock; a concurrent submission is rejected with abusyevent, not queued.cancel_active_chat_query()— per-query cancellation flag so the Stop button drops the in-flight reply (complete(None)) without tearing down the voice listener (distinct fromrequest_stop).get_hot_window_messages()— privacy-safe accessor the window reads to seed its transcript on first open (content is already redacted at memory-insert time).__CHAT__:/__CHAT_QUERY__:JSON IPC on stdout/stdin.run_reply_enginebefore the query reaches the model or the dialogue memory. The transcript is in-memory only; the diary remains the single durable record and sees only redacted text.__CHAT__:IPC lines carry only the redacted query.Lifecycle & UX hardening (this update)
closeEventnow explicitly hides: previously only accepted the event, so a programmatic close could leave the window visible. Now callshide()so the tray's single-instance contract holds and a reply landing while hidden still reaches the same transcript.Desktop runtime improvements
cfg.low_power_modeskips startup LLM warmups and shortens Ollamakeep_alivefrom30mto1mfor intent-judge and warmup calls. Does not change prompts, model selection, timeouts, or context limits.request_stop(skip_diary_update=True)skips the final shutdown diary LLM pass for the explicit fast-stop UI path.Tests
Full unit coverage for the chat window (structure, send, callbacks, stop, lifecycle, IPC dispatch, daemon-status, input keys, transcript scroll, close, hot-window replay) and the daemon submission path (contract, concurrency, IPC, stdin handler, shutdown modes, hot-window accessor). All conflict-touched suites (
test_desktop_app,test_llm_backend,test_intent_judge,test_voice_listener) pass after rebase; the single pre-existingtest_voice_listeneraudio-timing failure is unrelated and fails identically ondevelop.Closes the text-chat milestone.
Spec & docs
chat_window.spec.mddocuments the full contract (new: first-show hot-window replay lifecycle).desktop_app.spec.md,listening.spec.md,llm.spec.md, anddocs/llm_contexts.mdreflect the runtime/low-power/owned-runtime additions.Review fixes (maintainer push, Aug 2026)
qappfixture now setsQT_QPA_PLATFORM=offscreenso the unit-marked Qt tests run headless instead of aborting (SIGABRT);test_review_478_defects.pyis now@pytest.mark.unitso the four defect regressions actually run in CI.OpenAICompatibleBackend.warm_upaccepts (and ignores) thekeep_alivekwarg, sowarm_up_chat_modelno longer fails on everyopenai_compatibleprovider.run_reply_enginewithquiet=True, so the full reply no longer lands in the general log viewer via the engine's stdout print.develop(incl. test: fix full-suite failures on Windows (environmental test bugs) #580's scoped test-diary stubbing) and resolved thetest_diary_import.pyconflict, keeping the test: fix full-suite failures on Windows (environmental test bugs) #580 fixture.submit_text_query, and a runtime-status test that actually pins the worker-thread snapshot dispatch.