fix: address code review findings (F14 unicode, F18 hot path, F10 ANSI-C) - #94
Merged
Conversation
…I-C) 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.
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.
Three real bugs flagged by post-Track-F code review of PRs #76-#93: F14 unicode byte-offset panic (CRITICAL); F18 canonicalize syscall per permission check (HIGH); F10
$'...'ANSI-C bypass in bash splitter (MEDIUM). Other review findings verified as false positives — listed in commit body. 3 new tests, 687 pass.