Repository navigation
perf(animation,speech,explainer-video): skip the pydeps hook's probes while its ready stamp holds - #12473
Conversation
… while its ready stamp holds The install-python-deps SessionStart hook started about six processes on the ready path (interpreter probe, cygpath twice, pydeps.py, its interpreter probe and the load probe), which set the SessionStart critical path on Windows. - lib/pydeps.py: after a successful install through an interpreter found on PATH (never one only the py launcher listed), write <data>/python/.hook-ready atomically: a format tag, the plugin root, the installed directory and what each of the hook's candidate names resolves to. run drops it when the set is missing or a script exits non-zero; check drops it when a row fails. - lib/install-python-deps.sh: before any probe, exit 0 when the stamp holds, using shell builtins only (hash/BASH_CMDS for the PATH lookup, pure string drive-letter conversion, -nt and -d). Anything else takes the full path. shfmt now applies the repository's switch_case_indent setting to the file. - Copies regenerated with scripts/sync-shared-copies.sh. - Tests in each plugin: the hook starts no Python while the stamp holds and takes the full path on each invalidation; pydeps.py writes the stamp only through a PATH candidate and run and check drop it. - on-demand-dependencies contract 2.4.0: Rules P2-P4 name the stamp, what invalidates it, and the residual repointed-symlink hole. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…amp holds under Git Bash A stamp written the way native Windows Python writes it (backslashes, its own letter case, .EXE suffixes) holds under OSTYPE=msys and starts no Python; the same stamp under linux-gnu takes the full path, so the case and suffix folding applies only under Git Bash and Cygwin. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ory, not OSTYPE Windows Git Bash on the laptop lane reports OSTYPE=cygwin yet mounts drives at /c/, so the fast path turned the stamp's C:\ root into /cygdrive/c/... and never matched the hook's /c/... root. as_posix now uses /<letter>/ when that directory exists, else /cygdrive/<letter>/, and fails when neither does, which sends the hook down the full path. The new hook test case runs the hook in a user and mount namespace whose root holds /c: a drive-letter stamp holds under OSTYPE=cygwin and msys. Before this fix it started Python under cygwin (starts cygwin=1 msys=0). The case skips where unprivileged user namespaces are not allowed. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…through the hook's names pydeps.py wrote the ready stamp when the chosen interpreter matched any name in CANDIDATES, which pydeps.json can extend (python3.14 for a plugin allowing 3.14). The stamp and the hook check only HOOK_CANDIDATES, so a later change to such an interpreter would not invalidate the stamp. The stamp is now written only when the chosen path is a HOOK_CANDIDATES resolution. A new test in each plugin shows an interpreter chosen through python3.14 writes no stamp. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
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. |
|
Hold (do-not-merge): no CI job exercises the Windows fast path yet. The namespace-based drive-mount case may skip on GitHub's Ubuntu runners, and Windows CI runs none of these suites. The first version's |
![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: a4917bfe0e
ℹ️ 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".
… under real Git Bash in Windows CI Nothing in CI exercised the Windows fast path: the drive-mount case needs unprivileged user namespaces and Windows CI ran none of these suites. - Each hook test gains a case that runs only under Git Bash or Cygwin with cygpath. A real install writes the stamp; the test rewrites it in Windows form from the fixture's own paths (cygpath -w for the root, target and each candidate, .EXE where shutil.which would add it) and asserts the hook starts no Python, counted by a sitecustomize on PYTHONPATH that logs each start. Without the stamp the hook does start Python. On Windows the suite ends there: the later cases stub interpreters with shell scripts, simulate paths or need Linux namespaces. Elsewhere the case prints a SKIP line. - pr-test-platforms.yml test-windows-hooks runs the three suites, and scripts/test-windows-plan.txt keys them to each plugin's hook and pydeps suites, which a change to lib/pydeps.py, lib/install-python-deps.sh or any copy selects. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
claude-security-review reviewed Security review (incremental since
|
|
claude-review reviewed Code review: drive-stamp
|
…s as files, not strings Windows CI caught the stamp failing on one directory spelled two ways: pydeps.py records the resolved root (C:\Users\runneradmin\...), while the hook's ROOT comes through Git Bash's /tmp mount, an 8.3 short name (RUNNER~1). String comparison cannot match those, nor a symlinked root. stamp_holds now uses the builtin -ef (same device and inode): the stamp root's scripts/pydeps.py against $ROOT/scripts/pydeps.py, and each candidate's recorded resolution against BASH_CMDS, both empty or both the same file. as_posix stays for drive-letter conversion. path_key's case and .exe folding is gone, since -ef matches across case, 8.3 names, mounts and a dropped .exe (checked on Windows Git Bash). A new Linux case reaches the plugin root through a symlink and must start no Python. Before this fix it started four. The simulated Windows cases keep backslash and drive-letter conversion only; the real Git Bash case covers case, 8.3 names and .EXE. The contract states the file comparison. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Owner decision (2026-11-02, in the lane-2 session): "Yeah, let's go with recommendations." The ready marker replaces the per-start package check for speech, animation and explainer-video, with contract Rules P2, P3 and P4 amended as in this PR. Basis: the Windows measurement in Verification (critical path p50 4370 → 2114 ms). The |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…p under Git Bash, not a hand-written one The real Git Bash case rewrote the stamp by hand and appended .EXE after cygpath -w, which already returns python3.13.exe for a file that exists only with the suffix. The recorded python3.13.exe.EXE named no file, so the hook's -ef check rightly failed (Windows CI job 125502035270). Production never hand-writes the stamp, so the case now keeps the stamp the real install wrote, asserts its root line is a Windows drive-letter path, and then asserts the hook starts no Python. The fixture stays under /tmp, so the stamp's long-form root and the hook's 8.3-mapped ROOT still spell one directory two ways. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…tamped run fails A stamped set whose directory still exists but no longer imports failed the skill's script, and run() dropped the stamp silently, while Rule P4 promises the repair line. run() now prints one stderr line when the script exits non-zero and a ready stamp was present: the run failed, the next session start re-probes and rebuilds, or run the repair line now. The script's own exit code stands. With no stamp, a failing script prints nothing extra. A new test in each plugin's pydeps suite covers both cases. Rule P4 now says so, and its "Neither falls back" sentence is back after the sentences it refers to. The hook tests' comment on the simulated Windows case names only what the real Git Bash case covers: mounts, 8.3 names and .EXE suffixes, not letter case. Refs: #12258 Co-Authored-By: Claude Opus 5.5 <[email protected]>
…e ready stamp outlives A pyenv, asdf or mise shim keeps its path and file time across a version switch, so the hook's resolution and newer-file checks both pass. Name it beside the repointed-symlink case; both heal on the first skill run. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ashes out of the printf format lint-shell's shell-portability check flags \\b in a printf format, which some shells read as a backspace escape. Pass the backslashes as arguments; the stamp text is unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Closes #12258
Summary
On Windows, the
install-python-deps.shSessionStart rows ofexplainer-video,speechandanimationset the SessionStart critical path. Each row started about six processes even when its packages were already installed: the hook's interpreter probe, twocygpathcalls,pydeps.py, its interpreter check and handover, and a load probe that imports the package set. After this change, a session whose package set is ready starts no process in these rows. Measured on a Windows lane, the critical path drops from 4370 to 2114 ms at the median, and these rows no longer set it.Fix
Ready stamp (
lib/pydeps.py). After a successful install with an interpreter found through one of the hook's candidate names,pydeps.pywrites<data>/python/.hook-readyatomically. It records the plugin root, the installed directory, and what each candidate name (python3.13,python3.12,python3,python, pluspyon Windows) resolves to. An interpreter that only thepylauncher listed writes no stamp.Fast path (
lib/install-python-deps.sh). Before any probe, the hook checks the stamp with shell builtins only. It keeps the stamp only when all of these hold:requirements.txt,pydeps.pyorpydeps.jsonis newer than the stamp;Anything else takes the full path unchanged. Windows paths are converted with string operations; the drive mount (
/c/or/cygdrive/c/) is chosen by which directory exists. Git Bash on the measured host reportsOSTYPE=cygwinyet mounts drives at/c/, which the first version got wrong.Self-heal.
pydeps.py rundrops the stamp when the set is missing or a script exits non-zero, andcheckdrops it on any FAIL. A set that breaks then surfaces at skill time with the repair line, and the next session start probes and rebuilds it.Contract.
docs/contracts/on-demand-dependencies2.4.0 amends the Python rules:checkand on the repair line.It names the remaining gap: a candidate symlink repointed to an interpreter file older than the stamp passes until one failed skill run drops it.
Alternatives rejected.
async: trueis out. Under-p, Claude Code kills an async hook still running at teardown, so a headless first install would never finish. An async hook'ssystemMessagealso isn't shown to the user, which breaks Rule P4 (https://code.claude.com/docs/en/hooks#run-hooks-in-the-background, read 2026-11-02). Trimming one stage alone would not work: no single stage dominates, and the load probe is 448-1905 ms of a 1989-3151 ms row.Copies. The six plugin copies are regenerated by
scripts/sync-shared-copies.sh. Each plugin carries a patch changelog fragment.Verification
Tests (Linux). Each change has a test that fails before it:
requirements.txt,pydeps.pyorpydeps.json, a missing target) shows the full path runs. A Windows-form stamp holds under bothOSTYPE=cygwinandmsyswith a/cmount; it is simulated in a user and mount namespace, and skips where unprivileged namespaces are blocked.runandcheck.origin/main's copies, every hook suite failedready stamp write, and each pydeps suite failed 2 and errored 1.starts cygwin=1 msys=0).Checks:
sync-shared-copies.sh --check(567 copies match) and--check-bump,check-changelog-fragments.sh, shellcheck, shfmt,scripts/run-ruff.sh checkand markdownlint are all clean.Windows measurement. Host: laptop Windows lane, Claude Code 2.1.312, the perf(hooks): SessionStart p95 7.1 s from about 26 per-plugin prerequisites rows; use one shared async runner #6633 method (headless
claude.exe -pstartups one at a time with--debug-file, slowest row per kind).--plugin-dircopies, with the installed three plugins disabled per run, comparing main df4c50f against this branch at 7d07e92. 5270798 only narrows when the stamp is written; the chosen interpreter here (python3) is one of the hook's names, so the timing holds for the head.bash -xtrace on Windows Git Bash showsstamp_holdsreturning 0 and no Python process started.Not covered: Windows CI does not run these suites, so the Windows half rests on the measurement above.
Related
playwright's owninstall-python-deps.shrow (its own variant, not a shared copy) now sets the critical path on this host: 2114 ms p50 in the fix arm.🤖 Generated with Claude Code