feat(phase 1): first-wins harness/block + doc subagent isolation + README correction - #69
Merged
Merged
Conversation
…ADME stream fix Phase 1 of the deferred-items plan. Reference patterns: pi's `runner.ts` block semantics, opencode's subagent isolation. ## First-blocker-wins for harness/block Previously last-write-wins: if two plugins registered `on-tool-start` hooks and both called `(harness/block reason)`, the second overwrote the first. Block reason became load-order-dependent and hooks ran after a deny that obviously shouldn't proceed. Now matches pi's `packages/coding-agent/src/core/extensions/runner.ts:806-827` `tool_call` semantics: as soon as ANY plugin sets `harness-block`, dispatch stops and subsequent plugins do NOT run for this tool call. First blocker's reason wins. `harness/mutate-input` and `harness/replace-result` keep last- write-wins chaining (matches pi's `runner.ts:858-888` and opencode's `plugin.ts:102-134` Immer-draft shared mutation), so successive plugins can refine each other's mutations. Implementation: `dispatch_tool_hook` now iterates hooks one-by- one rather than reusing `dispatch()`. After each hook returns, peek `harness-block` via the new private `has_pending_block` helper — break out of the loop if set. Inlined the per-hook catch-wrap (sanitize error → push notif → tracing::warn) so the structure mirrors `dispatch()` without sharing. ## README "buffered on retry" claim corrected The README claimed "stream events are buffered and only flushed on success." False — `runner.rs::run_stream` emits Token / Reasoning / ToolCall events live as they arrive. On retry the UI shows "(error: …; retrying)" and the next attempt's tokens stream fresh. Tool calls dispatched before failure still inhibit retry (correct). Updated the wording. Also mentions `overloaded_error` in the retry list (added in the previous PR). ## docs/PLUGINS.md: hook precedence + subagent isolation New bullets in the "Subtle distinctions" section: - First-wins block semantics for multi-plugin `on-tool-start`, with reference to pi's design. - Subagent isolation: when the LLM calls `task`, dirge runs a one-shot LLM query with no tools / no hooks / no permission gates. Matches opencode's bare-LLM subagent and pi's full- subprocess design — isolation is intentional. Plugin authors who want subtask observability should route through `harness/log` from the parent's `on-response` hook. ## Tests 3 new tests, written failing first: - `dispatch_tool_hook_first_blocker_stops_dispatch` — two blockers, second has a notify side-effect; assert the second never fired AND the first's reason is the result. - `dispatch_tool_hook_runs_all_when_no_block` — confirms early- stop only triggers on actual blocks. - `dispatch_tool_hook_mutations_still_chain_last_wins` — guards against accidentally also short-circuiting on mutation slots. Updated `test_dispatch_tool_hook_block_sticks_across_hooks` → removed (replaced by the new first-blocker test which asserts stricter semantics). 637 pass total, 0 fail, all build profiles clean. ## Out-of-scope items now integrated into the plan The audit's deferred items have been folded into a 6-phase plan: - **Phase 1** (this PR): semantic fix + docs. - **Phase 2**: sibling-branch pruning on compress/rewind (opencode's drop-with-notification pattern). - **Phase 3**: tool-call structured persistence with interrupted-state pairing (opencode's `ToolPart` pattern). - **Phase 4**: pi-style `BranchSummaryMessage` (richer preservation if Phase 2 proves too lossy). - **Phase 5**: `/allow` CRUD slash command. - **Phase 6**: cost tracking (per-provider pricing + actual usage extraction). Phases 4-6 were initially listed as "out of scope" but the user asked for an integrated plan; documenting here so they're not forgotten. ## Test plan - [x] `cargo test --features plugin` -> 637 pass. - [x] `cargo build --all-features` -> compiles, 0 warnings.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
) Phase 5 of the plan. No clean equivalent in opencode or pi — both manage allowlist entries only via the interactive permission prompt's "allow always" answer. dirge adds explicit CRUD so users can inspect, manually add, drop one, or clear all entries without editing the session JSON. ## Commands - `/allow` or `/allow list` — show numbered entries - `/allow add <tool> <pattern>` — add a (tool, pattern) entry, e.g. `/allow add bash 'cargo *'` - `/allow remove <idx>` — drop one entry by 0-based index from the `list` output - `/allow clear` — drop all entries The `add` form preserves pattern strings containing spaces by re-deriving the args from the raw `text` slice (rather than relying on the SmallVec `parts[2]` which would get truncated at the first whitespace). Same approach `/cd` uses for paths. ## State sync Both surfaces stay aligned: - `PermissionChecker::session_allowlist` (in-memory, used by `check` / `check_path`) — updated via the existing `add_session_allowlist` and the two new `remove_session_allowlist_at(idx)` + `clear_session_allowlist`. - `Session::permission_allowlist` (persisted to JSON, restored on resume via `load_session_allowlist`) — mirrored manually in the slash handler so save/load round-trips show the user-edited list. Without the session-side mirror, a `/allow add foo bar` would stay in memory for this run but vanish on `-c` resume. Dedup at the session level too so `save()` writes a clean list. ## Tests 3 new permission tests, written failing first: - `remove_session_allowlist_at_returns_removed_entry`: remove(1) on [bash:cargo*, bash:git*, read:/tmp/*] yields Some(("bash","git *")) and the surviving entries shift down. - `remove_session_allowlist_at_out_of_range_returns_none`: remove(99) returns None without panicking; existing entries intact. - `clear_session_allowlist_empties_the_list`: clear() drops all entries. The slash handler itself is not unit-tested (requires the full UI loop fixtures); manual smoke test: ``` $ dirge --restrictive > bash 'echo hi' > (a) allow always > /allow list → [0] bash echo * > /allow add read /tmp/* > /allow list → [0] bash echo * [1] read /tmp/* > /allow remove 0 > /allow list → [0] read /tmp/* > /allow clear > /allow list → empty ``` ## Docs `/help` text gets four new lines covering each subcommand. ## Test plan - [x] `cargo test --features plugin` -> 649 pass (was 646). - [x] `cargo build --all-features` -> compiles. - [x] `cargo build --no-default-features` -> compiles. ## Plan status | Phase | PR | Done | |---|---|---| | 1 | #69 | first-wins block + docs | | 2 | #70 | sibling-branch pruning + notification | | 3 | #71 | structured tool-call persistence | | 4 | #72 | branch summary metadata in /tree | | 5 | this | /allow CRUD | | 6 | — | skipped per user request (cost tracking) | 5 of 6 phases complete; Phase 6 (cost tracking) skipped. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ADME stream fix (dirge-code#69) Phase 1 of the deferred-items plan. Reference patterns: pi's `runner.ts` block semantics, opencode's subagent isolation. ## First-blocker-wins for harness/block Previously last-write-wins: if two plugins registered `on-tool-start` hooks and both called `(harness/block reason)`, the second overwrote the first. Block reason became load-order-dependent and hooks ran after a deny that obviously shouldn't proceed. Now matches pi's `packages/coding-agent/src/core/extensions/runner.ts:806-827` `tool_call` semantics: as soon as ANY plugin sets `harness-block`, dispatch stops and subsequent plugins do NOT run for this tool call. First blocker's reason wins. `harness/mutate-input` and `harness/replace-result` keep last- write-wins chaining (matches pi's `runner.ts:858-888` and opencode's `plugin.ts:102-134` Immer-draft shared mutation), so successive plugins can refine each other's mutations. Implementation: `dispatch_tool_hook` now iterates hooks one-by- one rather than reusing `dispatch()`. After each hook returns, peek `harness-block` via the new private `has_pending_block` helper — break out of the loop if set. Inlined the per-hook catch-wrap (sanitize error → push notif → tracing::warn) so the structure mirrors `dispatch()` without sharing. ## README "buffered on retry" claim corrected The README claimed "stream events are buffered and only flushed on success." False — `runner.rs::run_stream` emits Token / Reasoning / ToolCall events live as they arrive. On retry the UI shows "(error: …; retrying)" and the next attempt's tokens stream fresh. Tool calls dispatched before failure still inhibit retry (correct). Updated the wording. Also mentions `overloaded_error` in the retry list (added in the previous PR). ## docs/PLUGINS.md: hook precedence + subagent isolation New bullets in the "Subtle distinctions" section: - First-wins block semantics for multi-plugin `on-tool-start`, with reference to pi's design. - Subagent isolation: when the LLM calls `task`, dirge runs a one-shot LLM query with no tools / no hooks / no permission gates. Matches opencode's bare-LLM subagent and pi's full- subprocess design — isolation is intentional. Plugin authors who want subtask observability should route through `harness/log` from the parent's `on-response` hook. ## Tests 3 new tests, written failing first: - `dispatch_tool_hook_first_blocker_stops_dispatch` — two blockers, second has a notify side-effect; assert the second never fired AND the first's reason is the result. - `dispatch_tool_hook_runs_all_when_no_block` — confirms early- stop only triggers on actual blocks. - `dispatch_tool_hook_mutations_still_chain_last_wins` — guards against accidentally also short-circuiting on mutation slots. Updated `test_dispatch_tool_hook_block_sticks_across_hooks` → removed (replaced by the new first-blocker test which asserts stricter semantics). 637 pass total, 0 fail, all build profiles clean. ## Out-of-scope items now integrated into the plan The audit's deferred items have been folded into a 6-phase plan: - **Phase 1** (this PR): semantic fix + docs. - **Phase 2**: sibling-branch pruning on compress/rewind (opencode's drop-with-notification pattern). - **Phase 3**: tool-call structured persistence with interrupted-state pairing (opencode's `ToolPart` pattern). - **Phase 4**: pi-style `BranchSummaryMessage` (richer preservation if Phase 2 proves too lossy). - **Phase 5**: `/allow` CRUD slash command. - **Phase 6**: cost tracking (per-provider pricing + actual usage extraction). Phases 4-6 were initially listed as "out of scope" but the user asked for an integrated plan; documenting here so they're not forgotten. ## Test plan - [x] `cargo test --features plugin` -> 637 pass. - [x] `cargo build --all-features` -> compiles, 0 warnings. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…irge-code#73) Phase 5 of the plan. No clean equivalent in opencode or pi — both manage allowlist entries only via the interactive permission prompt's "allow always" answer. dirge adds explicit CRUD so users can inspect, manually add, drop one, or clear all entries without editing the session JSON. ## Commands - `/allow` or `/allow list` — show numbered entries - `/allow add <tool> <pattern>` — add a (tool, pattern) entry, e.g. `/allow add bash 'cargo *'` - `/allow remove <idx>` — drop one entry by 0-based index from the `list` output - `/allow clear` — drop all entries The `add` form preserves pattern strings containing spaces by re-deriving the args from the raw `text` slice (rather than relying on the SmallVec `parts[2]` which would get truncated at the first whitespace). Same approach `/cd` uses for paths. ## State sync Both surfaces stay aligned: - `PermissionChecker::session_allowlist` (in-memory, used by `check` / `check_path`) — updated via the existing `add_session_allowlist` and the two new `remove_session_allowlist_at(idx)` + `clear_session_allowlist`. - `Session::permission_allowlist` (persisted to JSON, restored on resume via `load_session_allowlist`) — mirrored manually in the slash handler so save/load round-trips show the user-edited list. Without the session-side mirror, a `/allow add foo bar` would stay in memory for this run but vanish on `-c` resume. Dedup at the session level too so `save()` writes a clean list. ## Tests 3 new permission tests, written failing first: - `remove_session_allowlist_at_returns_removed_entry`: remove(1) on [bash:cargo*, bash:git*, read:/tmp/*] yields Some(("bash","git *")) and the surviving entries shift down. - `remove_session_allowlist_at_out_of_range_returns_none`: remove(99) returns None without panicking; existing entries intact. - `clear_session_allowlist_empties_the_list`: clear() drops all entries. The slash handler itself is not unit-tested (requires the full UI loop fixtures); manual smoke test: ``` $ dirge --restrictive > bash 'echo hi' > (a) allow always > /allow list → [0] bash echo * > /allow add read /tmp/* > /allow list → [0] bash echo * [1] read /tmp/* > /allow remove 0 > /allow list → [0] read /tmp/* > /allow clear > /allow list → empty ``` ## Docs `/help` text gets four new lines covering each subcommand. ## Test plan - [x] `cargo test --features plugin` -> 649 pass (was 646). - [x] `cargo build --all-features` -> compiles. - [x] `cargo build --no-default-features` -> compiles. ## Plan status | Phase | PR | Done | |---|---|---| | 1 | dirge-code#69 | first-wins block + docs | | 2 | dirge-code#70 | sibling-branch pruning + notification | | 3 | dirge-code#71 | structured tool-call persistence | | 4 | dirge-code#72 | branch summary metadata in /tree | | 5 | this | /allow CRUD | | 6 | — | skipped per user request (cost tracking) | 5 of 6 phases complete; Phase 6 (cost tracking) skipped. 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.
Phase 1 of the 6-phase deferred-items plan. First-blocker-wins matches pi's tool_call hook semantics. Subagent isolation documented as intentional (matches opencode + pi). README's incorrect 'buffered on retry' claim fixed. 3 new tests, 637 pass.