Repository navigation
fix(dev): install hooks during worktree bootstrap - #11496
Conversation
Husky runs `git config core.hooksPath .husky/_` with no --worktree, so from a linked worktree the value lands in the config every worktree of the repository shares while `.husky/_` is created only in the checkout being bootstrapped. Skip the Husky step and report it when the key is unset and this checkout does not own the repository config, so a bootstrap can no longer repoint hook resolution for roots that never received the wrappers. A primary checkout still installs hooks, and an already-configured `core.hooksPath` is untouched. Also drop the caller's success exit, which `install()` made unreachable when it started exiting on every successful path, and bring the pnpm-worktree-bootstrap design doc in line with a hook step it still recorded as deliberately skipped. The new fixture runs the real script against a throwaway root whose `.git` is a file or a directory and whose config comes from a real `git init` repo, which makes both new branches reachable and pins the fail-closed guard: the injected `GIT_CONFIG_*` constant holds one value for the child's whole lifetime and cannot express the unset state that asks husky to write. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-closeout/jmtuo29vduf
|
@qwen-code /triage |
`ownsRepositoryConfig()` inferred repository-config ownership from the
filesystem shape of `.git`, and the proxy is wrong at both ends. With no
`.git` at all, `statSync(..., { throwIfNoEntry: false })` returns `undefined`
and the predicate folded that absence into "owns the config", so a
repository-less checkout ran husky into its `.git can't be found` soft failure
(exit 0) and the fail-closed check then turned a successful dependency install
into exit 1 blaming Husky. A `.git` file is not only a linked worktree either:
`git clone --separate-git-dir` checkouts and submodules have one too and do own
their config, so hooks were declined where they would have been correctly
scoped.
Ask git instead: `rev-parse --git-dir` differs from `--git-common-dir` only in
a linked worktree, and a failed `rev-parse` names the no-repository state, so
the skip notice stops asserting "linked worktree" and the bootstrap does not
gain a hard git dependency.
Also bind the fail-closed check to an artefact husky's own write produced, not
only to the config value. husky 9.1.7 exits 0 on every soft-failure path
(`index.js:16` git command not found, `index.js:17` refused `git config` write)
before the `mkdirSync(_())` on line 19, and a linked worktree inherits
`core.hooksPath` from the config it shares, so re-reading that value compared
it against itself and passed exactly when husky had created nothing.
Restore the registry-fallback case's hermeticity: `PATH` holds only the stub
directory again, which now also pins that the retry needs no ambient git.
Rebuild the ownership fixture from real git layouts, because `rev-parse`
resolves nothing for a `mkdirSync`'d `.git` or a hand-written `gitdir:` file,
and give the stub husky a failing mode so the exit code husky returns is pinned
rather than the install result's.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Patrol-Run: qwen-pr-closeout/jmtuys2rjuu
- getHooksPath() no longer collapses a refused git config read (exit 128/2/3) into "unset": only an absent key (exit 1) or a missing git binary keeps the skip path; anything else fails the bootstrap with the read error instead of a green, hook-less worktree. - The linked-worktree skip notice now names the recovery path: re-run this script once the primary checkout has hooks installed. - Tests cover a git stub exiting 128, the real-world unset HUSKY state, and pin the recovery sentence in the skip notice.
|
The red
Likely origin (attribution, not blame): No fix has landed yet. Live main head is now What this PR needs: nothing from the author's side. The check goes green once main is fixed and this branch re-runs or re-merges main. A bot review ( |
Pick up main's 2488d12 (fix(web-shell): realign two suites with shipped behavior to unbreak main CI, #11530). This branch's Test job failed only in packages/web-shell (WorkspaceSessionProvider.loading.test.tsx and WebShellSidebar.brand.test.tsx), which it does not touch -- those were main's own failures, already fixed on main. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Patrol-Run: qwen-pr-conflict/jmtva7llqva
|
@qwen-code /triage |
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Approval blockers: none.
Triage: Standard — contained fix with comprehensive test coverage.
What I checked:
-
Husky skip conditions — correctly identifies when NOT to run husky:
HUSKY=0env set → skip (respects opt-out)- Custom hooks path set and differs from
.husky/_→ skip (preserves existing config) - No repository (
ownership === 'none') → skip (no config to write) - Linked worktree + unset
core.hooksPath→ skip (would repoint shared config for all worktrees)
-
Config ownership probe (
repositoryConfigOwnership) — correctly distinguishes:'owns':--git-dir === --git-common-dir(primary checkout or--separate-git-dirclone) → husky is safe'linked': linked worktree where config is shared → husky only safe ifcore.hooksPathalready set'none': no repository → skip
-
Fail-closed verification — after husky exec, checks BOTH
core.hooksPath === '.husky/_'AND.husky/_/pre-commitexists. Husky exits 0 on soft failures (.git can't be found, refusedgit configwrite), so both proofs are required. -
envValue()helper — correctly extracts case-insensitive env reads on Windows, factored out frompathValue(). -
Test coverage — comprehensive:
- Normal flow: install + husky exec
HUSKY=0opt-out: only install- Custom hooks path: only install
- Unset HUSKY env: still reaches husky (not treated as disabled)
- Primary checkout: hooks install
- Linked worktree with unset
core.hooksPath: skipped --separate-git-dirclone: hooks install (owns its own config)- No repository: skipped
- Husky configures path but writes no wrappers: fail-closed
- Husky exits 0 writing nothing: fail-closed
- Husky fails with non-zero exit: propagates exit code
- Git refuses to read config (exit 128): surfaces error
-
Cross-check:
qwen-code-ci-botapproved. The Critical from previous round is fixed and pinned by a test that fails without it.
Unreviewed dimensions: Windows/Linux behavior (covered by existing worktree smoke workflow per PR description).
Reviewed with AI assistance.
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical-only scan at b7882567 — both historical Criticals verified fixed in the current code; no new blocking defect. Approving.
Read in full: scripts/setup-worktree.js (183 lines at this head), the ownership and hook-install decision path, and the changed fixtures in scripts/tests/package-scripts.test.js. The design-doc hunk is documentation and carries no runtime surface.
Historical blocking issues — both confirmed fixed, by reading this head rather than the replies
Two [Critical] findings were filed against earlier commits, the second one fix-induced by the first one's remedy. Both are certifies-falsely + regression. I verified each against the code as it stands now.
Round-1 Critical (husky invoked from a linked worktree rewrites the config every sibling worktree shares) — fixed and still fixed. The skip is intact at setup-worktree.js:119-131: when core.hooksPath is unset and the checkout does not own the repository config, husky is never invoked and the script exits with the install result, so the shared config is not written.
Round-2 Critical (ownsRepositoryConfig() inferred ownership from the filesystem shape of .git, misclassifying both ends) — fixed. The .git-shape proxy is gone entirely: neither statSync nor throwIfNoEntry appears anywhere in the file now. Ownership comes from git, at setup-worktree.js:93-102:
const probe = spawnSync('git', ['rev-parse', '--git-dir', '--git-common-dir'], { cwd: rootDir, env, encoding: 'utf8' });
if (probe.status !== 0) return 'none';
const [gitDir, commonDir] = probe.stdout.trim().split(/\r?\n/);
return gitDir === commonDir ? 'owns' : 'linked';That is the discriminator the finding prescribed, and it closes each of the three states it named:
- No
.gitat all →rev-parsefails →'none'→ the skip branch at :120-121 fires and prints "git could not resolve a repository for this checkout; skipping Husky because there is no repository config for it to write", thenexitWithResult(result)on the successful install. The regression — a source tarball or zip checkout turning a clean dependency install intoexit 1blaming Husky — cannot occur, because husky is never reached in that state. .gitas a file wheregit-dir == git-common-dir(a--separate-git-dirclone, a submodule) →'owns'→ husky runs and hooks are installed. The silent skip that left every commit in such a checkout bypassing.husky/pre-commitis gone.- A genuine linked worktree → the two paths differ →
'linked'→ skipped with the config left alone, which is what keeps the round-1 blocker fixed.
The notice text no longer asserts "linked worktree" as the explanation for a .git file; each branch names its own state, as the finding required.
The nested-repository false success is also closed, by a different mechanism than the finding proposed but a sufficient one. A checkout nested in a repository that already carries core.hooksPath=.husky/_ does not match the skip condition, so husky runs and is then held to two proofs at :139-147 — the config value must read .husky/_ and .husky/_/pre-commit must exist on disk — otherwise the script exits 1. Since husky exits 0 on soft failures, requiring both is what stops a run from reporting success while having created nothing.
The two facts the finding said a fix must not violate still hold. expect(hookless.status).toBe(1) and expect(hookless.stderr).toContain('Husky did not install hooks') are present at package-scripts.test.js:509-510, alongside the configOnly assertion at :504.
The fixture constraint was honoured, which is the part most likely to have been missed. The finding warned that .git stand-ins are valid only while the script uses statSync, and that mkdirSync(checkout/'.git') or a hand-written gitdir: file resolves to nothing under rev-parse. No such stand-in remains — a search for mkdirSync on a .git path and for a literal gitdir: returns nothing in the test file. The fixtures now build real layouts with real git: git init --quiet (:386), git worktree add --quiet --detach (:393) and a --separate-git-dir clone (:400), which is exactly the P/L/S set the finding's witness enumerated, and :476 asserts the --separate-git-dir shape installs hooks rather than skipping.
Independent scan of the current code — no Critical
getHooksPath()(:69-85) maps exit 0 to the value, exit 1 or a spawn error toundefined, and fails closed with a diagnostic on any other status. Treating "git unavailable" the same as "key absent" is what prevents a missing git from becoming a spuriousexit 1, and the ownership probe independently reports that shape as'none'. The remaining fail-closed arm needs an already-unreadable git config to trigger, a state in which the hooks decision genuinely should not be guessed.- The decision order in
install()is correct: theHUSKY=0opt-out and an explicit non-huskycore.hooksPathboth exit with the install result before ownership is consulted, so a pre-existing hook configuration is never overridden. - The registry-retry driver at :172-183 is unchanged in substance and only reached on a failed install, since
install()exits on every success path. - Nothing here writes to a shared git config, touches secrets, or can lose work.
spawnSync('git', …)without a shell is fine on Windows, whererunPnpmalready setsshell: process.platform === 'win32'.
CI
Green at this head with nothing attributable to the diff: Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke, both Desktop Shell jobs and all three Install jobs (ubuntu, macOS, windows) pass — the Install legs matter here because a case in this suite smoke-tests the real worktree bootstrap on every supported host. Only review-pr is still in progress, which is not a gate on this review. A fresh sandboxed verification was running at publish time; I did not wait on it.
3 participants
What this PR does
Ensures the optimized pnpm worktree bootstrap installs the repository's Husky hooks after dependency installation. It preserves an existing custom hooks path, respects the
HUSKY=0opt-out, and fails if Husky reports success without configuring the expected hooks path.Why it's needed
The worktree bootstrap intentionally skips the root prepare phase to avoid rebuilding and bundling the project. That also skipped Husky, so a fresh worktree could have
core.hooksPath=.husky/_without the generated hook wrappers. Commits from that worktree then bypassed the Prettier and ESLint pre-commit checks, leaving CI to report formatting failures.Reviewer Test Plan
How to verify
Create a fresh linked worktree, run the documented pnpm worktree bootstrap, and confirm that the pre-commit hook is generated and executable. Confirm that a custom hooks path and
HUSKY=0are left unchanged.Evidence (Before & After)
N/A (non-UI change). Before the fix, the bootstrap completed successfully while the generated pre-commit hook remained absent and a traced commit invoked no hook. After the fix, the bootstrap creates the hook, retains the expected hooks path, and the regression tests cover the normal, opt-out, custom-path, offline, and registry-fallback paths.
Tested on
Environment (optional)
macOS arm64, Node.js 22.22.0, pnpm 11.24.0.
Risk & Scope
Linked Issues
Related CI failure: https://github.com/QwenLM/qwen-code/actions/runs/34353187365/job/102471375065?pr=11270
中文说明
本 PR 做了什么
确保优化后的 pnpm worktree 初始化在依赖安装完成后安装仓库的 Husky hooks。同时保留已有的自定义 hooks 路径、尊重
HUSKY=0的禁用设置，并在 Husky 返回成功但未配置预期 hooks 路径时让初始化失败。为什么需要这个改动
worktree 初始化为了避免重新构建和打包项目，会有意跳过根目录的 prepare 阶段，但这也一并跳过了 Husky。因此，新 worktree 可能已配置
core.hooksPath=.husky/_，却没有生成 hook 包装脚本；从该 worktree 提交时就会绕过 Prettier 和 ESLint 的 pre-commit 检查，最后由 CI 报出格式错误。Reviewer 测试计划
如何验证
创建一个新的 linked worktree，运行文档中的 pnpm worktree 初始化，然后确认 pre-commit hook 已生成且可执行。再确认自定义 hooks 路径和
HUSKY=0设置不会被修改。证据（修复前后）
不适用（非 UI 改动）。修复前，初始化会成功完成，但生成的 pre-commit hook 仍不存在，跟踪提交过程也确认没有调用 hook。修复后，初始化会生成 hook 并保留预期的 hooks 路径；回归测试覆盖正常、禁用、自定义路径、离线安装和联网回退路径。
测试平台
环境（可选）
macOS arm64、Node.js 22.22.0、pnpm 11.24.0。
风险与范围
关联问题
相关 CI 失败：https://github.com/QwenLM/qwen-code/actions/runs/34353187365/job/102471375065?pr=11270