plugin: granular streaming hooks — P3 (on-turn-start/end + on-message-update) - #38
Closed
yogthos wants to merge 2 commits into
Closed
plugin: granular streaming hooks — P3 (on-turn-start/end + on-message-update)#38yogthos wants to merge 2 commits into
yogthos wants to merge 2 commits into
Conversation
added 2 commits
May 20, 2026 11:53
Preparatory work for P3 (granular streaming hook events) and P4
(session tree branch-aware accounting). Rig's multi_turn stream is
a flat sequence of assistant content + tool results + a final response;
this commit introduces a pure state machine that detects per-turn
boundaries and emits them on the event channel.
A "turn" = one LLM call + the tool calls it dispatched + the tool
results returning. Pure-text response = 1 turn. Run with two cycles
of tool calls = 2 turns.
Implementation:
* `TurnTracker` (src/agent/runner.rs) is a 3-state state machine —
Idle, InTurn, AwaitingNext — that consumes observation calls
(`observe_assistant_content`, `observe_tool_result`,
`observe_stream_end`) and returns `Vec<Boundary>` describing the
TurnStart/TurnEnd events to emit.
* The InTurn→AwaitingNext→InTurn transition collapses N parallel
tool results into a single turn boundary, no matter how many tool
results arrive between assistant messages.
* Lazy emission: TurnEnd fires either when the next assistant
content begins (closing the previous turn) or on stream end. This
avoids needing lookahead to know whether more tool results are
still coming.
* `run_stream` instantiates one TurnTracker and forwards Boundary
events through the existing `event_tx` before each assistant-content
event and at stream termination (Done, Error, Interjected, or
unexpected stream close).
New events:
* `AgentEvent::TurnStart { index: u32 }` — fires before the first
assistant content of each turn.
* `AgentEvent::TurnEnd { index: u32 }` — fires after the last event
of each turn (either before the next TurnStart, or before Done /
Error / Interjected).
The UI loop currently handles both with a no-op match arm; P3 will
wire them into plugin `on-turn-start` / `on-turn-end` hooks.
Tests: 8 new unit tests on `TurnTracker` in `mod turn_tracker_tests` —
pure-text, single tool call, multi-result collapsing, alternating
turns, empty stream, mid-dispatch end, lone tool result, restart
reuse. Total: 510 pass with plugin feature (was 502); 460 pass
without (was 452); 12 pre-existing plugin-test fails unchanged.
Refs dirge-e17.
Builds on P0's turn-boundary detection. Wires the new
AgentEvent::TurnStart / TurnEnd events into three new plugin hooks:
on-turn-start {:index N}
on-message-update {:index N :partial "text-so-far"}
on-turn-end {:index N :message "full turn text"}
These give plugins per-turn observability that wasn't possible from
the previous `on-response` hook alone (which only fires once at the
end of the whole agent run). Typical uses:
* Per-turn cost / latency tracking.
* Token-level filtering or annotation.
* Cancel-on-condition based on streaming output.
* Streaming a live transcript to an external sink.
Implementation:
* `src/ui/streaming.rs` — new TokenBatcher type that collects tokens
since the last flush and yields the accumulated text once the count
threshold (DEFAULT_BATCH_TOKENS = 16) is crossed. Count-based
rather than time-based so the tests are deterministic without
mocking Instant. Module is cfg-gated to the plugin feature.
* `src/ui/mod.rs`:
- New per-turn streaming state (token_batcher, current_turn_text,
current_turn_index), cfg-gated.
- AgentEvent::Token arm now pushes to the batcher; on flush,
dispatches `on-message-update` with the accumulated text.
- AgentEvent::TurnStart arm resets per-turn state and dispatches
`on-turn-start`.
- AgentEvent::TurnEnd arm flushes any trailing partial batch (as
a final `on-message-update`), then dispatches `on-turn-end`
with the full turn text.
* `src/main.rs` — adds the three hook names to auto-discovery.
`harness/replace-message` (mutating the persisted assistant text from
on-message-end) is deferred. It needs session-write position tracking
and interacts with usage accounting; will land in a follow-up phase.
Tests: 6 new unit tests on `TokenBatcher` — threshold yielding,
post-flush fresh batch, partial-batch draining, reset, zero-threshold
clamping, lossless content over a run. Total: 516 pass with plugin
(was 510); 460/12 baseline without plugin unchanged.
plugins/turn_timing.janet — example using on-turn-start / on-turn-end
to notify the user how long each turn took.
Refs dirge-87x.
3 tasks
yogthos
force-pushed
the
feat/runner-turn-events
branch
from
May 20, 2026 16:44
4505898 to
4f8ed7d
Compare
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…aths (#111) 23 audit findings verified REAL via parallel agent verification + cross-check against opencode/pi reference patterns. Shipping the 10 most concrete fixes here; the rest go in a follow-up docs/test batch. ## Security - **#9 bash quote_aware_split missed bare `|`** — `safe_cmd | rm -rf /` was treated as one segment; only the LHS got permission-checked. Pipe RHS rode in unchecked under the fallback (non-semantic-bash) path. Added single-byte `|` split after `||` is matched. The tree-sitter path was already correct. - **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc into the LLM as lossy UTF-8 wasted tokens and confused the model. Ported opencode `read.ts:153-198`: reject by extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the first 4 KiB — null byte = binary, >30% non-printable = binary. Clear error message tells the agent to use bash + xxd instead. ## Correctness - **#2 skill override inverted** — README contract: "Project skills override global skills by name". Code used `map.entry(name).or_insert(skill)` which KEEPS the first (global) value and silently drops project overrides. Switch to `map.insert` (last-write-wins) since globals iterate first and project iterates second. - **#37 skill empty name** — frontmatter `name:` with empty value parsed to "", which then matched any `skill ""` call silently. Fall back to directory name when frontmatter name is empty/whitespace-only. - **#1 session_tree.janet hook never fired** — plugin defined `(defn on-message ...)` but `(def hooks [])` was empty AND the hook name doesn't exist (dirge uses `on-message-update`). `/label` was permanently broken ("no entry yet"). Fix: rename to `on-message-update` + register in hooks vector. - **#7 workflow.janet hooks vector missing entries** — plugin defined `workflow-on-tool-end`, `-on-error`, `-on-complete` but only registered the first four hook names. Three hooks were dead. Added them. - **#26 MCP malformed JSON silently empty args** — `serde_json::from_str(&args).unwrap_or_default()` turned bad JSON into None, sending the server an empty argument set. Server then errored with confusing "missing required field" instead of dirge surfacing the actual parse error. Now returns ToolError with the parse error message + first 200 chars of the offending JSON. - **#22 /prompt default unreachable** — README documents `default` as a built-in prompt (prompts/default.md exists), but `/prompt default` was intercepted as a magic "clear" keyword. If `default` is registered in `context.prompts`, the new branch falls through to the normal name-lookup. Only acts as clear-keyword when no `default` prompt is present (legacy fallback). - **#23 /allow add accepted invalid tools** — typo `/allow add bsah ...` silently created an inert rule the user couldn't debug. Added a known-tools whitelist matching PermissionConfig fields; unknown tools error with the valid list. ## Performance + correctness - **#11 grep loaded whole files into memory** — no size cap meant a 9MB file got fully buffered. Added 10 MiB per-file cap via metadata pre-check. - **#15 Python dunder methods marked non-exported** — `!name.starts_with('_')` treats `__init__`/`__call__`/etc. as private, even though they're Python's standard public protocol. Recognize `__x__` dunder pattern as exported. ## UI - **#36 panel char-count truncation vs Unicode width** — panel truncation used `chars().count()` while wide emoji and CJK take 2 cells. A status line with an emoji overflowed the right border by one cell. Switched to `UnicodeWidthStr::width` for both truncation and padding. ## Tests 4 new regression tests: - `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc - `test_is_binary_content_null_byte` — null byte trigger, UTF-8 Japanese stays clean, all-non-printable triggers - `quote_aware_split_splits_on_bare_pipe` — pipe security - `quote_aware_split_or_and_pipe_distinct` — `a || b | c` produces 3 segments, not 2 725 plugin / 599 default pass. All build profiles clean. ## Verified false positives (not fixed, audit was wrong) - #3 cache.rs clear() race — generation counter gating in `get` makes stale entries invisible, no correctness impact. - #17 DeepSeek auto-detect priority — auto-detect only fires when env vars present; default-default is still OpenRouter. - #19 semantic tools in collision filter — semantic tools added separately, can't be shadowed by MCP. - #20 glob global gitignore — intentionally disabled to match grep behavior. - #28 nearest_root blocking std::fs — function doesn't exist in current code. - #32 ReadArgs.path vs GrepArgs.path — semantically different by design (file vs dir), documented in schema. - #33 install_plugin_providers dead-without-feature — gated with explicit `#[cfg_attr(not(feature), allow(dead_code))]`. - #34 websearch double-gated — config + API key serve distinct purposes (enable + auth). ## Deferred to follow-up batches Docs-only fixes (#6 CONFIG.md tools, #12 temperature, #13 --api-key, #14 acp_host/port), MCP/LSP architecture (#8, #25, #27), test gaps (#38-40), and lower-priority polish — all in a follow-up PR. Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos
added a commit
that referenced
this pull request
May 21, 2026
Follow-up to PR #111. Tier-2 items from the 23-bug audit batch: docs corrections and two small correctness/UX fixes. ## Docs - **#12 temperature** — CONFIG.md claimed "parsed but not currently applied". Actually applied since PR #105 with a clamp warning. Rewrote the cell. - **#13 --api-key** — flag existed but neither README nor CONFIG.md mentioned it. Added a Quick-start example noting the process-list visibility caveat. - **#14 acp_host/acp_port** — CONFIG.md documented both keys but the CLI flags were intentionally removed (stdio-only transport). Removed both from the keys table + ACP section. - **#6 tools** — `Config::tools` (per-tool enable map) was fully wired in code but undocumented. Added a row to the keys table covering `tools.websearch` and `tools.webfetch`. - **#21 find_callers** — README claimed "word-boundary regex" but the impl uses the tree-sitter symbol index. Updated to reflect actual behavior; the user-visible word-boundary semantics are preserved. ## Code - **#16 semantic index skip_dir** — `SymbolIndex::find_callers` filter had its own hardcoded `matches!(name, "node_modules" | "target" | ".git" | "__pycache__")` while the rest of the codebase uses `agent::tools::is_skip_dir`. Switched to the shared helper so future additions stay in lockstep. - **#18 context::load_file** — silently swallowed `read_to_string` errors via `.ok()`. A permission-denied AGENTS.md looked identical to a missing file. Now emits a stderr warning naming the path + reason; still returns None so callers' behavior is unchanged. 725 plugin / 599 default pass. All build profiles clean. ## Remaining audit items (deferred to feature work) - **#8 LSP no crash restart**: needs broken-pipe IO error handling + exponential backoff. Touches manager state machine. - **#10 task tool fire-and-forget**: needs timeout + cleanup coordination via JoinHandle tracking. - **#25 MCP no reconnection**: similar architectural concern to #8. - **#27 LSP didClose**: client lifecycle hook missing. - **#29 token estimation len/4**: needs per-provider usage extraction (Phase 6 work). - **#5 MCP shutdown**: rmcp Drop semantics need verification. - **#38/39/40 semantic test gaps**: get_symbol_body untested, list_symbols kind_filter untested, find_definition test vacuous. Sat down to add but each requires a fixture build. Together with PR #111 (10 code fixes), 17 of the 23 verified items are now shipped. Remaining 6 are architectural or test-infrastructure work better tackled as discrete PRs. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…aths (dirge-code#111) 23 audit findings verified REAL via parallel agent verification + cross-check against opencode/pi reference patterns. Shipping the 10 most concrete fixes here; the rest go in a follow-up docs/test batch. ## Security - **dirge-code#9 bash quote_aware_split missed bare `|`** — `safe_cmd | rm -rf /` was treated as one segment; only the LHS got permission-checked. Pipe RHS rode in unchecked under the fallback (non-semantic-bash) path. Added single-byte `|` split after `||` is matched. The tree-sitter path was already correct. - **#4 read.rs no binary detection** — feeding a PDF/ELF/.pyc into the LLM as lossy UTF-8 wasted tokens and confused the model. Ported opencode `read.ts:153-198`: reject by extension list (zip/exe/.o/.pdf/.png/etc.), then sniff the first 4 KiB — null byte = binary, >30% non-printable = binary. Clear error message tells the agent to use bash + xxd instead. ## Correctness - **#2 skill override inverted** — README contract: "Project skills override global skills by name". Code used `map.entry(name).or_insert(skill)` which KEEPS the first (global) value and silently drops project overrides. Switch to `map.insert` (last-write-wins) since globals iterate first and project iterates second. - **dirge-code#37 skill empty name** — frontmatter `name:` with empty value parsed to "", which then matched any `skill ""` call silently. Fall back to directory name when frontmatter name is empty/whitespace-only. - **#1 session_tree.janet hook never fired** — plugin defined `(defn on-message ...)` but `(def hooks [])` was empty AND the hook name doesn't exist (dirge uses `on-message-update`). `/label` was permanently broken ("no entry yet"). Fix: rename to `on-message-update` + register in hooks vector. - **dirge-code#7 workflow.janet hooks vector missing entries** — plugin defined `workflow-on-tool-end`, `-on-error`, `-on-complete` but only registered the first four hook names. Three hooks were dead. Added them. - **dirge-code#26 MCP malformed JSON silently empty args** — `serde_json::from_str(&args).unwrap_or_default()` turned bad JSON into None, sending the server an empty argument set. Server then errored with confusing "missing required field" instead of dirge surfacing the actual parse error. Now returns ToolError with the parse error message + first 200 chars of the offending JSON. - **dirge-code#22 /prompt default unreachable** — README documents `default` as a built-in prompt (prompts/default.md exists), but `/prompt default` was intercepted as a magic "clear" keyword. If `default` is registered in `context.prompts`, the new branch falls through to the normal name-lookup. Only acts as clear-keyword when no `default` prompt is present (legacy fallback). - **dirge-code#23 /allow add accepted invalid tools** — typo `/allow add bsah ...` silently created an inert rule the user couldn't debug. Added a known-tools whitelist matching PermissionConfig fields; unknown tools error with the valid list. ## Performance + correctness - **dirge-code#11 grep loaded whole files into memory** — no size cap meant a 9MB file got fully buffered. Added 10 MiB per-file cap via metadata pre-check. - **dirge-code#15 Python dunder methods marked non-exported** — `!name.starts_with('_')` treats `__init__`/`__call__`/etc. as private, even though they're Python's standard public protocol. Recognize `__x__` dunder pattern as exported. ## UI - **dirge-code#36 panel char-count truncation vs Unicode width** — panel truncation used `chars().count()` while wide emoji and CJK take 2 cells. A status line with an emoji overflowed the right border by one cell. Switched to `UnicodeWidthStr::width` for both truncation and padding. ## Tests 4 new regression tests: - `test_is_binary_extension_known` — pdf/tgz/.so/.jpg/.pyc - `test_is_binary_content_null_byte` — null byte trigger, UTF-8 Japanese stays clean, all-non-printable triggers - `quote_aware_split_splits_on_bare_pipe` — pipe security - `quote_aware_split_or_and_pipe_distinct` — `a || b | c` produces 3 segments, not 2 725 plugin / 599 default pass. All build profiles clean. ## Verified false positives (not fixed, audit was wrong) - #3 cache.rs clear() race — generation counter gating in `get` makes stale entries invisible, no correctness impact. - dirge-code#17 DeepSeek auto-detect priority — auto-detect only fires when env vars present; default-default is still OpenRouter. - dirge-code#19 semantic tools in collision filter — semantic tools added separately, can't be shadowed by MCP. - dirge-code#20 glob global gitignore — intentionally disabled to match grep behavior. - dirge-code#28 nearest_root blocking std::fs — function doesn't exist in current code. - dirge-code#32 ReadArgs.path vs GrepArgs.path — semantically different by design (file vs dir), documented in schema. - dirge-code#33 install_plugin_providers dead-without-feature — gated with explicit `#[cfg_attr(not(feature), allow(dead_code))]`. - dirge-code#34 websearch double-gated — config + API key serve distinct purposes (enable + auth). ## Deferred to follow-up batches Docs-only fixes (dirge-code#6 CONFIG.md tools, dirge-code#12 temperature, dirge-code#13 --api-key, dirge-code#14 acp_host/port), MCP/LSP architecture (dirge-code#8, dirge-code#25, dirge-code#27), test gaps (dirge-code#38-40), and lower-priority polish — all in a follow-up PR. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…de#112) Follow-up to PR dirge-code#111. Tier-2 items from the 23-bug audit batch: docs corrections and two small correctness/UX fixes. ## Docs - **dirge-code#12 temperature** — CONFIG.md claimed "parsed but not currently applied". Actually applied since PR dirge-code#105 with a clamp warning. Rewrote the cell. - **dirge-code#13 --api-key** — flag existed but neither README nor CONFIG.md mentioned it. Added a Quick-start example noting the process-list visibility caveat. - **dirge-code#14 acp_host/acp_port** — CONFIG.md documented both keys but the CLI flags were intentionally removed (stdio-only transport). Removed both from the keys table + ACP section. - **dirge-code#6 tools** — `Config::tools` (per-tool enable map) was fully wired in code but undocumented. Added a row to the keys table covering `tools.websearch` and `tools.webfetch`. - **dirge-code#21 find_callers** — README claimed "word-boundary regex" but the impl uses the tree-sitter symbol index. Updated to reflect actual behavior; the user-visible word-boundary semantics are preserved. ## Code - **dirge-code#16 semantic index skip_dir** — `SymbolIndex::find_callers` filter had its own hardcoded `matches!(name, "node_modules" | "target" | ".git" | "__pycache__")` while the rest of the codebase uses `agent::tools::is_skip_dir`. Switched to the shared helper so future additions stay in lockstep. - **dirge-code#18 context::load_file** — silently swallowed `read_to_string` errors via `.ok()`. A permission-denied AGENTS.md looked identical to a missing file. Now emits a stderr warning naming the path + reason; still returns None so callers' behavior is unchanged. 725 plugin / 599 default pass. All build profiles clean. ## Remaining audit items (deferred to feature work) - **dirge-code#8 LSP no crash restart**: needs broken-pipe IO error handling + exponential backoff. Touches manager state machine. - **dirge-code#10 task tool fire-and-forget**: needs timeout + cleanup coordination via JoinHandle tracking. - **dirge-code#25 MCP no reconnection**: similar architectural concern to dirge-code#8. - **dirge-code#27 LSP didClose**: client lifecycle hook missing. - **dirge-code#29 token estimation len/4**: needs per-provider usage extraction (Phase 6 work). - **dirge-code#5 MCP shutdown**: rmcp Drop semantics need verification. - **dirge-code#38/39/40 semantic test gaps**: get_symbol_body untested, list_symbols kind_filter untested, find_definition test vacuous. Sat down to add but each requires a fixture build. Together with PR dirge-code#111 (10 code fixes), 17 of the 23 verified items are now shipped. Remaining 6 are architectural or test-infrastructure work better tackled as discrete PRs. Co-authored-by: Yogthos <yogthos@gmail.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.
Summary
Builds on PR #37 (P0). Wires P0's `AgentEvent::TurnStart` / `TurnEnd`
boundary events into three new plugin hooks:
These give plugins per-turn observability that wasn't possible from
the previous `on-response` hook alone (which only fires once at end
of the whole agent run).
Implementation
threshold 16 tokens) that buffers streamed tokens between
`on-message-update` dispatches so Janet doesn't get hammered
with one call per token.
`current_turn_index`) gated to the plugin feature. `Token` arm
pushes through the batcher; `TurnStart` resets and dispatches;
`TurnEnd` flushes the trailing partial then dispatches.
`harness/replace-message` is deferred — needs session-write
position tracking that's out of scope for this PR.
Test plan
6 new unit tests in `mod streaming::tests`:
threshold yielding
post-flush fresh batch
partial-batch draining via `flush_remaining`
reset
zero-threshold clamping
lossless content over a full run
`cargo test --features plugin` — 516 pass (was 510).
`cargo test` (no plugin) — 460 + 12 pre-existing fails (unchanged).
Both builds clean; no new clippy warnings.
`plugins/turn_timing.janet` is included as a working example using
`on-turn-start` / `on-turn-end` to notify the user how long each
turn took.
Stacked on
Builds on PR #37 (P0 turn-boundary detection). Targets
`feat/runner-turn-events` so the diff is just the hook wiring +
streaming module, not P0's tracker code.
Refs dirge-87x.