Skip to content

Dimensional Unified TUI dio#1544

Draft
jeff-hykin wants to merge 27 commits intodevfrom
jeff/feat/dio
Draft

Dimensional Unified TUI dio#1544
jeff-hykin wants to merge 27 commits intodevfrom
jeff/feat/dio

Conversation

@jeff-hykin
Copy link
Member

Draft as a reminder for me later, don't review

Problem

Closes DIM-XXX

Solution

Breaking Changes

How to Test

Contributor License Agreement

  • I have read and approved the CLA.

@jeff-hykin jeff-hykin marked this pull request as draft March 13, 2026 22:03
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR introduces dio, a Dimensional Unified TUI built on Textual, along with a significant rework of how DimOS daemon processes are launched and tracked. The old in-process double-fork approach is replaced by a clean subprocess + double-fork model where the caller writes launch_params.json and returns immediately; the daemon process reads the params, forks, redirects I/O via a new OutputTee, builds the blueprint, and registers itself in a new instance_registry (~/.dimos/instances/<name>/current.json). The old run_registry becomes a compatibility shim. The TUI itself is a multi-panel app with sub-apps for launching, status/log viewing, config editing, dtop, lcmspy, and a chat interface.

Key findings:

  • Bug (theme.py:553): _self.ACCENT = v["dio-text"] should be _self.ACCENT = v["dio-accent"]. Both CYAN and BORDER are correctly mapped to v["dio-accent"] on the adjacent lines, confirming this is a copy-paste error. Every UI element using theme.ACCENT (thinking indicator, chat syntax highlighting, animations) will render in the plain text color instead of the accent color across all themes.
  • Fragile internal API (humancli.py): _ThinkingIndicator._remove() directly mutates RichLog._line_cache, RichLog.lines, and RichLog.virtual_size — all private Textual internals — to surgically remove the animated thinking line. This approach is brittle and will break silently on Textual version updates.
  • Minor teardown gap (daemon.py): On health-check failure, os._exit(1) is called before tee.close(), leaving the OutputTee pipe write-end open at process termination.
  • os._exit(0) in app.py: Intentional bypass of Textual's teardown but also skips all atexit handlers; worth a conscious decision if any cleanup callbacks are registered by imported libraries.

Confidence Score: 3/5

  • Not safe to merge as-is — the theme.ACCENT bug will produce visibly incorrect colors across all themes in the new TUI.
  • The core daemon architecture, instance registry, OutputTee, and CLI refactor are all well-designed and the logic is sound. However, the confirmed ACCENT = v["dio-text"] bug in theme.py is a user-visible regression that will affect every element using theme.ACCENT (thinking indicator, chat highlights, animations) in all six shipped themes. Since the PR is also marked as a draft, the score reflects that one concrete bug plus the private-API fragility in humancli.
  • dimos/utils/cli/theme.py (ACCENT color bug) and dimos/utils/cli/dio/sub_apps/humancli.py (private Textual internals in _ThinkingIndicator._remove).

Important Files Changed

Filename Overview
dimos/utils/cli/theme.py Major rewrite adding named multi-theme support for DIO TUI; contains a confirmed bug where ACCENT is assigned v["dio-text"] instead of v["dio-accent"], causing all accent-colored UI elements to display with the wrong color.
dimos/core/daemon.py Comprehensive rewrite replacing in-process daemonization with a subprocess + double-fork architecture; adds launch_blueprint(), attached_tail(), and _daemon_main(); well-structured with minor issue: tee not closed before os._exit(1) on health-check failure.
dimos/core/instance_registry.py New file replacing the old run_registry; clean implementation of InstanceInfo, CRUD helpers (register, unregister, get, list_running, stop), PID liveness checks, and run directory management; no issues found.
dimos/utils/cli/dio/app.py New DIO TUI main app with multi-panel layout, theme management, sub-app lifecycle, and modal prompt/sudo hooks; ends with os._exit(0) which skips atexit handlers — acceptable but worth noting.
dimos/utils/cli/dio/sub_apps/humancli.py New LCM-backed chat sub-app with thinking indicator animation; directly mutates private Textual RichLog internals (_line_cache, lines) in _ThinkingIndicator._remove(), which is fragile against Textual version updates.
dimos/utils/cli/dio/sub_apps/runner.py New 1231-line status sub-app for monitoring running blueprints; handles multi-instance picker, log tailing, stop/restart/kill controls with sudo escalation, and OutputTee error detection; well-engineered with proper thread-safe UI updates.
dimos/utils/cli/dio/sub_apps/launcher.py New blueprint picker and launcher sub-app; gathers config overrides from ConfigSubApp, runs autoconf, calls launch_blueprint() on a daemon thread, and notifies the StatusSubApp; solid implementation with no issues found.
dimos/utils/prompt.py New three-tier prompt system (DIO hook → rich terminal → auto-accept default) with thread-safe caching by question_id to deduplicate concurrent identical prompts; clean implementation.
dimos/core/output_tee.py New fd-level stdout/stderr tee that fans output to stdout.log (with ANSI) and stdout.plain.log (stripped); runs a reader thread on an internal pipe; error-bounded logging via tee_errors.log; no issues found.
dimos/robot/cli/dimos.py CLI updated to use new instance_registry; run command now has separate daemon and foreground paths; new --daemon, --detach, --name, --force-replace options; adds dio subcommand; clean refactor with no issues found.

Sequence Diagram

sequenceDiagram
    participant CLI as dimos CLI / DIO TUI
    participant LB as launch_blueprint()
    participant FS as Filesystem (~/.dimos/instances/)
    participant SUB as Subprocess (daemon.py __main__)
    participant REG as instance_registry
    participant COORD as ModuleCoordinator

    CLI->>LB: launch_blueprint(robot_types, config_overrides)
    LB->>FS: make_run_dir(instance_name)
    LB->>FS: write launch_params.json + config.json
    LB->>SUB: subprocess.Popen([python, -m, dimos.core.daemon, run_dir], start_new_session=True)
    LB-->>CLI: LaunchResult(instance_name, run_dir)

    Note over SUB: Double-fork → daemon grandchild
    SUB->>FS: redirect stdin→/dev/null, stdout/stderr→stdout.log
    SUB->>SUB: OutputTee.start() (pipes fd1+2 → stdout.log + stdout.plain.log)
    SUB->>SUB: global_config.update(**config_overrides)
    SUB->>COORD: autoconnect + blueprint.build()
    COORD-->>SUB: coordinator ready
    SUB->>SUB: coordinator.health_check()
    SUB->>REG: register(InstanceInfo) → writes current.json
    SUB->>SUB: install_signal_handlers()
    SUB->>COORD: coordinator.loop() [blocks forever]

    Note over CLI: DIO StatusSubApp tails stdout.log in real-time

    CLI->>REG: list_running() / get(name) [polls current.json]
    REG-->>CLI: InstanceInfo(pid, run_dir, ...)

    CLI->>SUB: SIGTERM (stop / force-kill)
    SUB->>COORD: coordinator.stop()
    SUB->>REG: unregister(name) → deletes current.json
Loading

Comments Outside Diff (1)

  1. dimos/utils/cli/dio/app.py, line 2556 (link)

    os._exit(0) skips all atexit handlers and library teardown

    Using os._exit(0) prevents any registered atexit callbacks, Python finalizers, or C-extension cleanup routines from running. For interactive TUI sessions this is usually acceptable (it avoids the hang on sys.exit that Textual sometimes triggers), but any library that registers an atexit handler (e.g., multiprocessing forkserver cleanup, temp-file cleanup, crash reporters) will be silently skipped.

    If the intent is simply to avoid blocking on Textual's internal teardown, consider calling sys.exit(0) and catching the resulting SystemExit before it reaches Textual, or use Textual's own app.exit() and let the framework clean up normally.

Last reviewed commit: 313ed9b

_self.BACKGROUND = v["dio-bg"]
_self.BG = v["dio-bg"]
_self.FOREGROUND = v["dio-fg"]
_self.ACCENT = v["dio-text"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACCENT mapped to wrong theme variable

ACCENT is being assigned the dio-text value instead of dio-accent. Immediately below it, both CYAN and BORDER are correctly assigned v["dio-accent"], which confirms this is a copy-paste error.

All UI elements that use theme.ACCENT — including the "thinking…" indicator in the chat tab, syntax highlighting in the human CLI, gradient stops in animations, and more — will render in the plain text color instead of the intended accent color.

Suggested change
_self.ACCENT = v["dio-text"]
_self.ACCENT = v["dio-accent"]

Comment on lines +112 to +115
self._log.lines = [l for l in self._log.lines if id(l) not in ids]
self._strips = []
self._log._line_cache.clear()
self._log.virtual_size = Size(self._log.virtual_size.width, len(self._log.lines))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direct manipulation of private Textual internals

self._log.lines, self._log._line_cache, and self._log.virtual_size are all private / internal attributes of Textual's RichLog widget. There is no public API contract around them, so any Textual patch release could rename, restructure, or remove them and silently break the "thinking…" indicator at runtime.

If Textual does not expose a public way to remove specific rendered lines, consider a simpler approach: instead of surgically deleting the thinking strip, clear the entire log and re-write the message history when hide() is called (maintaining a _messages list). That removes the dependency on internals entirely.

Comment on lines +334 to +339
info = get(instance_name)
if info is not None:
return info.pid
return None

def _forward_sigint(signum: int, frame: object) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tee not closed before os._exit(1) on health-check failure

When health check fails, os._exit(1) is called without closing tee. The internal pipe's write-end (fd 1 and 2) remains open, and the reader thread in OutputTee may not flush the last bytes to stdout.log before the process terminates.

Since sys.stderr was opened in line-buffered mode (buffering=1) and the error message ends with \n, the data is flushed in practice. But making the cleanup explicit is safer and mirrors the pattern used in the except block below:

Suggested change
info = get(instance_name)
if info is not None:
return info.pid
return None
def _forward_sigint(signum: int, frame: object) -> None:
if not coordinator.health_check():
sys.stderr.write("Error: health check failed — a worker process died.\n")
sys.stderr.flush()
coordinator.stop()
tee.close()
os._exit(1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant