perf(tui): stop rebuilding per-call work on the shell, hook and cloud paths (#6208) - #6264
Conversation
… paths (#6208) Five mechanical fixes from the 2026-09-15 perf review (`codewhale-ops/PERF-OPPORTUNITIES-20260915.md` §1), each independently verifiable and behavior-preserving. Q3 — `output_is_image` lowercased the whole tool output, hundreds of kilobytes at worst, to sniff eight ASCII suffixes, once per MCP completion. It now scans the raw bytes case-insensitively. The historical `contains` contract is unchanged, including "weird.pngx" counting as an image; only the allocation is gone. Q4 — the mention browser's `sort_by_key(|entry| entry.to_lowercase())` allocated a fresh key O(n log n) times per keystroke. `sort_by_cached_key` computes it once per entry. Q5 — the cloud launcher built a fresh `reqwest::blocking::Client` per call, so all nine control-plane call sites paid a new TCP+TLS handshake and one of them is a poll loop. One process-wide client now owns the connection pool (`clone()` is a refcount bump) and the total timeout moved to the request. That is also the shape the harness turn needs: its budget comes from its own command and must not inherit the 120s control-plane cap, which is why the per-client timeout existed in the first place. Q6 — `execpolicy.toml` was re-read, re-parsed and its regexes recompiled on every shell execution. `load_default_policy` now caches the parsed rules against (path, length, mtime) — length joins the timestamp because a coarse-mtime filesystem can report the same instant for two revisions — and `matcher::compiled_glob` compiles each pattern once. Q7 — hook tool-name globs compiled a fresh `Regex` per hook per tool-call/stop event. `tool_name_matches_condition` now uses the same `compiled_glob`, which deletes its copy of the escape/anchoring logic. `compiled_glob` escapes every regex metacharacter except `*` exactly as `pattern_matches` always has, so what matches does not change — including the fact that `regex`'s `.` does not cross a newline. Two tests pin the new helper's contract (one compilation per pattern, escaping preserved), and one pins the image sniff. Verification on this machine (macOS, aarch64), `cargo check` and `cargo clippy` with the project's flags (`-D warnings` plus the three documented allowances) both clean: test result: ok. 217 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out (codewhale-execpolicy — lib, authorization_order, redirection_policy, doctests) test result: ok. 8 passed; 0 failed (tui::history::tool_output) test result: ok. 44 passed; 0 failed (working_set) test result: ok. 109 passed; 0 failed (hooks::executor) test result: ok. 151 passed; 0 failed; 1 ignored (tools::shell) test result: ok. 32 passed; 0 failed (cloud_dispatch) Not verified here: hosted CI, and the Q5 change has no automated test that pins connection reuse — it is exercised only through the existing cloud_dispatch tests, which cover the request/response shape rather than the pooling. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 2m 33s —— View job Review: perf(tui): stop rebuilding per-call work on the shell, hook and cloud paths (#6264)
SummaryThis is a clean, mechanical set of caching/reuse fixes. All five changes preserve their stated behavioral contracts and the diff is well within the "reuse and simplification" spirit of AGENTS.md's ponytail ladder — nothing new was invented where an existing pattern ( Correctness
Minor: misleading doc comment ( Provider/model/route safety — not applicable; this diff touches no model/provider selection code. Reuse/simplification — good: Tests — the three new tests ( Note: I was unable to run |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 7 file patches, sha256:166f0fbfcff87a6f0d5961c36030831192ef6b6ff0195809baea2337cd866497.
Pass 1: Five mechanical perf changes across execpolicy, hooks, cloud dispatch, shell policy loading and the MCP image sniff. Under source inspection the four behavioural rewrites are equivalent to the code they replace (same anchored regex, same ASCII case-insensitive suffix scan, same stable sort), so I found no functional regression; the only defect is a new doc comment in compiled_glob that states the opposite of what the compiled regex does. Two cache-validity tradeoffs and two things I could not check (build/edition, non-send_json callers) are recorded in the assessment.
Findings
- [INFO]
compiled_globdoc says*matches newlines; the compiled.*does not (crates/execpolicy/src/matcher.rs:11)
The new doc comment oncompiled_globclaims*matches 'any run of characters, newline included'. The regex it builds isformat!("^{escaped}$")where*became.*and nothing enables(?s), and Rust'sregexcrate does not let.match\n;^/$are text anchors by default, so a pattern such asmcp__*orrm -rf *will not span a newline. Runtime behaviour is unchanged (pattern_matchesand the old hook matcher built exactly this regex inline), so nothing is broken today — but the comment contradicts the property the PR description says must be preserved, and someone reasoning about multi-line command coverage (the deny path intoml_rules::evaluaterelies onexpanded_commandsto split lines precisely because a single anchored glob cannot cross one) would be misled by it. Smallest fix: correct the parenthetical.
Suggestions
-
crates/execpolicy/src/matcher.rs:11— Correct the wildcard description so it matches the regex actually compiled:*becomes.*, and.does not match a newline unless(?s)is set, which it is not. Lines 11-12 of the new file read as one sentence with the following blank///, so rewording both keeps the paragraph valid./// (matching any run of characters, but not a newline: `.` does not match /// one). `None` means the pattern does not compile; callers treat that as /// "no match".
Assessment
Pass 1: Source inspection only — no build, clippy run or test execution was performed here, so the PR's cargo check/test claims are unverified. Within the diff, the four behaviour-bearing rewrites look equivalent to what they replace: compiled_glob builds ^<regex::escape(p) with \* -> .*>$, byte-for-byte the regex pattern_matches and the old tool_name_matches_condition built inline; [u8]::windows + eq_ignore_ascii_case cannot produce a false positive across a UTF-8 boundary (continuation bytes are >= 0x80, and no non-ASCII character lowercases to '.', 'p', 'n' or 'g'), so it preserves the lowercase+contains contract for the eight ASCII suffixes; sort_by_cached_key is a stable sort like sort_by_key; and the request-level .timeout() in send_json_on carries the same 120 s / budget+slack values the per-client timeouts did. The one defect found is the new compiled_glob doc comment, which is the only issue reported.
Tradeoffs I judged deliberate rather than defective, but which are worth stating: (1) load_default_policy now keys its cache on (path, len, mtime), so a same-length edit made inside the filesystem's mtime granularity — or any edit that preserves mtime (touch -r, cp -p, some checkouts) — is ignored for the life of the process, where the previous code re-read the file on every shell execution. That file carries deny rules, so hashing the contents (one read, still skipping the parse and the regex compiles) would remove the caveat; the author documents the length+mtime compromise explicitly. (2) compiled_glob's cache is process-global and never evicted; today its inputs are configuration patterns, so growth is bounded, but any caller that ever passes per-command or user-dictated strings would grow it without limit.
Open questions I could not settle from the supplied evidence: whether any cloud-dispatch request is built directly from blocking_client() without going through send_json/send_json_on — such a request would now have no total timeout at all, because the client-level timeout was removed; the diff's own evidence (the removed builder wrapper had exactly two callers, and send_json is the only remaining one) suggests all nine call sites are covered. Also unverified: that codewhale_execpolicy::matcher is publicly reachable from codewhale-tui (the new codewhale_execpolicy::matcher::compiled_glob path in hooks/executor.rs requires pub mod matcher), and that the tui crate's edition/MSRV accept the let-chain in load_default_policy (let-chains are gated to edition 2024 in current Rust). Finally, the image sniff now makes eight naive byte-window passes instead of one to_lowercase plus memchr-backed contains; the allocation is gone, but a throughput win is not established without measurement.
Advisory review by Codewhale (codewhale review --pr 6264 --post, head e522e4f93c0d925717724b202b1ddbbc6d5e8f9f). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| /// `pattern` compiled once as an anchored `*`-glob, then reused. | ||
| /// | ||
| /// Every other regex metacharacter is escaped, so `*` is the only wildcard | ||
| /// (matching any run of characters, newline included). `None` means the pattern |
There was a problem hiding this comment.
[INFO] compiled_glob doc says * matches newlines; the compiled .* does not
The new doc comment on compiled_glob claims * matches 'any run of characters, newline included'. The regex it builds is format!("^{escaped}$") where * became .* and nothing enables (?s), and Rust's regex crate does not let . match \n; ^/$ are text anchors by default, so a pattern such as mcp__* or rm -rf * will not span a newline. Runtime behaviour is unchanged (pattern_matches and the old hook matcher built exactly this regex inline), so nothing is broken today — but the comment contradicts the property the PR description says must be preserved, and someone reasoning about multi-line command coverage (the deny path in toml_rules::evaluate relies on expanded_commands to split lines precisely because a single anchored glob cannot cross one) would be misled by it. Smallest fix: correct the parenthetical.
| /// (matching any run of characters, newline included). `None` means the pattern | ||
| /// does not compile; callers treat that as "no match". |
There was a problem hiding this comment.
Correct the wildcard description so it matches the regex actually compiled: * becomes .*, and . does not match a newline unless (?s) is set, which it is not. Lines 11-12 of the new file read as one sentence with the following blank ///, so rewording both keeps the paragraph valid.
| /// (matching any run of characters, newline included). `None` means the pattern | |
| /// does not compile; callers treat that as "no match". | |
| /// (matching any run of characters, but not a newline: `.` does not match | |
| /// one). `None` means the pattern does not compile; callers treat that as | |
| /// "no match". |
Summary
Five mechanical, behavior-preserving fixes from
codewhale-ops/PERF-OPPORTUNITIES-20260915.md§1, closing out the #6208 checklist. Each one removes work that was being redone per call for data that does not change between calls.Closes #6208
The five changes
Q3 — zero-alloc image sniffing.
output_is_imagebuilt a lowercased copy of the entire tool output — hundreds of kilobytes at worst — to look for eight ASCII suffixes, once per MCP completion. It now scans the raw bytes case-insensitively. The historicalcontainscontract is unchanged, includingweird.pngxstill counting as an image.Q4 — cached sort key. The mention browser's
sort_by_key(|entry| entry.to_lowercase())allocated a fresh keyO(n log n)times per keystroke.sort_by_cached_keycomputes it once per entry.Q5 — one HTTP client. The cloud launcher built a fresh
reqwest::blocking::Clientper call, so all nine control-plane call sites paid a new TCP+TLS handshake — and one of them is a poll loop. One process-wide client now owns the connection pool (clone()is a refcount bump) and the total timeout moved onto the request. That is also the shape the harness turn needs: its budget comes from its own command and must not inherit the 120s control-plane cap, which is the reason the per-client timeout existed.Q6 — execpolicy stopped re-reading itself.
~/.deepseek/execpolicy.tomlwas re-read, re-parsed, and its regexes recompiled on every shell execution (tools/shell.rs:5161).load_default_policynow caches the parsed rules against(path, length, mtime)— length joins the timestamp because a coarse-mtime filesystem can report the same instant for two revisions — andmatcher::compiled_globcompiles each pattern once instead of per rule per expanded command.Q7 — hook globs compiled once.
tool_name_matches_conditioncompiled a freshRegexper hook per tool-call/stop event. It now uses the samecompiled_glob, which deletes its copy of the escape/anchoring logic rather than leaving two implementations to drift.What deliberately did not change
compiled_globescapes every regex metacharacter except*exactly aspattern_matchesalways has, so what matches is unchanged — including the fact thatregex's.does not cross a newline. The issue suggested a hand-rolled glob for Q6 ("most patterns are plainprefix*"); I kept the regex and cached it instead, because a byte-glob would silently start matching across newlines in multi-line commands and that is a behavior change this slice should not smuggle in.Verification
cargo check -p codewhale-execpolicy -p codewhale-tui --all-targets --all-features --lockedandcargo clippywith the project's flags (-D warningsplus the three documented allowances) both clean. Tests viascripts/with-hermetic-test-home.sh:Three tests are new: two pin
compiled_glob's contract (one compilation per pattern, escaping preserved) and one pins the image sniff, which had no test at all.Not verified
Hosted CI. And the Q5 change has no test that pins connection reuse — it is exercised only through the existing
cloud_dispatchtests, which cover the request/response shape rather than the pooling. If we want that guaranteed rather than argued, it needs a test with a local listener counting accepts.