docs(hooks): add the hook-performance-levers program plan - #3625
kyle-sexton wants to merge 2 commits into
Conversation
…resolution Plan for measuring and cutting every runtime-cost surface the marketplace ships on the Windows Git Bash host: phases 0 to 8, scope fences, open questions with their arbiters, and the per-phase design gate verdicts. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011boRpMmkKhritwmR4ZPMvP
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @kyle-sexton's task in 4m 40s —— View job Claude finished the code review
This is a docs-only PR (2 new files, 1010 lines) adding a program plan and its design-resolution gate for a hook-performance effort. No code, hooks, or plugin versions change, so the review focused on the plan's internal accuracy and consistency — the closest analogue to "correctness" for a document that many later PRs will be executed against. Posted 3 inline comments:
Everything else checked out against the repo: the file counts, line numbers (e.g. |
…tions log The extended harness keeps max_ms absolute and appends cpu_x_s and max_x_s (spawn-equivalents) instead of a byte-identical max_abs_ms twin. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011boRpMmkKhritwmR4ZPMvP
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # hook-performance-levers | ||
|
|
||
| Draft for orchestrator review. Every phase is `[TODO]`. Branch: `perf/hook-fanout-consolidation` | ||
| (four commits ahead of `main` with the guardrails dispatcher, PR #3621, already shipped). |
There was a problem hiding this comment.
The plan's central premise ("guardrails dispatcher already shipped") does not match this repo's state.
This line, and Phase 4a/4b throughout, treat plugins/guardrails/hooks/run-guards.sh as an existing consolidated dispatcher that PR #3621 already shipped, replacing 8 per-Bash-call guard processes with 1. Checking the actual tree on this branch (and origin/main, one commit behind it):
run-guards.sh/run-guards.test.shdo not exist anywhere in the repository (grep -r run-guards .outside these two new docs returns nothing).plugins/guardrails/hooks/hooks.jsonstill declares 8 separatePreToolUseBash guard commands individually (no dispatcher entry).plugins/guardrails/.claude-plugin/plugin.jsonis at0.30.0, not the0.31.0this PLAN cites twice (Baseline values table, lines 631-632) as the source of the "8 processes → 1 process" accounting.- Neither
plugins/guardrails/README.mdnorCHANGELOG.mdmentions a dispatcher.
Phase 4a's file list, ALLOWED-files table, and Sanity Check (bash plugins/guardrails/hooks/run-guards.test.sh at PLAN.md:394) all assume this file exists today. The same false premise is restated in design/design-resolution.md lines 29-30 ("The dispatcher (plugins/guardrails/hooks/run-guards.sh) sources it once per event..."). If PR #3621 is still pending (its own PR body here calls it a "precursor," not the dispatcher itself), Phase 4a needs to either build the dispatcher itself or the plan needs a corrected baseline and an explicit precondition instead of asserting it's already in place.
|
|
||
| Measure and cut every runtime-cost surface this marketplace ships on the binding host (Windows 11, | ||
| Git Bash, Claude Code 2.1.258): hook spawn count and critical-path time per event, Windows-only | ||
| production no-ops, the skill listing payload, and the statusline. Eight PR-sized phases, each with a |
There was a problem hiding this comment.
The PR/phase count is inconsistent across the document. This line states "Eight PR-sized phases," and open question 1 (line 816) repeats "a checklist of the eight PRs." But:
- Open question 10 (line 851) says the goal condition caps the program at "60 turns across nine PRs."
- The
[EXEC-SHAPE]decision table (line 889) explicitly calls Phase 4c "A tenth PR."
Counting the actual phase list (1, 2, 3, 4a, 4b, 4c, 5, 6, 7, 8 — phase 0 is explicitly "Not a PR," and 4b is "promoted to its own topic" but still described here with its own PR-shaped Sanity Check and ALLOWED-files entry), none of "eight," "nine," or "tenth" is self-evidently the intended total. A tracking issue built off "eight PRs" (open question 1) and a turn budget built off "nine PRs" (open question 10) will disagree with each other and with the phase list itself.
| 9. **Tool-command guards on this host** block `cat >`, `printf >`, `echo >`, `python -c`, and any | ||
| literal disk-hygiene engine filename inside a Bash command string. Workers write files with the | ||
| Write tool. The hardcoded-path guard also blocks machine-specific paths in written files, so | ||
| this PLAN uses `~/` and `<placeholder>` forms. |
There was a problem hiding this comment.
Constraint 9 states "this PLAN uses ~/ and <placeholder> forms" to stay clear of the hardcoded-path guard, but the PLAN itself hardcodes D:/worktrees/dotfiles-perf-hooks and related D:/worktrees/... paths 7 times elsewhere (e.g. lines 82, 85-86, 122-124, 159-163, 196-197) — a specific operator's personal directory layout, not a placeholder. It won't trip the automated guard (hardcoded-path-check.sh's patterns match Windows user-home paths like C:\Users\<name>\, not arbitrary drive-root paths like D:/worktrees/...), but it does contradict this constraint's own stated intent, and bakes one operator's machine layout into a document other operators will execute against.
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 4m 1s —— View job Claude finished the
|
| Draft for orchestrator review. Every phase is `[TODO]`. Branch: `perf/hook-fanout-consolidation` | ||
| (four commits ahead of `main` with the guardrails dispatcher, PR #3621, already shipped). |
There was a problem hiding this comment.
This states PR #3621 (the guardrails dispatcher) is "already shipped," but DEVIATIONS.md — added in this same PR's second commit — records the opposite: "PR #3621 is still open... code phases branch from perf/hook-fanout-consolidation until #3621 merges." This line wasn't corrected to match. Since this brief is meant to be handed to future phase workers as ground truth (Phase 0/3 reference "the guardrails dispatcher" and cache-delivery state built on this branch), leaving the stale "already shipped" claim here risks a later phase assuming the dispatcher is on main when it isn't.
|
|
||
| Measure and cut every runtime-cost surface this marketplace ships on the binding host (Windows 11, | ||
| Git Bash, Claude Code 2.1.258): hook spawn count and critical-path time per event, Windows-only | ||
| production no-ops, the skill listing payload, and the statusline. Eight PR-sized phases, each with a |
There was a problem hiding this comment.
"Eight PR-sized phases" here (and "checklist of the eight PRs" at line 816) doesn't match Open question 10 at line 851: "the goal condition caps the program at 60 turns across nine PRs and two repositories." Counting the actual phase list (1, 2, 3, 4a, 4c, 5, 6, 7, 8 — excluding 4b, which is promoted to its own topic/PR track) gives nine, not eight. Since this plan leans on exact counts elsewhere (e.g. the 66-skill count, the 41-script count) and the turn budget in question 10 is computed off the PR count, this mismatch should be reconciled to one number.
|
|
||
| ## Open questions | ||
|
|
||
| 1. **Tracking issue.** No issue number exists for this program. Recommendation: one tracking issue |
There was a problem hiding this comment.
"No issue number exists for this program" appears to be stale: this PR's own description states "tracking issue #3623 stays open until phase 8 closes it," and the "Related" section links it. If #3623 already is the program's tracking issue, this open question is answered and should say so (and phase 8's final step, which closes the tracking issue, should name #3623 explicitly) rather than reading as an unresolved question for the operator.
|
Closing: docs/topics// is the Contract tier per docs/conventions/topic-docs/README.md, committed on the task branch only and pruned before merge, so a docs-only PR that lands the slice on main is the wrong shape (contract-slice-prune-gate red-lines it by design). The plan stays on the program branch perf/hook-performance-program; each phase PR carries only its own changes and the slice is pruned with a pointer at the end of the program. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6586e8446b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Draft for orchestrator review. Every phase is `[TODO]`. Branch: `perf/hook-fanout-consolidation` | ||
| (four commits ahead of `main` with the guardrails dispatcher, PR #3621, already shipped). |
There was a problem hiding this comment.
The reviewed commit is based directly on a34bd7d, and a repo-wide search of that tree finds no plugins/guardrails/hooks/run-guards.sh; plugins/guardrails/hooks/hooks.json still declares the eight guards separately. Consequently, the baseline assumptions and phases 4a/4b refer to a dispatcher, profiling mode, and tests that are unavailable when this plan is executed from its actual base. Land or rebase onto PR #3621 first, or make adding the dispatcher an explicit prerequisite phase rather than describing it as already shipped.
Useful? React with 👍 / 👎.
| discipline survive unchanged. Add a no-change skip: hash the extracted fields (excluding | ||
| `captured_at`) with a builtin-only digest or a cached copy of the last extracted string in the | ||
| same directory, and skip the write when equal, so an unchanged payload costs no rename. |
There was a problem hiding this comment.
When the extracted rate-limit fields remain unchanged for more than ten minutes, this no-change optimization would also stop refreshing captured_at. The authoritative reader contract in plugins/rate-limit-guard/reference/reader-contract.md treats a snapshot whose captured_at is older than ten minutes as stale, so active statusline refreshes would incorrectly push consumers into reactive-only mode. Any write-elision design must preserve a current observation timestamp or change the reader contract and freshness mechanism together.
Useful? React with 👍 / 👎.
1 participant
No related issue: this PR lands the program plan only; tracking issue #3623 stays open until phase 8 closes it.
Summary
Adds
docs/topics/hook-performance-levers/PLAN.mdand its design resolution. The plan covers phases 0 to 8 of the program that measures and cuts every hook and runtime-cost surface this marketplace ships on the Windows Git Bash host: the extended dotfiles harness, baseline capture,ifgates and matcher scoping, async on non-deciding hooks, the guardrails dispatcher hot path, per-Write formatter hot path, Windows path-form fixes, the skill listing budget, statusline cost, and the final accounting.Fix
Docs only. Two new files under
docs/topics/hook-performance-levers/. No code, hooks, or plugin versions change.Verification
scripts/affected-tests.sh --runon the branch (PLAN.md maps to the planning open-questions suite; the design file is on the no-suite allowlist).Related
🤖 Generated with Claude Code
https://claude.ai/code/session_011boRpMmkKhritwmR4ZPMvP