fix(F1): route ACP permission asks to auto-deny - #76
Merged
Conversation
Track F-CRITICAL #1 from ROADMAP.md. ## Problem `extras/acp/mod.rs::build_acp_permission` constructed `(ask_tx, _ask_rx)` and immediately dropped `ask_rx`. When a tool needed `Ask` confirmation in ACP mode (Zed / editor client), the permission check called `ask_tx.send()`, awaited the oneshot reply, and waited 30 seconds before timing out. The user saw a generic tool failure with no context. Tools requiring permission were effectively unrunnable from ACP clients unless the user configured `--yolo` or explicit allow rules in `permission` config. ## Fix New `spawn_acp_ask_drain` task takes ownership of the receiver and responds to every `AskRequest` with `UserDecision::Deny` immediately. Fail-fast with a clear deny beats a 30s timeout: - The LLM sees the denial in the tool result and can re-plan. - The error message is "Permission denied by user", not a generic timeout. - Explicit allow rules (set via config or `/allow add` in interactive mode beforehand) still work — they short-circuit before the ask channel. Routing the ask through the ACP protocol as a real `requestPermission` notification (so the editor surfaces a dialog) is a larger Phase C5-ish feature; F1 is the minimum-viable non-hang fix. ## Tests Two new tests in `extras::acp::tests`: - `acp_ask_drain_responds_with_deny`: send one `AskRequest`, assert the reply arrives within 200ms with `Deny`. - `acp_ask_drain_handles_multiple_concurrent_asks`: send 5 asks in a loop, assert all 5 replies arrive promptly with Deny — guards against a single-shot drain bug. ## Test plan - [x] `cargo test --features acp` — 2 new tests pass. - [x] `cargo test --features plugin` — 649 pass (full suite). - [x] `cargo build --all-features` — compiles, no warnings.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
All actionable Track F items now landed: - F-CRITICAL (PRs #76, #77): ACP perm asks routed to Deny, find/glob/list_dir hide dotfiles by default - F-HIGH (PRs #78–#84): compress aligns cut-point, read streams large files, ACP parallel tool ids, bash pgid cleanup, symlink canonicalize, session schema version, quote-aware bash splitter - F-MEDIUM (PRs #85, #86, #87, #89, #90, #91): interleaved bash output, compress net-savings, Retry-After parsing, plugin docs, relative-path normalize, BOM strip, bounded interject channel - F-SKIP (F9, F11, F15): verified false positives or N/A; rationale documented inline Status legend updated; ordering recommendation replaced with the shipped-status section. Co-authored-by: Yogthos <yogthos@gmail.com>
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…I-C) (#94) Three real bugs flagged by post-Track-F code review of PRs #76–#93. Other review findings were verified as false positives or low- priority docs (documented in the response, not actioned). ## Review fix 1 — F14 unicode byte-offset panic (CRITICAL) `retry_after_from_error_msg` parsed the lowercased message to find the label, then indexed into the ORIGINAL message at the lowercased string's byte offset. `to_lowercase()` can change byte length for some unicode (Turkish `İ` → `i̇` is 2 → 3 bytes, Greek `Σ` → `σ` differs), so the offset disagreed with the original. `&msg[idx + label.len()..]` could land mid-UTF-8 and panic. Fix: scan the original byte windows directly with case-insensitive ASCII compare (the label is ASCII, so the match itself is sound). Added `is_char_boundary` defense at the tail-slice point for double safety. Also capped digit-run consumption at 11 chars so a malformed `Retry-After: 999999...` doesn't overflow u64 before the 5-min cap clamps. ## Review fix 2 — F18 canonicalize hot path (HIGH) `is_external_path` called `std::fs::canonicalize(cwd)` on EVERY permission check. With hundreds of tool calls per session that accumulated to hundreds of stat() syscalls, plus the canonicalize already happening inside `resolve_absolute`. Fix: new `working_dir_canonical: String` field on `PermissionChecker`. Computed once at construction via `canonicalize_for_cache`, refreshed by `set_working_dir`. `is_external_path` now reads the cached value — zero syscalls per check. ## Review fix 3 — F10 `$'...'` ANSI-C quoting bypass (MEDIUM) The bash-substitution detector flagged `$(`, backticks, `<(`, `>(` but missed `$'...'` (ANSI-C quoting). A command like `echo $'hi\nrm -rf /; ls'` had its body treated as one quoted token by `quote_aware_split`, so the `;` inside wasn't a separator and the whole thing checked as one permission rule. Adding `$'` to the substitution list routes such commands through the whole-command check rather than the per-segment splitter. ## Review findings NOT actioned (verified false positives) - F4 trailing-newline count: tokio's `next_line()` returns `None` at EOF without yielding an empty final line, so `"a\nb\n"` reports 2 lines correctly. - F12 biased stdout starvation: `tokio::select! { biased; ... }` biases tie-breaks (when both arms are ready) — it doesn't block one arm while the other is busy. - F20 try_send semantic: documented in F20's PR description. - Avatar with multi-line input: `input_top` is computed from `rows - input_rows - 1`, so the avatar's row correctly tracks multi-line input. - fit() w==0 edge case: render_table caps at `per_col >= 1` before calling fit, so w==0 never reaches fit() in practice. ## Tests 3 new tests in `agent::recovery::tests`: - `retry_after_handles_unicode_before_label`: the original panic reproducer (`İoError: Retry-After: 8`); must parse without panic and return 8s. - `retry_after_label_match_is_case_insensitive`: both `RETRY-AFTER-MS` and `Retry-After-Ms` parse. - `retry_after_caps_pathological_digit_run`: 22-digit run doesn't overflow; final backoff still caps at 5 minutes. 687 pass (was 684). All build profiles clean. 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#76) Track F-CRITICAL #1 from ROADMAP.md. ## Problem `extras/acp/mod.rs::build_acp_permission` constructed `(ask_tx, _ask_rx)` and immediately dropped `ask_rx`. When a tool needed `Ask` confirmation in ACP mode (Zed / editor client), the permission check called `ask_tx.send()`, awaited the oneshot reply, and waited 30 seconds before timing out. The user saw a generic tool failure with no context. Tools requiring permission were effectively unrunnable from ACP clients unless the user configured `--yolo` or explicit allow rules in `permission` config. ## Fix New `spawn_acp_ask_drain` task takes ownership of the receiver and responds to every `AskRequest` with `UserDecision::Deny` immediately. Fail-fast with a clear deny beats a 30s timeout: - The LLM sees the denial in the tool result and can re-plan. - The error message is "Permission denied by user", not a generic timeout. - Explicit allow rules (set via config or `/allow add` in interactive mode beforehand) still work — they short-circuit before the ask channel. Routing the ask through the ACP protocol as a real `requestPermission` notification (so the editor surfaces a dialog) is a larger Phase C5-ish feature; F1 is the minimum-viable non-hang fix. ## Tests Two new tests in `extras::acp::tests`: - `acp_ask_drain_responds_with_deny`: send one `AskRequest`, assert the reply arrives within 200ms with `Deny`. - `acp_ask_drain_handles_multiple_concurrent_asks`: send 5 asks in a loop, assert all 5 replies arrive promptly with Deny — guards against a single-shot drain bug. ## Test plan - [x] `cargo test --features acp` — 2 new tests pass. - [x] `cargo test --features plugin` — 649 pass (full suite). - [x] `cargo build --all-features` — compiles, no 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#92) All actionable Track F items now landed: - F-CRITICAL (PRs dirge-code#76, dirge-code#77): ACP perm asks routed to Deny, find/glob/list_dir hide dotfiles by default - F-HIGH (PRs dirge-code#78–dirge-code#84): compress aligns cut-point, read streams large files, ACP parallel tool ids, bash pgid cleanup, symlink canonicalize, session schema version, quote-aware bash splitter - F-MEDIUM (PRs dirge-code#85, dirge-code#86, dirge-code#87, dirge-code#89, dirge-code#90, dirge-code#91): interleaved bash output, compress net-savings, Retry-After parsing, plugin docs, relative-path normalize, BOM strip, bounded interject channel - F-SKIP (F9, F11, F15): verified false positives or N/A; rationale documented inline Status legend updated; ordering recommendation replaced with the shipped-status section. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…I-C) (dirge-code#94) Three real bugs flagged by post-Track-F code review of PRs dirge-code#76–dirge-code#93. Other review findings were verified as false positives or low- priority docs (documented in the response, not actioned). ## Review fix 1 — F14 unicode byte-offset panic (CRITICAL) `retry_after_from_error_msg` parsed the lowercased message to find the label, then indexed into the ORIGINAL message at the lowercased string's byte offset. `to_lowercase()` can change byte length for some unicode (Turkish `İ` → `i̇` is 2 → 3 bytes, Greek `Σ` → `σ` differs), so the offset disagreed with the original. `&msg[idx + label.len()..]` could land mid-UTF-8 and panic. Fix: scan the original byte windows directly with case-insensitive ASCII compare (the label is ASCII, so the match itself is sound). Added `is_char_boundary` defense at the tail-slice point for double safety. Also capped digit-run consumption at 11 chars so a malformed `Retry-After: 999999...` doesn't overflow u64 before the 5-min cap clamps. ## Review fix 2 — F18 canonicalize hot path (HIGH) `is_external_path` called `std::fs::canonicalize(cwd)` on EVERY permission check. With hundreds of tool calls per session that accumulated to hundreds of stat() syscalls, plus the canonicalize already happening inside `resolve_absolute`. Fix: new `working_dir_canonical: String` field on `PermissionChecker`. Computed once at construction via `canonicalize_for_cache`, refreshed by `set_working_dir`. `is_external_path` now reads the cached value — zero syscalls per check. ## Review fix 3 — F10 `$'...'` ANSI-C quoting bypass (MEDIUM) The bash-substitution detector flagged `$(`, backticks, `<(`, `>(` but missed `$'...'` (ANSI-C quoting). A command like `echo $'hi\nrm -rf /; ls'` had its body treated as one quoted token by `quote_aware_split`, so the `;` inside wasn't a separator and the whole thing checked as one permission rule. Adding `$'` to the substitution list routes such commands through the whole-command check rather than the per-segment splitter. ## Review findings NOT actioned (verified false positives) - F4 trailing-newline count: tokio's `next_line()` returns `None` at EOF without yielding an empty final line, so `"a\nb\n"` reports 2 lines correctly. - F12 biased stdout starvation: `tokio::select! { biased; ... }` biases tie-breaks (when both arms are ready) — it doesn't block one arm while the other is busy. - F20 try_send semantic: documented in F20's PR description. - Avatar with multi-line input: `input_top` is computed from `rows - input_rows - 1`, so the avatar's row correctly tracks multi-line input. - fit() w==0 edge case: render_table caps at `per_col >= 1` before calling fit, so w==0 never reaches fit() in practice. ## Tests 3 new tests in `agent::recovery::tests`: - `retry_after_handles_unicode_before_label`: the original panic reproducer (`İoError: Retry-After: 8`); must parse without panic and return 8s. - `retry_after_label_match_is_case_insensitive`: both `RETRY-AFTER-MS` and `Retry-After-Ms` parse. - `retry_after_caps_pathological_digit_run`: 22-digit run doesn't overflow; final backoff still caps at 5 minutes. 687 pass (was 684). All build profiles clean. 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.
Track F-CRITICAL #1. ACP mode previously dropped
ask_rxso any tool requiringAsktimed out at 30s. Nowspawn_acp_ask_drainimmediately replies withDenyfor fail-fast behavior. Routing through the ACP protocol as a realrequestPermissionnotification is future work. 2 new tests, 649 pass.