Repository navigation
feat(herdr): run og inside a terminal multiplexer - #21
Open
jmvbambico wants to merge 78 commits into
Open
jmvbambico wants to merge 78 commits into
jmvbambico wants to merge 78 commits into
Conversation
Stdlib-only NDJSON-over-AF_UNIX client for herdr 0.9.3: socket path resolution (arg > HERDR_SOCKET_PATH > HERDR_SESSION > default), request framing with monotonic ids, result unwrapping, HerdrError for both error frames and transport failures, wrappers for the workspace/tab/pane/agent methods, and an events.subscribe generator that skips unsolicited event frames on the shared connection.
Projects live Omnigent sessions into herdr panes: one tab per session whose pane runs `omnigent attach <session_id>`. Adds installer/og_herdr.py (the bridge + CLI) and an `og herdr` subcommand in bin/og that forwards to it. The two low-level sibling modules (og_herdr_client, og_herdr_watch) live on other branches, so the bridge imports them lazily and the tests inject fakes — nothing touches a real socket or server. The bridge is idempotent per session, tolerates HerdrError without aborting, and by contract only REPORTS the blocked state: it never resolves an elicitation, which would race the user's own client for Omnigent's single answer Future. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Polls GET /v1/sessions?kind=any (the server default hides sub-agent workers) and diffs the listing into added/removed/changed SessionEvents. Also parses the per-session SSE tail with an incremental parser, so a frame split across reads still dispatches. No herdr knowledge, no websocket dependency, no token ever logged.
sun_path in an AF_UNIX sockaddr caps at 104 bytes on macOS and 108 on Linux. pytest's tmp_path is already long on a stock macOS TMPDIR (/private/var/folders/<..>/T/ + pytest-of-<user>/pytest-N/<test-name><n>/), so parametrised names such as test_report_agent_rejects_invalid_state_before_socket[IDLE] pushed the bind past the cap and every test errored with 'AF_UNIX path too long' before its body ran -- a flake that only showed up off the author's short-TMPDIR machine. Give each fake its own tempfile.mkdtemp(dir="/tmp") with a 6-char socket name (~25 bytes, independent of the test name) and remove it in close(), so nothing is left behind. Adds a test asserting the path still fits the 104-byte cap.
Captures what was measured against Omnigent 0.17.0 and herdr 0.9.3 while scoping this: why the pane command is `omnigent attach`, why multiple clients are safe, why agent state is pushed rather than detected, and the three things that cannot work (replacing Omnigent's tmux layer, detecting an agent through a nested tmux attach, giving an ACP worker a terminal). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_module_imports_without_sibling_modules asserted the sibling modules were ABSENT from disk — true only while the fan-out branches were separate, and inverted the moment they batch together. It was never evidence of the property that matters: that og_herdr.py imports cleanly when a sibling is missing. Rewrite it to parse og_herdr.py with ast and split its imports by where they execute, asserting none of the sibling imports is module-level while the ones inside function bodies are present (a positive control so deleting them cannot pass vacuously). This is independent of file layout and of test execution order, so it holds whether or not the siblings are on disk. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
… event Three blocking findings, all about partial failure while handling a single event: - `_added` now records the tab/pane mapping immediately after `tab_create` and tracks setup progress, so a failure in pane_run/report_agent/ report_metadata no longer leaves the session unmapped. A redelivered `added` resumes the remaining steps on the existing pane instead of opening a second tab (Finding 1). - `tab_create` results are validated before use: a reply without usable `tab_id`/`pane_id` stores no mapping and reports a reconcile-style error, and a named tab with no pane is closed best-effort so it is not orphaned (Finding 2). `_id_of` now only checks the real `pane_id`/`tab_id` keys. - `_removed` runs release_agent/tab_close before dropping the mapping, so a failed cleanup survives for a retry; a `not_found` reply is treated as already-gone success (Finding 3). Tests: fakes now return the real `tab_id`/`pane_id` keys; added coverage for partial-setup retry, unusable ids, orphan-tab cleanup, failed removal retry, and not_found removal. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
The old test_events_do_not_alias_server_objects asserted on event.previous, which poll_once builds from the *first* poll's stored object — a dict unrelated to the one it mutated. It passed with the copies removed from poll_once, so it reported coverage that did not exist. Mutating the fixture listing proves nothing either: StubOpener JSON-encodes it, so poll_once sees a fresh decode. Reach the real object through watcher._seen instead, rewrite it after the poll, and assert the event still reports the poll-time values; then assert neither event dict is that object. The second direction gets its own test: scribbling on event.session must not edit the watcher's retained state into a fabricated `changed` event. Both go red when the corresponding dict() in poll_once is removed. Also drops the sys.path insert and the now-unused `sys` import: the root conftest.py already puts installer/ on the path, which is how the other test modules import their module.
An independent reviewer raised one blocking and two non-blocking findings
against the bridge removal fixes.
- _removed wrapped release_agent and tab_close in one try, so a `not_found`
from release_agent skipped tab_close entirely and then dropped the mapping,
orphaning an open tab nothing tracked. A `not_found` from release_agent only
means "no marker to release" (setup may have failed part-way), not that the
tab is gone. The two calls are now handled separately: release_agent
`not_found` is nothing-to-do and we PROCEED to tab_close; only a `not_found`
from tab_close drops the mapping; any other error keeps it for a retry.
- _id_of iterated ("pane_id", "tab_id") for both sub-objects, so it was only
accidentally correct — the live root_pane carries BOTH keys. The caller now
names the id it wants, and the live shape is recorded in the docstring.
- _reject_partial_create could only clean up an orphan tab when a tab id was
present; a pane id without a tab id left the pane behind. That case now
closes the pane best-effort with the same record-don't-raise handling.
Adds tests for both not_found directions, the retry-after-tab_close-failure
path, key-explicit _id_of lookups on the real shape, and both asymmetric
partial-create cleanups. Stdlib only; the AST test proving the bridge never
resolves an elicitation still passes.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
…rror frames
The test suite was green because the fake server encoded a wrong belief about
herdr. All three defects below were measured against a live herdr 0.9.1 server
by probing it directly; the fake now models what the server actually does.
1. herdr serves ONE request per connection and then hangs up, whatever the
method. Measured: workspace.list x3 on one connection -> reply, EOF,
BrokenPipeError; ping then agent.list -> pong, BrokenPipeError. The client
reused one connection, so every second call failed — for the bridge's
four-call session (tab_create, pane_run, report_agent, report_metadata) that
was alternating failure on every event. call() is now one connection per
request: connect, send, read, close. It also sets aside a live subscription
socket rather than borrowing and closing it. events_subscribe keeps its own
connection, since a subscription is the connection. close() and the context
manager still work. Module docstring, class docstring and the _write
no-retry comment rewritten to say what is true now.
2. events.subscribe takes `subscriptions`, not `types`. Measured: `{}` and
`{"types":[...]}` are both rejected with "missing field `subscriptions`";
entries are internally-tagged objects, a per-type entry also needs `pane_id`,
and `[]` is accepted as the catch-all. The parameter is `subscriptions`,
entries pass through verbatim, the default is `[]`, and `types=` is kept as a
source-compatible alias. All of it documented in the docstring.
3. A request herdr cannot parse is answered with `"id": ""`, not our request
id. _await_reply skipped it on the id mismatch, read EOF, and reported
`disconnected` — discarding the server's actual message. An empty-or-absent
id on a frame carrying `error` is now raised as the answer to the in-flight
request. Also skip any frame carrying an `event` key before the id check:
defensive, not observed (ids are client-generated and monotonic), but it
removes the class for one condition. Same empty-id carve-out applied to the
subscribe ack loop.
Tests: the fake closes after every reply (real behaviour), which is the change
that would have caught #1. Added four-calls-in-a-row regression, the exact
subscribe params frame plus the `[]` default and the `types=` alias, the
empty-id error frame on both the call and subscribe paths, an event frame
carrying the awaited id, and a call not disturbing a live subscription. The
hand-written peer-close test became a hangup-before-reply test — against a
server that hangs up after every reply, "the second call fails" is no longer
what a correct client does. RESULTS["tab.create"] and test_tab_create updated
to the real reply shape (tab.tab_id / root_pane.pane_id, root_pane carrying
both). Dropped the unused `client` fixture from the tests that build their own.
Gate: /opt/homebrew/bin/pytest tests/ -q -> 453 passed.
…okens to the server Four findings from review of installer/og_herdr_watch.py: * watch() called poll_once() bare, and the only production caller (og_herdr.py's run_forever) wraps nothing — one refused connection or malformed listing stopped the daemon until someone noticed and restarted it. The loop now reports the failure on stderr (URL userinfo scrubbed, token never printed), retries on a backoff that doubles to MAX_POLL_BACKOFF and resets on the first success, and resumes yielding on its own. poll_once() stays strict; _seen is replaced as its last statement, so a failed poll leaves the last observed state intact and recovery does not emit a removed/added storm for every live session. * _SSEParser grew without bound on a peer that never sent a newline or a dispatching blank line. Both buffers are now capped; over the cap the pending junk is dropped (resynchronising on the line terminator) rather than raised, because a live tail must survive one malformed frame. * Bare-CR line endings were not parsed, though the docstring claimed them. Fixed rather than documented away: CR, LF and CRLF all terminate a line, with a lone CR at the end of a buffer held back so a split CRLF is not read as two terminators. * discover_token sorted for a loopback key and returned the first usable token, so a store holding only a remote server's token sent that bearer to the loopback server. It now matches the token to the base URL it was issued for and returns None when nothing matches; the loopback preference is gone because a matching key is by definition this server. The dict-subclass concern in herdr_state is not addressed: every session here comes from json.loads, which only produces plain dicts.
events_subscribe advertised types=[<name>, ...] and mapped the names into
subscriptions, but a list of strings is precisely what herdr refuses:
{"subscriptions": ["pane.agent_status_changed"]}
-> invalid type: string, expected internally tagged enum Subscription
So the alias could only ever produce a request guaranteed to be rejected on
the wire, and the rejection surfaced at stream time as a server error rather
than at the call site. There is no in-repo caller of the alias, so keeping
it preserved compatibility with a call that could not work.
The parameter is gone; events_subscribe(subscriptions=None) only, still
defaulting to the measured catch-all []. The docstring keeps all four
measured wire results and now states why no types= spelling exists.
The alias test becomes one that pins the supported form: subscriptions
entries are sent through verbatim, and passing types= raises TypeError at
argument binding with nothing written to the socket.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three facts about the herdr socket were only discovered by probing a running server, each after passing a full suite against a fake built on the opposite assumption: a connection serves exactly one request, events.subscribe takes `subscriptions` rather than a list of type names, and an unparseable request answers with an empty id. The first would have broken the feature outright. Also records the operational behaviour the bridge needs in order to run for days (poll backoff, SSE buffer caps, per-server token matching, resumable pane setup) and the non-blocking findings review left for later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… field
`_added` read `session.get("workspace")`, but a live session row has no
`workspace` key at all, so the lookup always yielded None and every pane
opened in $HOME instead of the repo the session works in. The `or` hid the
missing field behind what looked like a deliberate default.
Drop the lookup. Add `--cwd` (default: the process cwd) threaded through
Bridge as `cwd=None`, falling back to os.getcwd(); `og herdr` runs from the
repo whose sessions it projects, so that is the directory to use. Record why
the cwd comes from the invocation rather than the row, citing the live-server
verification so nobody goes looking for the field again.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Four defects, all from the watcher being written against a contract that was
taken from sys_session_get_info's MCP output instead of the REST API.
* The elicitation field is PLURAL. The row carries
pending_elicitations_count; reading pending_elicitation_count meant the key
never matched and "blocked" never fired — the one state a human most needs
to see, silently dead. Plural decides; the MCP spelling stays a fallback for
a caller that hands us a session dict from that tool.
* No pagination, so sessions silently vanished. The listing is newest-first
with a default page of 20: the root session — the conversation the user is
driving — was pushed off page 1 and never seen, and a session crossing the
page boundary between polls looked exactly like a deletion, which the bridge
turns into a closed tab. list_sessions now walks ?after=<last_id> while
has_more is true, asking for limit=100 explicitly, bounded by
MAX_SESSION_PAGES=10 and MAX_LISTED_SESSIONS=1000 (order-of-magnitude
headroom over the ~100 sessions a busy machine holds) so one poll cannot be
made unbounded.
* Nothing filtered dead sessions: all 91 idle and 8 failed ones here would
have been projected. New module-level should_project() decides what is worth
projecting and poll_once applies it, so the bridge only ever sees the live
set: a root while listed and not archived (it reads idle the whole time it
waits for the human to type — retiring it would close the pane they are
typing into), a sub-agent only while running or holding elicitations.
* The envelope and id keys were guesses that happened to work through
("sessions", "items", "data") and ("session_id", "id") fallbacks. data and
id are now the documented primary path, pinned by tests built from a row
captured off a live 0.17.0 server, and the module docstring records the
measured envelope and row keys — replacing the list that claimed a row has
session_id, kind and workspace. It has none of them.
Unchanged: bounded poll backoff, SSE caps, bare-CR handling, per-server token
matching, event copy semantics. Still no herdr in this module, stdlib only,
no test opens a socket.
The cap left a residual from the last round: when the walk stopped early, the rows it never fetched were indistinguishable from deleted ones, so poll_once reported them removed and the bridge closed their tabs. On this machine the first rows to fall past the cap are the oldest — the roots, the panes the user is typing into — so the symptom the cap was supposed to avoid was reachable from the cap itself, at a threshold it merely delayed. A truncated listing now says so, and that changes what an absence means: * _fetch_listing returns (rows, truncated) and reports every way the walk can stop short — page cap, row cap, an envelope promising more with no cursor to continue with, a cursor the server repeats. list_sessions keeps its list return and delegates. * poll_once emits no removals on a truncated poll, and merges the fresh rows over _seen instead of replacing it. Replacing would only move the damage: this poll's phantom removals become the next complete poll's phantom `added`, and the consumer opens a second tab for a session whose pane is already there. `added` and `changed` keep flowing from the rows that were fetched, so only the absent-based events go quiet. * One stderr line on entering the truncated state and one on leaving it, and nothing in between: a watcher polling every few seconds in a permanently truncated state would otherwise bury the rest of the log. The cost is named in poll_once's docstring rather than hidden: a session that finished during a truncated window keeps a stale pane until a complete listing arrives, and if no listing is ever complete again, removals never resume. That is a stale pane instead of a wrongly closed one, and the stderr line is what makes the condition findable. Unchanged: the plural-primary elicitation count and its documented singular fallback, cursor pagination with the three caps, should_project, the measured envelope/row shape, bounded poll backoff, SSE caps, bare-CR handling, per-server token matching, event copy semantics, no herdr in this module.
The session listing was written up here from sys_session_get_info's MCP
output rather than the REST response, and the wrong version reached all
three implementation contracts. Measured against a live 0.17.0 server, a
row spells the field pending_elicitations_count (plural, so the singular
key never matched and `blocked` could never fire), keys the id as `id`,
carries no `kind` and no `workspace`, and arrives in a paginated
{"object":"list","data":[...]} envelope with no server-side status filter.
Records why pagination is a correctness problem rather than a performance
one, why truncation must suppress removals and merge rather than replace,
that the suppression depends on the listing being newest-first, and which
sessions earn a pane.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two blocking defects from the independent review.
* poll_once suppressed EVERY removal on a truncated poll, which conflated two
absences that mean opposite things. A row it fetched and rejected — a
sub-agent that went idle, a row that came back archived — is direct evidence
the session is done, and suppressing it left a finished worker holding a
stale pane for as long as the listing stayed too big. Absence is now read in
three states: fetched-and-rejected retires (truncated or not), never-fetched
is kept and emits nothing, and unchanged keeps flowing as before.
poll_once therefore tracks every id that came back, not only the ones that
passed should_project. On a duplicated id the last row wins, for the
retained object and the verdict alike, which is the rule fresh already
applied to the object; the id keeps its first appearance's position so event
order still matches listing order. The merge still preserves never-fetched
ids, and now explicitly drops the retired ones so the next poll does not
retire them a second time.
* The SSE frame cap cleared the data buffer at the line that broke it but did
not mark the frame as finished, so the rest of that frame accumulated as
though it were fresh and the next blank line dispatched it: an oversized
frame followed by `data: {"ok": true}` handed the consumer a fragment it
cannot tell from real data, and the consumer is a UI that acts on what it
gets. The cap now poisons the frame, _line swallows every line until the
blank line that would have dispatched it, and that boundary clears the flag
so the next frame — multi-line included — parses normally. flush() refuses
to dispatch at all while a frame is poisoned, so a stream that ends
mid-discard cannot turn its remainder into a frame.
Six new tests cover each behaviour. Verified against the pre-fix module: seven
of the new tests fail there, including the reviewer's exact SSE example.
Noted in passing: two of the flush tests were written calling feed() without
draining it — a generator, so the parser was never fed and the tests passed
vacuously. Fixed, and the comment in the test says why the drain matters.
Pending-by-cwd with existence-based pruning (no TTL to tune), og agents scoped by cwd the same way the launcher is, and warn-and-proceed for a missing multiplexer. Records why the first could not go the other way: og chat is `exec omnigent run`, so the session id does not exist when the launcher needs it, and the launcher ends in `exec herdr` so it cannot wait to learn it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Frames multiplexer support as a catalog the way supported agents already are, and says plainly what a registry row buys -- detection and listing, not a working backend. The Agent status column is the point: the backend contract is capability-based, not lowest-common-denominator. create/close/run/focus/ rename are required, report-agent-state is optional. herdr has it; tmux would not, so a tmux backend gives named windows and nothing more. Also flags the naming hazard for a future tmux backend: tmux is already a required dependency, since Omnigent runs every native agent terminal in its own private tmux server, so `og start tmux` would make the word mean two things in one tool. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`og agents` renders the orchestrator (a ROOT session) and its delegated workers, live. The listing drives the view; the 121 KB DETAIL row is read lazily, only for the report snippet and the root's workspace/harness. The renderer is a pure function from state to lines, so `--once` drives it with no tty and curses only paints the same list. Nothing is ever written to a session: the module only GETs. Budget: one listing walk per refresh, plus detail fetches only for (1) a live root's first sighting, (2) a worker's first idle report, (3) the selected row. Cached by session id, so a stable listing costs nothing after the first refresh. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
The state file moves from `{"version": 1, "workspaces": {ws_id: sid}}` to
`{"version": 2, "spaces": {sid: {...}}, "pending": {cwd: {...}}}`.
v1 is keyed by workspace id, which answers "is this space ours?" — all that
--cleanup needs — but not "does this session already have a space?", which is
the whole of adoption. Once `og start <mux>` exists, a space may be on screen
for a session this process has no memory of: the launcher made it, or the
bridge restarted and its map died with the process. Either way the old code
opened a second space for one conversation.
v1 is UPGRADED IN PLACE at read time, never discarded: a v1 file can describe
spaces live on the operator's screen, and dropping it orphans them from
--cleanup. The upgrade is a transpose, so the workspace ids survive, and it
records only what v1 held — no tab id, no pane id, none invented, because a
guessed pane id is exactly what a stale adoption hands to every later
report_agent.
In `_added`, in order: already in `_recs` (unchanged), then ADOPT from
`spaces[sid]`, then CLAIM a `pending[cwd]` whose directory matches this
session's live `workspace`, then create. Roots only — a worker that reached
the launcher's directory before its root would otherwise claim the launcher's
whole space and leave the root to open a second one. `owner` survives a claim:
it records who opened the space, not who is looking at it.
Every adopt and claim requires the workspace to appear in workspace.list, and
`workspace.list` answers THREE ways — present, absent, or "could not read".
Only the first two are answers; the third adopts nothing and prunes nothing,
because treating unknown as absent would turn one transient herdr hiccup into
a duplicate space for every live session. A stale record is dropped and a
fresh space created: a dead pane id fails silently forever, a duplicate space
the operator can close.
Adoption runs no setup steps. The pane is already live and already running
what it should; re-running would type the attach command a second time, and
for a launcher's space that is a second co-drive client on one session — the
race `_state` exists to avoid. What it does instead is rename the space to
the session title, which is the launcher's entire reason for labelling from
the directory.
Pending records are pruned by EXISTENCE — drop any whose workspace is not in
workspace.list. No TTL and no clock: a space closed by hand must stop being
claimable immediately, and a clock would need a field only time could advance.
--cleanup closes both halves, for the same reason: a pending record is a space
something opened on the operator's behalf, and leaving it behind means
`og start <mux>` has no way back.
Two bugs the new tests caught while being written: a worker's removal
forgetting its ROOT's space record, and a dry run writing the state file it
had just read (which would consume the pending record the next real run
needed). Both fixed, both now guarded.
Add a `multiplexers` catalog to registry.json as a sibling of `agents`, with one herdr row and a `$multiplexers` comment stating plainly that a row buys detection and listing only -- the backend code still has to exist per multiplexer, exactly as an agent row needs a harness plugin. tmux is deliberately absent (no backend, no `og start tmux`). Add multiplexer detection alongside the agent PATH scan, through an injectable `which` seam so tests never depend on the real PATH (this machine has herdr). Surface it in `--check` as an optional tool -- a missing multiplexer is a warn, never an err, and never touches the exit code -- and in `--questions` as `detected_multiplexers` plus the catalog. Add the `herdr_agents_pane` install question (bool, default true), asked always like tunnl_ssh_key and noting it is ignored without herdr. It reaches og-install.json via the interactive plan and, crucially, via `main()`'s --plan path, which is how `og update` re-applies a live state file that predates the key: it now loads, gets the default, and re-applies cleanly. Tests: catalog shape (+ no tmux row), injected-seam detection, --check exit 0 without a multiplexer, the --questions entry, the old-state idempotence case, and the state round-trip. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Three small cleanups in the herdr bridge, no behaviour change: - og_herdr.main(): delete a duplicated run_forever block that sat below the return above it and could never run. Pre-existing at HEAD. The live forever path still catches KeyboardInterrupt and returns 0. - SessionWatcher._fetch_listing -> fetch_listing. og_agents reached across modules into a private to read the `truncated` flag that list_sessions discards. Renamed public with a docstring stating the (rows, truncated) contract; no alias kept, so a straggler caller fails loudly. Callers updated: og_herdr_watch (internal x2) and og_agents. - HerdrClient.workspace_rename: workspace.rename had no wrapper, so the bridge sent it through the generic call() and the conformance guard in tests/test_og_herdr_client.py — which enumerates wrappers by introspection — could not see the frame. Wrapper added beside workspace_create/workspace_close; og_herdr now routes through it. Tests: fetch_listing is public and _fetch_listing is gone; the conformance inventory and the named workspace-wrappers test now cover workspace_rename; the exact workspace.rename frame is pinned; main() returns 0 on KeyboardInterrupt. The bridge-side rename schema test is kept as the specific case, now driving the named wrapper. 741 passed (was 738; three new tests). Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
`og start herdr` now starts the server as usual and then opens the session inside a herdr space: a space for this directory, a chat (or an attach) in it, and the TUI handed to the operator. Four cases, keyed on whether we are already inside herdr and whether this directory already has a live session. bin/og: - Split argument parsing into parse_start_args so the multiplexer keyword is stripped from ANY position BEFORE the bare-argument arm can read it as an ngrok reserved domain. `og start herdr` no longer resolves to "tunneled on ngrok reserved domain herdr". - Mux ids come from registry.json's multiplexers array, with a fallback row (herdr) so a checkout predating that array degrades to "no multiplexer" rather than to a keyword that does nothing. - Widen the help-block sed range (2,20 -> 2,26) for the two added lines. installer/og_herdr_launch.py (new): - The launcher itself. Everything is an injected dependency (herdr client, session watcher, server spawn), so no test reaches a socket. - Adoption asks workspace.list EVERY time and treats an unreadable listing as an error: "nothing recorded", "not in the listing" and "could not read the listing" are three different answers. - No redundant workspace.focus after workspace.create(focus=True) in case B; case C still focuses explicitly because it is about to close the pane it is in, which lives in a different space. - The pending record (cases A/D) is written through og_herdr's own writer so its reader/`--cleanup` can claim it by directory. tests/test_og_herdr_launch.py (new): fake client + stub watcher covering all four cases, the agents split, the create-reply guard, and the install settings. Verified: pytest tests/ -q -> 761 passed; bash -n bin/og; shellcheck -S error bin/og install.sh. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
tests/test_og_start_mux.py did not exist, though bin/og:2165 cites it by
name as "the guard that cannot rot". Add it: the missing guard is exactly
how a phantom herdr method shipped past a green suite.
Drives parse_start_args by sourcing bin/og in a subshell (never `og start`,
which would boot a server) and pins:
- the 8-row argv -> mode/domain/provider/mux table, including the
regression that `tunneled mydomain` and bare `mydomain` still read as
an ngrok reserved domain now that a mux keyword can appear anywhere;
- the bare-argument info line, so the explicit form is distinguishable;
- every top-level dispatch arm appears in the `sed -n '2,26p'` help block;
- `og start herdr` on a PATH with no herdr warns and degrades, proven by
reaching the NEXT precondition (no agent bundle) instead of a mux fatal.
HOME is sandboxed per test; nothing starts a server, touches ~/.omnigent,
or runs herdr. Verified to bite: a scratch copy with the mux-strip loop
neutered fails the herdr rows with the old ngrok-domain reading.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
jmvbambico
changed the title
Three defects from a real `og start herdr` session. 1. The bridge was never started, so nothing was ever projected: the space opened with one chat pane and no agent tabs. bin/og now starts og_herdr.py as a detached daemon (nohup + og-herdr.pid + log under logs/herdr) just before the launcher takes over, idempotent on a live pid; cmd_stop ends it and clears its pidfile. 2. Omnigent's server auto-opened a browser at boot (OMNIGENT_ACCOUNTS_AUTO_OPEN defaults to true, server/app.py:1674). A mux launch now exports it as 0 before the server boots — scoped to the mux path and only when the operator has not set it themselves. 3. pane.split had no ratio, so herdr halved the tab. Pass 0.8 (the fraction RETAINED by the split pane), giving the chat 80% and the agents sidebar 20%. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
A multiplexer launch replaces `og start`'s terminal with `exec <mux>`, so the QR + URL show_access prints there is never seen — the chat pane only ever ran `og chat`, which printed one info line and exec'd the REPL. cmd_chat now recovers the address and prints the SAME banner before the exec, on a tty only: - `access_url()` is the one recovery helper: a live tunnel URL (validated against its owner's pid) else the LAN address, setting OG_PUBLIC_URL and OG_MODE for show_access. It returns non-zero and touches nothing when neither resolves, so no banner beats a wrong address. cmd_start keeps its own logic on purpose — its branches establish the address and owe the provider log / missing-LAN-IP failure a read-only recovery cannot give. - print_qr() now skips a code too wide for the pane (measured from the ASCII render, bytes/2, locale-independent; `tput cols` with an 80-column fallback), so a narrow pane keeps the URL line and drops the QR. Missing qrencode still degrades to nothing. tests/test_og_chat_access.py drives cmd_chat under a pty with a fake omnigent on PATH; 5 cases fail on the pre-fix tree (bug witnesses) and 2 pass on both (regression guards). Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Inside a multiplexer launch the terminal `og start` printed the access banner into is replaced by `exec`, so the address is never visible in the split. Add a bottom panel to `og agents`, toggled with `a` (default off, also `--access` for the one-shot view), that shows the QR and the address on demand. The panel is part of `render`'s lines, computed from state on the Frame, so the renderer stays pure and the whole feature is testable through `render` / `--once` with no tty. - The QR is drawn only when it fits the pane's width. Fit is measured with the escapes stripped (`_visible_width`): ANSIUTF8 wraps each line in SGR escapes, and measuring those with `len()` reads a 27-column code as ~41 and silently degrades the panel to URL-only forever. - `qrencode` absent -> URL line only. No address resolved -> one honest line naming no address, because a banner pointing somewhere dead is worse than none. - Address recovery lives in one function (`access_info`): the tunnel cache `$OMNIGENT_HOME/og-tunnel.url` first, else the LAN IP at the server's port. It names bin/og's counterparts (`access_url`, `tunnel_url`, `lan_ip`, `show_access`) so a future consolidation is one edit. Deliberately a second implementation, not a call into bin/og: `og agents` is standalone and must not depend on `og` being on PATH. - Panel height is content-sized; the agent list keeps a floor of ACCESS_LIST_FLOOR (3) rows when it is open. Module stays read-only toward Omnigent (the AST guard passes). Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
`access_info` read `og-tunnel.url` as-is, unlike bin/og's `tunnel_url`, so a tunnel that crashed but left its cache behind was advertised as authoritative — the worst address to hand a phone, since scanning it yields nothing. The panel was built on "a wrong address is worse than no address"; this closes that gap. `_cached_tunnel_url` mirrors the three gates of bin/og's `tunnel_url` in its order: a non-empty URL file, a provider record naming ngrok or tunnl, and that provider's pidfile holding a live pid (`_pid_alive`, the Python form of `kill -0`). Any gate failing falls through to the LAN address, exactly as if no cache existed. It does NOT clear the cache, unlike bin/og: `og agents` is a read-only viewer (the AST guard pins that), and the operator's state files belong to `og`'s lifecycle, not a viewer's. The stale value is ignored; `og` cleans it up. Tests cover a dead pid, an unrecognised provider, a missing pidfile, a live pid and a missing/empty URL file, plus a byte-for-byte assertion that no case modifies or deletes the cache. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
…den the pane Three defects from a real `og start herdr` on adoboflix. 1. TWO spaces for one conversation. The pending record was written AFTER `og chat` was typed, but `og chat` mints the session the moment it runs and the herdr bridge is already polling (bin/og starts it first) — so a poll in that window saw a root session with no record for its directory and opened its own space. Write the pending record BEFORE the send, in cases A and D (both go through `_open_space`). Every id it needs is in the workspace.create reply, so nothing is waited on. If `pane_run` then fails, the record is dropped and the space closed, so no pending record points at a dead space; the create-missing-pane cleanup runs before the record, so it cannot dangle one either. 2. The browser still opened. `suppress_accounts_autopen` silenced omnigent's SERVER auto-open, which (a) receives the var fine — `nohup env VAR=1 cmd` inherits the environment — and (b) could never fire for og anyway, because that open is gated on a LOOPBACK OMNIGENT_ACCOUNTS_BASE_URL and og always points it at the LAN/tunnel address (docs/TROUBLESHOOTING.md says so). The browser came from og's OWN openers on the same run: `og chat` -> `omnigent run` opens the conversation for an interactive launch, and cmd_start's needs_setup path calls open_in_browser itself. `omnigent run` has no --no-open and no env var for it, so the mux suppression now also sets the `auto_open_conversation` config key (only when unset — an explicit choice wins) and gates the needs_setup open behind a flag that prints the URL instead. The `has_autopen`/value is NOT the bug: env_var_is_truthy treats 1/true/yes as true, so "0" was already falsy. 3. The agents pane was too narrow. 0.8 left it ~43 columns on the operator's 215-column terminal and clipped the ~52-column `og agents` footer. 0.7 gives ~30% (~64 columns), enough for the footer and the 27-column QR. A pane that cannot show its own controls is broken. Tests: the ordering is asserted at the instant the chat line is sent (the new tests fail on the pre-fix tree); the config write and the needs_setup gate are pinned with a stubbed omnigent. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
…ig write Follow-up to b902c15. That fix shut the chat pane's browser by running `omnigent config set --global auto_open_conversation=false` — a persistent, machine-wide change to the operator's omnigent install, triggered by one `og start herdr`. It overstepped: they asked for no browser on the mux path, not to change omnigent's default everywhere, and nothing would have told them og did it. Replace it with a per-pane environment variable. `webbrowser` — which `omnigent run` uses to open the live conversation — honours $BROWSER, and pointing that at `true(1)` makes open() run `/usr/bin/true <url>`: it succeeds and opens nothing. The pane runs `og chat` in a FRESH shell, so the value has to travel to the pane rather than the launcher; herdr takes an env map on every pane-creating call and installs it in the pane's shell. - og_herdr_client: `workspace_create` / `tab_create` take an optional `env` map, sent only when set — every existing frame is byte-for-byte unchanged. - og_herdr_launch: `pane_env_for` builds `{BROWSER: <operator's own or "true">}` and it is threaded onto every pane a launch creates — the space (both `_open_space` and `_adopt`) and the agents `pane.split`. - bin/og: delete `suppress_conversation_autopen` and its global write. Keep `suppress_server_autopen`, re-commented as a GUARD for a path that does not fire for og today (that open is loopback-gated), and keep the needs_setup gate. An operator who already set $BROWSER is passed through untouched, not overridden. Tests: BROWSER on workspace.create / tab.create / pane.split (schema-checked), an existing $BROWSER passed through, no `env` key when a launch supplies none, and a pinned assertion that no code path writes omnigent's config. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
…it covers The approved design: a plain harness header per root, a full box per worker (`├─`/`└─` down a tree spine, `▸` on the selected box's FIRST row only), ASCII glyphs (`+ ! x ?`) with a 6-dot braille spinner for anything working, an access box sized to its own content and flush left, and a one-line footer. render() stays pure — the spinner frame and the selection are inputs, not clocks — so `--once` prints exactly the lines curses would paint. Bug 1 — the QR printed its own escapes. `qr_lines` returned `qrencode -t ANSIUTF8` output with its ANSI colour escapes intact, and curses does not interpret escapes, it PRINTS them: the operator's pane showed `^[[40;37;1m` down both sides of the code. `--once` looked fine only because the terminal running it interpreted them. Escapes are stripped at the source and again on the way out of `access_lines`, so no ESC byte can reach `addnstr`; the block characters alone already read as black-on-white. Bug 2 — the view showed every session on the machine. `select_scope` was correct and re-resolved every poll; the failure was upstream, where a detail was fetched only for roots the live heuristic kept — and a brand-new `og chat` in this directory (idle, nothing dispatched yet) is not live by that rule, so its `workspace` was never fetched, nothing matched `$PWD`, and the view fell back to the wide set and stayed wrong. The cwd question is not a question about activity, so it is now asked independently: roots are probed until one matches (each result cached per session id, bounded by the same MAX_ROOT_DETAILS cache and pruned against the roots this poll asked about), and a match IS the scope. Also: the spinner ticks on its own 120ms clock, independent of the 3s data poll, and nothing is repainted unless the composed lines actually change — with nothing working the render is byte-identical, so the loop blocks on the keyboard until the next poll instead of erasing and refreshing eight times a second forever in a pane the operator leaves open all day. The display state is derived here from `status` + `pending_elicitations_count` rather than by widening `herdr_state`: that function is shared with the herdr bridge, where a fifth state would change which sessions get a herdr tab. The only difference is unknown -> failed. Tests cover the B1 shape verbatim, every glyph, the spinner's advance and purity, the six-dot invariant (no descenders, so the row cannot jitter), the absence of ESC in every line handed to the painter, a quiet worker-less root in `$PWD` narrowing the view, the content-sized access box, an idle pane scheduling no repaint, and `--once` without curses. Fifteen of the seventeen new tests fail against the pre-change module; the two that pass on both (`test_the_cwd_probe_is_skipped_entirely_under_all_roots` and `test_main_once_reports_the_missing_tty`) are regression guards, not bug witnesses.
tui_proto.py renders the four layout candidates and the component variants (glyphs, headers, selection markers, footers, access panels); tui_pick.py renders the chosen combination -- B1 -- with the four box styles and a live spinner, at the width the pane actually gets. Kept because the design was chosen by looking at these rather than from a description, and because the pane being NARROW is the constraint that decided it: every variant renders at the real width, so a future change can be judged the same way. python3 docs/proto/tui_proto.py layouts --demo python3 docs/proto/tui_pick.py --live B1 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Runs og inside herdr: one space per conversation, each
delegated worker a tab inside it, live agent-status badges, and a terminal UI
over the session tree. The browser and the ngrok tunnel keep working — herdr
becomes the desk interface, the web UI stays the phone one.
og is fully usable without herdr. The multiplexer is a view, never the
runtime;
og start herdron a machine without it warns and starts normally.What's in it
og start herdr [tunneled] [--use tunnl]og chatin it, optionally splitsog agentsbeside it, attaches the TUIog herdrog agents [--all]Four modules: a herdr socket client, an Omnigent session watcher (knows nothing
about herdr), the bridge, and the launcher. Plus
installer/registry.jsongaining a
multiplexerscatalog and a new install question.Gate
Zero failures, including the two
test_og_install.pyPYTHONPATH cases thatfail on
mainfor anyone who has actually installed og (#22 folded in; #23'sspec folded in as
docs/MUX-LAUNCHER.md).Found by running it, after the tests were already green
The author installed this and ran a real session. Five things the suite could
not have told us:
og start herdropened the space and ranog chat, and nothing ran the daemon that projects sessionsserver/app.py:1674, gated byOMNIGENT_ACCOUNTS_AUTO_OPEN, default Truepane.splitwas never passed aratioog startprints the QR in the terminal thatexec herdrthen replacesAll five fixed. The bridge is now a proper daemon (
OG_HERDR_PIDFILE, startedwith the server, stopped by
og stop); the browser is suppressed only onthe mux path and never when the operator set the variable themselves; the split
is 80/20;
og chatprints the access banner; andog agentsgained anatoggle showing a content-sized QR panel on demand.
A measurement error worth recording
The QR was first measured at 41 columns by taking
len()ofqrencode'sANSIUTF8 output — which counts ANSI colour escapes as visible characters. The
true width is 27. On that bad number the agents pane looked too narrow for a
QR and the feature was nearly built URL-only.
It was caught because a worker independently measured via
qrencode -t ASCIIand halved, and the two disagreed. There is now a test asserting that a block
carrying colour escapes is measured by visible columns — the one defect class
that would have silently degraded this feature forever with every test green.
Zero failures. The two
test_og_install.pyPYTHONPATH cases that fail onmainfor anyone who has actually installed og are fixed here too (#22 isfolded in; #23's spec is folded in as
docs/MUX-LAUNCHER.md).Verified live, not just in tests
Run against the author's real herdr session and Omnigent server:
hivemind,og chatstarted, pending record written keyed by cwd
creating a second one:
pendingemptied, owner preserved, setup not re-run.This was the design's central risk and it holds.
created nothing
space, with
working/blocked/idlebadgesspinner, prompt, status bar and all
og agents— the session tree with correct status glyphs, scoped by cwd23 blocking defects found and fixed
The split by how each was found is the most useful thing in this PR:
The ones that would have shipped broken
pane.runis not a herdr API method. It's a CLI subcommand. Every callwas rejected — four tabs opened, all empty. No pane would ever have run
anything. Survived 223 tests, a full cross-vendor review and a live dry run;
caught only by running the bridge for real.
the bridge's four-calls-per-session flow failed on every second call.
tab.createtakesworkspace_id, notworkspace. serde drops unknownkeys silently, so
--workspacedid nothing and every tab landed in thefocused workspace — including the author's real one.
pending_elicitations_countis plural. The code read the singular MCPspelling, so
blocked— the state a human most needs — could never fire.session was pushed off page 1 and never projected; a session crossing the page
boundary looked identical to a deletion, so the bridge closed a live pane.
Defects of the author's own making
Several were wrong contracts written from the endpoint that was to hand rather
than the one the code calls —
pending_elicitation_count,session_id,kindand
workspaceall came fromsys_session_get_info's MCP output instead of theREST shape. The implementers built the contract faithfully.
docs/HERDR.mdnowcarries the measured contract so the next change doesn't re-derive it.
The guard that stops it recurring
herdr publishes its full contract (
herdr api schema --json). It's vendored astests/fixtures/herdr_api_schema.json, and a conformance test drives everyclient wrapper against a recording fake, captures each emitted frame, and asserts
the method is a real variant with valid, complete parameters.
It's driven by introspecting the client, not a hand-written list, because a
hand-written list rots exactly the way the comments did. The distinction that
matters: a strict fake catches a bad method; only the schema catches a bad
parameter. When
workspacewas reintroduced as an experiment, the fake wasperfectly happy — the schema check was the only thing that failed.
What it cannot do
"tmux"is hardcoded ininner/terminal.pywith no backend hook, and each terminal is its own privatetmux server. Nothing for a shim to impersonate.
inner/acp_executor.py:514— stdioJSON-RPC, no pty), so those panes show the attach event stream, not a TTY.
every client but parks a single Future — first resolver wins. Auto-answering
would race the user's own browser. An AST test pins this.
Non-blocking, left for later
Listed in
docs/HERDR.md§Open items: a never-fetched session retires only whenit comes back into view; duplicate rows for one id are last-wins;
pane_readaccepts five payload spellings where one is in use;
_announce_listingcan losea notice if stderr fails.
🤖 Generated with Claude Code