Repository navigation
Conversation
…s cold-start-only, not a per-call cost #1011/#1028's own CI measurement (strace -f -e trace=clone,clone3,fork,vfork, whole-hook total for post-tool-hook.sh's no-save path, minimum of 9 runs) is 67 on HEAD, down from v0.43.0's 69 and v0.42.4's 68 -- but still on the order of several dozen processes for one no-save PostToolUse call, per #1029. Traced to the test's own fixture: tests/test_post_tool_total_processes_1011.py creates a brand-new tmp_path (HOME, PROJECT, REMEMBER_DIR, TMPDIR all freshly created) for every one of the n repeated runs, so every measured run takes the slow/cold path -- _remember_env_cache_load misses, so resolve-paths.sh, detect-tools.sh, bootstrap-dirs.sh and log.sh all get sourced, pulling in lib-memory-dir.sh's worktree-detection forks, _config_load's jq read, and several mktemp+mv atomic cache publishes. On the warm path -- any call after the first in a real session, the common case once a project has run once -- _remember_env_cache_load succeeds and none of those four files are sourced at all; the only forks left are _pt_scan_lines's own tail+wc pair, which the code's own comment directly above the wc call already documents as unavoidable without reintroducing #1017's quadratic bash-expansion regression. No source change lands in this commit: every candidate #1029 named (bootstrap-dirs.sh's directory creation, the mktemp+mv atomic-write pattern, the jq config read, _pt_scan_lines's two forks) traced to either already-gated cold-path-only work (#230/#666), work inside the byte-pinned lib-memory-dir.sh (tests/test_case_divergence_298.py forbids touching it), or a fork already proven, in-repo, unavoidable without a worse regression. Forcing a cut here would mean either reaching into the pinned file or reintroducing one of #1013/#1017/#1021's own fixed regressions, neither of which is a safe trade for a performance-only, non-blocking issue. tests/test_post_tool_total_processes_1011.py now asserts an absolute budget (69 -- the measured 67 plus 2 of slack) for both the below-threshold and cooldown paths, in addition to the existing relative tag-comparisons, so a real future reduction or regression on this cold path is still caught even on a CI leg where neither tagged comparison tree can be materialized (no network reaching origin). Adds a changelog fragment documenting the corrected attribution with the CI-measured totals. Co-Authored-By: Claude Sonnet 5 <[email protected]>
….sh, removing three unconditional subshell forks per call Self-review (Explore + oss:auditor review of commit 3116fcf) found a real, low-risk cut this round's investigation missed: _stdin_json_string in scripts/post-tool-hook.sh was called through X=$(...) command substitution at three sites (REMEMBER_HOOK_CWD, STDIN_SESSION_ID, STDIN_TRANSCRIPT_PATH), all above the _REMEMBER_FAST if/else, so all three ran unconditionally on EVERY invocation of this hook -- cold-start and warm path alike -- each forking a subshell even though the function itself is pure parameter expansion with no external exec at all. session-start-hook.sh already carries a fork-free twin of this exact extractor, _stdin_json_string_into (#665, part of #660), written into VARNAME via printf -v instead of printed through a subshell. Ports that same function (renamed in place, same matching algorithm: same first-occurrence scan, same #344 nested-key gap, same #829 double-backslash collapse) into post-tool-hook.sh and switches all three call sites to it. No behaviour change: tests/test_hook_cwd_end_to_end_411.py, tests/test_codex_stdin_session_id_468.py and tests/test_session_end_hook_345.py, which all exercise this extraction end-to-end, pass unchanged, along with 47 other post-tool-hook.sh tests run locally (test_post_tool_* family, test_hot_path_cost_pin_330, test_pt_scan_lines_wall_clock_1017, test_post_tool_fast_path_350) and tests/test_release_branch_check_900.py's byte-budget pin. tests/test_locale_ranges_695.py's five post-tool-hook.sh line pins shift by +4 for the lines this change adds above them. Tightens tests/test_post_tool_total_processes_1011.py's new absolute-budget assertions (added in 3116fcf) from <69 to <67 -- strictly below #1028's own prior total, reflecting this cut -- for both the below-threshold and cooldown paths; the exact new total is reasoned, not re-measured locally (this file's own strace gate needs Linux, unavailable here), and this pull request's own CI run will confirm it. Updates the changelog fragment and this file's own module docstring to describe the fix landed rather than "no further cut was found." Co-Authored-By: Claude Sonnet 5 <[email protected]>
…_json_string_into 00f26b7 ported the fork-free printf-v extractor into post-tool-hook.sh, renaming _stdin_json_string to _stdin_json_string_into there (same shape as session-start-hook.sh's own #665/#660 port). The #447 characterization tests still looked for the old name by string match, so all 7 post-tool/* parametrized cases failed collection-time assertions on every CI leg ("_stdin_json_string not found in scripts/post-tool-hook.sh"), not a behaviour regression. Confirmed the new extractor keeps the same first-occurrence scan, the same documented nested-before/after gap, and the same #829 double-backslash collapse by reading its body directly (post-tool-hook.sh:212-230): identical mechanism to _stdin_json_string, just printf -v into a named variable instead of printed and captured via command substitution. No product fix needed -- only the test's CASES table, which already supported the "_into" calling convention via session-start-hook.sh's own rows in the same table. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The previous commit fixed test_stdin_extractor_top_level_wins_447.py's own CASES table but missed this file's separate EXTRACTORS table, which also names post-tool-hook.sh's extractor by the old "_stdin_json_string" / keyed=True / into=False shape. CI's first re-run surfaced the gap directly: test_windows_cwd_is_decoded[post-tool], test_unc_prefix_decodes_pairwise[post-tool] and test_posix_cwd_is_unchanged[post-tool] all failed the same "_stdin_json_string not found in scripts/post-tool-hook.sh" assertion. Same fix as before: the function is now _stdin_json_string_into (printf -v, no subshell), so this row flips to into=True, matching session-start-hook.sh's own "session-start/into" row in the same table. Co-Authored-By: Claude Sonnet 5 <[email protected]>
printf -v resolves against the innermost local, so the port left a latent collision: a future call site passing a VARNAME of value, dq, rest, raw, field or prefix would write the function own local, not the caller. session-start-hook.sh copy already prefixes every local for this reason. Same line count, so the locale-range pins do not move. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
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.
What
post-tool-hook.sh's _stdin_json_string extractor was invoked through command substitution at three unconditional sites -- REMEMBER_HOOK_CWD, STDIN_SESSION_ID, STDIN_TRANSCRIPT_PATH -- all three above the _REMEMBER_FAST if/else, so all three forked a subshell on EVERY hook invocation, cold and warm path alike, even though the function is pure parameter expansion.
This ports the fork-free _stdin_json_string_into form (printf -v into a named variable, no subshell) that session-start-hook.sh already carries for the identical extractor (#665/#660) -- same matching algorithm, same #344 first-occurrence-gap and #829 double-backslash-collapse behaviour, no behaviour change.
Also tightens the absolute-budget assertions in tests/test_post_tool_total_processes_1011.py from a less-than-or-equal-69 bound to a strict less-than-67 bound: the earlier bound was vacuous against the exact bad value the repo's own history calls already-regressed (v0.43.0 measured 69), caught by a reviewing auditor this round. The new bound is strictly below both the bad value and #1028's own prior total of 67.
This is a follow-up to #1011 and its round-2 fix in #1028: #1011/#1028 brought the whole-hook strace total down to 67 but did not identify this cut; #1029 asked for the remaining attribution and the cut this PR makes.
Testing
All local testing was on macOS, where strace is unavailable, so the two tests that actually measure this issue's claimed fork count (tests/test_post_tool_total_processes_1011.py) run SKIPPED locally, both before and after the fix -- never seen green or red locally. This is flagged, not hidden: the fork-count claim rests on (a) static reading of the old forking vs new fork-free extractor forms, (b) the identical pattern already proven in session-start-hook.sh's own history, and (c) this PR's own CI run, which must be checked before merge to confirm the new budget actually holds.
116 other tests passed locally (0 failed, 2 skipped -- the strace-gated ones above), covering the exact stdin-extraction call sites changed, the fast and slow path split, cooldown, basename and session-id handling, and the byte-budget pin in tests/test_locale_ranges_695.py (five line pins shifted plus four each for the new lines added above them).
The changelog assembly check passes with the new changelog.d/1029.fixed.md fragment.
Scope note
scripts/session-end-hook.sh still carries the OLD forking _stdin_json_string extractor -- same opportunity, same fix available -- but is NOT touched here, since #1029 is scoped to post-tool-hook.sh specifically and touching a third hook file would widen blast radius beyond what was briefed. Filed as a follow-up candidate rather than rolled into this PR.
Closes #1029.
Self-review note
One below-bar reviewer finding on the new absolute-budget assertions: the changelog fragment and commit message both explicitly describe the absolute-budget assertions as existing so a regression is caught even on a no-network CI leg -- exactly the narrow case the reviewer confirmed it still covers correctly.
🤖 Generated with Claude Code
[AI-generated]