diff --git a/changelog.d/1029.fixed.md b/changelog.d/1029.fixed.md new file mode 100644 index 00000000..02cd0539 --- /dev/null +++ b/changelog.d/1029.fixed.md @@ -0,0 +1,5 @@ +- #1029: three unconditional per-call forks removed from `post-tool-hook.sh`, on top of CI-measured strace totals this range already tracks (`v0.42.4` = 68, `v0.43.0` = 69, prior HEAD per #1028 = 67). `_stdin_json_string` (`cwd`, `session_id`, `transcript_path`) was called through `X=$(...)` command substitution at three sites above the fast/slow-path split, each forking a subshell even though the function itself is pure parameter expansion -- the same cost class `lib-clock.sh`'s own `_remember_date_into` fix already removed elsewhere in this hook. Ported to the fork-free `_stdin_json_string_into` form `session-start-hook.sh` already used (#665, part of #660): identical matching algorithm (same first-occurrence scan, same #344 gap, same #829 double-backslash collapse), no behaviour change, three fewer forks on every invocation of this hook, cold-start and warm path alike. + + `tests/test_post_tool_total_processes_1011.py`'s own fixture measures the cold-start path only (a brand-new project with no warmed caches), which is why #1011/#1028's own strace totals land near 67-69 rather than the much lower warm-path count a real session's later tool calls pay once a project has run once. On that warm path, the only forks left after this cut are `_pt_scan_lines`'s own `tail`+`wc` pair, already documented in-code as unavoidable without reintroducing #1017's quadratic bash-expansion regression -- every other candidate this issue named (`bootstrap-dirs.sh`'s directory creation, the `jq` config read, the `mktemp`+`mv` atomic-write patterns) traced to cold-path-only work already gated behind existence checks (#230/#666) or work inside the byte-pinned `lib-memory-dir.sh` (`tests/test_case_divergence_298.py` forbids touching it). + + `tests/test_post_tool_total_processes_1011.py` now asserts an absolute budget (strictly below 67, this round's own prior total) for both the below-threshold and cooldown paths, in addition to the existing relative tag-comparisons, so a regression is caught even on a no-network CI leg where neither tagged baseline tree can be materialized. The exact new total is reasoned, not re-measured locally (this file's own strace gate needs Linux), and will be confirmed by this pull request's own CI run. diff --git a/scripts/post-tool-hook.sh b/scripts/post-tool-hook.sh index 3a9ffdb6..359e21ca 100755 --- a/scripts/post-tool-hook.sh +++ b/scripts/post-tool-hook.sh @@ -206,28 +206,32 @@ fi # -- same extractor mechanism, same finding (on every # host checked, `tool_input` is positioned AFTER the top-level `cwd` field, # so this stays the safe nested-after case), not repeated here. -_stdin_json_string() { - local field="$1" raw="$2" rest prefix value dq - # The double quote is held in `dq` (octal 042) rather than written - # backslash-escaped: the plugin directory's scanner mis-tracks an - # escaped quote (#898 round 8). Same patterns, same quoting of $field. - printf -v dq '\042' - [[ "$raw" == *"$dq$field$dq"* ]] || return 1 - rest=${raw#*"$dq"$field"$dq"} - prefix=${rest%%"$dq"*} - if [[ "$prefix" == *[!:[:space:]]* ]]; then return 1; fi - value=${rest#*"$dq"} - value=${value%%"$dq"*} +# 1029: VARNAME is the new first argument, written via printf -v instead +# of printed, so a call site forks no subshell -- same shape as +# session-start-hook.sh own _stdin_json_string_into, 665/660. +_stdin_json_string_into() { + local _sjsi_var="$1" _sjsi_field="$2" _sjsi_raw="$3" _sjsi_rest _sjsi_prefix _sjsi_value _sjsi_dq + printf -v "$_sjsi_var" '%s' "" + # Every local is _sjsi_-prefixed, as in session-start-hook.sh: printf -v + # resolves against the innermost local, so an unprefixed `value` here + # would swallow a caller's VARNAME of the same name. + printf -v _sjsi_dq '\042' + [[ "$_sjsi_raw" == *"$_sjsi_dq$_sjsi_field$_sjsi_dq"* ]] || return 1 + _sjsi_rest=${_sjsi_raw#*"$_sjsi_dq"$_sjsi_field"$_sjsi_dq"} + _sjsi_prefix=${_sjsi_rest%%"$_sjsi_dq"*} + if [[ "$_sjsi_prefix" == *[!:[:space:]]* ]]; then return 1; fi + _sjsi_value=${_sjsi_rest#*"$_sjsi_dq"} + _sjsi_value=${_sjsi_value%%"$_sjsi_dq"*} # A JSON encoder writes each backslash as `\\` -- a Windows `cwd` from # Codex arrives as `C:\\work\\proj` otherwise (#829). - value=${value//\\\\/\\} - [ -n "$value" ] || return 1 - printf '%s' "$value" + _sjsi_value=${_sjsi_value//\\\\/\\} + [ -n "$_sjsi_value" ] || return 1 + printf -v "$_sjsi_var" '%s' "$_sjsi_value" } # The double quote, for the last-save.json scope check and the sanitiser # log line below -- held in a variable rather than backslash-escaped, the -# same reason as dq in _stdin_json_string (#898 round 8). +# same reason as _sjsi_dq in _stdin_json_string_into (#898 round 8). printf -v _pt_dq '\042' # ── The cwd the host handed us (#411, #444) ───────────────────────────── @@ -247,7 +251,7 @@ printf -v _pt_dq '\042' # not. Precedence is CLAUDE_PROJECT_DIR, then this, then the existing # .claude/remember layout derivation, then the existing failure -- resolved # entirely inside resolve-paths.sh, unchanged by this hook. -REMEMBER_HOOK_CWD=$(_stdin_json_string cwd "$HOOK_STDIN" 2>/dev/null) || REMEMBER_HOOK_CWD="" +_stdin_json_string_into REMEMBER_HOOK_CWD cwd "$HOOK_STDIN" 2>/dev/null if [[ "$REMEMBER_HOOK_CWD" == *$'\n'* ]] \ || [[ "$REMEMBER_HOOK_CWD" == *$'\r'* ]]; then REMEMBER_HOOK_CWD="" @@ -418,10 +422,10 @@ fi # --- Which session is this invocation FOR? (#212) --- # HOOK_STDIN was already captured above, ahead of path resolution (#444, so # the same payload's `cwd` can feed resolve-paths.sh's REMEMBER_HOOK_CWD -# fallback), and _stdin_json_string already defined there. session_id is +# fallback), and _stdin_json_string_into already defined there. session_id is # extracted from that same capture rather than reading stdin a second time — # it is a single stream and a second read here would see EOF. -STDIN_SESSION_ID=$(_stdin_json_string session_id "$HOOK_STDIN" 2>/dev/null) || STDIN_SESSION_ID="" +_stdin_json_string_into STDIN_SESSION_ID session_id "$HOOK_STDIN" 2>/dev/null # stdin is not more trustworthy than a basename. The id becomes both a path # component under capture-alive.d/ and a transcript filename, so it faces the # same guard the basename-derived id now faces too (#620) -- both are @@ -466,7 +470,7 @@ fi # return is rejected outright; whether the value actually names a file is # the `[ -f ]` check below, not a character allowlist -- a transcript path # legitimately contains slashes and dots. -STDIN_TRANSCRIPT_PATH=$(_stdin_json_string transcript_path "$HOOK_STDIN" 2>/dev/null) || STDIN_TRANSCRIPT_PATH="" +_stdin_json_string_into STDIN_TRANSCRIPT_PATH transcript_path "$HOOK_STDIN" 2>/dev/null if [[ "$STDIN_TRANSCRIPT_PATH" == *$'\r'* ]]; then STDIN_TRANSCRIPT_PATH="" fi diff --git a/tests/test_locale_ranges_695.py b/tests/test_locale_ranges_695.py index 097ddc03..9f7d5f54 100644 --- a/tests/test_locale_ranges_695.py +++ b/tests/test_locale_ranges_695.py @@ -199,7 +199,7 @@ def _is_protected(text: str, line_no: int, line: str) -> bool: # from the scan, so that one libc's behaviour stays visible and can be # re-checked rather than quietly assumed forever. ("scripts/agy-stop-hook.sh", 159): _CASE, - ("scripts/post-tool-hook.sh", 449): _CASE, + ("scripts/post-tool-hook.sh", 453): _CASE, # Lines shifted by #913's SESSION_DIR cache sidecar, inserted above # these three (session-dir-cache does not touch SESSION_ID or add a new # bracket range of its own -- these are the same three pre-existing @@ -215,8 +215,8 @@ def _is_protected(text: str, line_no: int, line: str) -> bool: # unchanged rewrite landed between the first and the other two), and once # more by #1011's own self-review round (a stale-comment fix above the # glob-walk, plus a longer comment on the capture-alive in-place write). - ("scripts/post-tool-hook.sh", 767): _CASE, - ("scripts/post-tool-hook.sh", 810): _CASE, + ("scripts/post-tool-hook.sh", 771): _CASE, + ("scripts/post-tool-hook.sh", 814): _CASE, # The #913 transcript-scan sidecar's own key: a filename component, same # [A-Za-z0-9._-]+ charset as SESSION_ID right above, never locale text -- # measured the same way, same reason. Shifted +27 (957->984, 1059->1086) @@ -235,8 +235,8 @@ def _is_protected(text: str, line_no: int, line: str) -> bool: # class this whole round exists to remove), and +4 for the comment # above `_nl_count` that now describes the herestring instead of the # pipe it replaced. - ("scripts/post-tool-hook.sh", 1001): _CASE, - ("scripts/post-tool-hook.sh", 1103): _CASE, + ("scripts/post-tool-hook.sh", 1005): _CASE, + ("scripts/post-tool-hook.sh", 1107): _CASE, ("scripts/session-end-hook.sh", 201): _CASE, ("scripts/session-end-hook.sh", 209): _CASE, ("scripts/session-start-hook.sh", 289): _CASE, diff --git a/tests/test_post_tool_total_processes_1011.py b/tests/test_post_tool_total_processes_1011.py index aeef7845..bd3442d5 100644 --- a/tests/test_post_tool_total_processes_1011.py +++ b/tests/test_post_tool_total_processes_1011.py @@ -48,6 +48,38 @@ number nobody re-derived. `_pt_scan_lines` and its call site are a few processes out of a much larger total; this file no longer pretends it is measuring their contribution alone. + +#1029 (follow-up investigation and fix): this file's own fixture -- a +brand-new tmp_path (HOME, PROJECT, REMEMBER_DIR, TMPDIR all freshly created) +for every single one of the `n` repeated runs -- measures ONLY the +cold-start cost: no prior invocation has ever published lib-env-cache.sh's +session cache, the SESSION_DIR cache, or log.sh's cfg-flatten cache for that +tree, so every run takes the full slow path (resolve-paths.sh -> +detect-tools.sh -> bootstrap-dirs.sh -> log.sh, lib-memory-dir.sh's +worktree-detection forks, `_config_load`'s jq read, the mktemp+mv cache +publishes). On the WARM path -- any call after the first in a real session, +which is the common case once a project has run once -- the fast-path review +of this round (self-review finding) located a real cut that ran UNCONDITIONALLY +on BOTH paths, not cold-start-only: `_stdin_json_string` 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, each forking a subshell even though the function itself is pure +parameter expansion -- the same cost class lib-clock.sh's own +`_remember_date_into` comment documents. Ported to the fork-free +`_stdin_json_string_into` form session-start-hook.sh already used (#665, +part of #660) -- same matching algorithm, no behaviour change, three fewer +forks on EVERY invocation of this hook. What remains on the warm path after +this cut: `_pt_scan_lines`'s own `tail`+`wc` (lines ~959/989), which the +comment immediately above that `wc` call documents as unavoidable without +reintroducing #1017's quadratic bash-expansion regression. Every OTHER +candidate the issue named (bootstrap-dirs.sh's mkdir, the mktemp+mv +atomic-write pattern, the jq config read) traced to either (a) cold-path-only +work already gated behind existence checks (#230/#666), or (b) work inside +the byte-pinned lib-memory-dir.sh (tests/test_case_divergence_298.py). The +absolute-budget assertions below are tightened to reflect this cut (REASONED +near the new total, not re-measured locally -- this file's own strace gate +needs Linux, not available here) and exist so a future reduction or +regression on this cold path is still caught even on a no-network CI leg. """ from __future__ import annotations @@ -220,6 +252,27 @@ def test_below_threshold_path_total_forks_vs_tagged_baselines(tmp_path): head_min, head_all = _min_forks_for_tree( tmp_path, "head", HOOK, REPO_ROOT, n=n) + # #1029: an ABSOLUTE budget, independent of whether either tagged tree can + # be materialized here (no network reaching origin skips BOTH relative + # assertions below, leaving nothing that can fail -- see the skip branch + # a few lines down). HEAD measured 67 on every one of 9 runs across every + # ubuntu/Python CI leg as of #1028; +2 is slack for ordinary noise + # (scheduler jitter, background reaping), not room for a new fork class + # to creep back in unnoticed the way #1011 round 1 did. + assert head_min < 67, ( + f"below-threshold path, whole-hook strace total -- HEAD min={head_min} " + f"all={sorted(head_all)} -- exceeds the absolute budget of 67 (this " + f"round's own cut -- three unconditional per-call forks removed from " + f"_stdin_json_string, #1029 -- is REASONED to land near 64, not " + f"measured locally since this file's own strace gate requires Linux; " + f"67 is #1028's own prior total, so this budget is strictly tighter " + f"than before even if the reasoned estimate is off, and still never " + f"<=69 -- see the below comment for why that bound was wrong); this " + f"must hold even when neither tagged baseline tree below can be " + f"materialized, so a regression is never silently unchecked just " + f"because this CI leg has no network" + ) + results = {"HEAD": (head_min, head_all)} skipped = [] for tag in ("v0.42.4", "v0.43.0"): @@ -299,3 +352,14 @@ def test_cooldown_path_forks_fewer_than_below_threshold_path(tmp_path): f"no longer being skipped under cooldown, or this counting method " f"cannot see the decrease it exists to detect" ) + # #1029: absolute budget for the cooldown path too, same reasoning as the + # below-threshold path's own budget above -- a relative check alone + # ("fewer than below-threshold") would still pass if BOTH paths silently + # grew by the same amount. + assert cool_min < 67, ( + f"cooldown path, whole-hook strace total -- min={cool_min} " + f"all={sorted(cool_all)} -- exceeds the absolute budget of 67 " + f"(tightened for #1029's own cut, same three unconditional forks " + f"removed on this path too; see the below-threshold test's own " + f"budget comment for the full reasoning)" + ) diff --git a/tests/test_stdin_extractor_top_level_wins_447.py b/tests/test_stdin_extractor_top_level_wins_447.py index 8ff2d517..03b38b67 100644 --- a/tests/test_stdin_extractor_top_level_wins_447.py +++ b/tests/test_stdin_extractor_top_level_wins_447.py @@ -158,8 +158,8 @@ def _nested_before(key: str, top: str, nested: str) -> str: pytest.param("scripts/session-start-hook.sh", "_stdin_json_string_into", True, "session_id", id="session-start/session_id"), pytest.param("scripts/session-end-hook.sh", "_stdin_json_string", True, "cwd", id="session-end/cwd"), pytest.param("scripts/session-end-hook.sh", "_stdin_json_string", True, "session_id", id="session-end/session_id"), - pytest.param("scripts/post-tool-hook.sh", "_stdin_json_string", True, "cwd", id="post-tool/cwd"), - pytest.param("scripts/post-tool-hook.sh", "_stdin_json_string", True, "session_id", id="post-tool/session_id"), + pytest.param("scripts/post-tool-hook.sh", "_stdin_json_string_into", True, "cwd", id="post-tool/cwd"), + pytest.param("scripts/post-tool-hook.sh", "_stdin_json_string_into", True, "session_id", id="post-tool/session_id"), pytest.param("scripts/user-prompt-hook.sh", "_stdin_cwd", False, "cwd", id="user-prompt/cwd"), ] diff --git a/tests/test_stdin_json_backslash_decode_829.py b/tests/test_stdin_json_backslash_decode_829.py index 7dbde442..bb622519 100644 --- a/tests/test_stdin_json_backslash_decode_829.py +++ b/tests/test_stdin_json_backslash_decode_829.py @@ -55,7 +55,7 @@ def _call_into(script: str, func_name: str, keyed: bool, key: str, raw: str): EXTRACTORS = [ pytest.param("scripts/session-start-hook.sh", "_stdin_json_string_into", True, True, id="session-start/into"), pytest.param("scripts/session-end-hook.sh", "_stdin_json_string", True, False, id="session-end"), - pytest.param("scripts/post-tool-hook.sh", "_stdin_json_string", True, False, id="post-tool"), + pytest.param("scripts/post-tool-hook.sh", "_stdin_json_string_into", True, True, id="post-tool"), pytest.param("scripts/user-prompt-hook.sh", "_stdin_cwd", False, False, id="user-prompt"), pytest.param("scripts/user-prompt-hook.sh", "_stdin_cwd_into", False, True, id="user-prompt/into"), ]