chore: sync align local command templates #6

jercik · 2026-10-24T06:56:52Z

jercik commented

2026-10-24 06:56:52 +00:00

Regenerates the repository's managed files with Align 13.0.0 so declaration-check recovery instructions use the current local repair command. Includes the staged-snapshot cleanup update and review wrapper update with the existing zero-failure requirement retained.

jercik added 1 commit

2026-10-24 06:56:52 +00:00

chore: sync align local command templates

Checks / pr-flow-canary (pull_request) Has been skipped

Details

Checks / reuse (pull_request) Successful in 6s

Details

commit-msg / commitlint (pull_request) Successful in 11s

Details

Checks / quality-checks (24.15.0) (pull_request) Successful in 31s

Details

Checks / quality-checks (26.5.0) (pull_request) Successful in 31s

Details

Review / Review (pull_request_target) Successful in 5m12s

Details

58029b8ce2

forgejo-actions commented

2026-10-24 07:42:55 +00:00

Review 01M5PDGEM0X15E82HRR1DE664H — head 58029b8ce21c50383b24ba80ffa17f1613b51597

Review — j4k-oss/askpplx @ `67d084827e`

Scope: diff against base tree 7b1cb72307af
Review finished: 10 succeeded, 0 failed
Triage: settled
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v5",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (2)

medium — The cleanup watcher can finish before worktree creation stops

claim: 01M5PDNZ9PAAZ3C3Y06BSYHWGD
anchor: .githooks/commit-msg (snippet)

git --git-dir="$git_common_dir" -c worktree.useRelativePaths=false -c core.hooksPath=/dev/null worktree add --quiet --detach "$snapshot_dir" "$snapshot_commit" 6>&-

lens: general-bug · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M5PDPGGF5CBK0B97VB45X6NY · valid: The grounded anchor shows git worktree add running in the foreground with '6>&-', so it does not hold the watcher's pipe open. Claim 01M5PDMVF4 grounds the watcher, which waits for EOF on fd 6, and describes run_step as the path that sends PIDs. Worktree add sends no PID. If only the hook process is SIGKILLed during worktree add, its last write end closes and the watcher gets EOF right away with an empty process group. It then runs remove_snapshot_files while the orphaned git child keeps going: a parent's SIGKILL does not end the child. Git can then recreate the directory contents and the worktree registration after cleanup. The trace is internally consistent, and nothing in the batch suggests the watcher waits for or kills that child. The reported reproduction gives specific conditions (Bash 5.2, Git 2.47, a PATH shim delaying only worktree add before it execs the real git, and SIGKILL to the hook PID only). Its result, a leftover pre-commit-snapshot.* directory and a worktree-list entry, follows from the mechanism. The shim widens a window that real checkouts also have. The defect needs a kill aimed at the hook PID alone, such as a supervisor timeout, rather than a process-group kill. That narrows its reach, but SIGKILL is exactly the case the watcher exists to handle. Medium severity is defensible, though low-to-medium would also be reasonable given the narrow trigger.
disposition: none

Killing the hook during worktree creation can leave an orphaned snapshot directory and registered worktree despite the new SIGKILL cleanup. The foreground Git child can keep creating the worktree after the watcher has removed it and exited.

This invocation closes fd 6 but never sends its PID to the watcher. Consequently, when the hook dies, watch_snapshot receives EOF with an empty process_group and calls remove_snapshot_files without stopping or waiting for the still-running worktree add. The process-group protocol in run_step protects the checks, but does not cover this setup command. Track worktree creation in a separate process group and stop it before cleanup, while continuing to close fd 6 in children that might spawn daemons.

I reproduced this with the subject hook under Bash 5.2 and Git 2.47 in a temporary repository, using a pnpm stub. Running node /app/probe_add.cjs put a Git shim on PATH that delayed only worktree add for one second before executing /usr/bin/git with the original arguments. After the shim signaled that add had started, the harness sent SIGKILL only to the hook. Two seconds later, the temporary directory still contained pre-commit-snapshot.* and git worktree list --porcelain listed its detached worktree. The delay forces the scheduling window; it does not replace Git's worktree operation. Normal completion and SIGKILL during run_step both cleaned up in the companion probe.

The demonstrated failure depends on worktree add continuing after its parent dies. Ensuring that this child is terminated and reaped before the watcher removes the snapshot would close the race; a cancellation probe at the start of worktree add should then leave neither the directory nor its registration.

low — The snapshot watcher's SIGKILL cleanup only works if every later command closes fd 6 by hand and every cleanup path is assigned before the watcher forks

claim: 01M5PDMVF4V7DTA4JCQ3RWQ7KJ
anchor: .githooks/commit-msg (snippet)

exec 6> >(set -m; watch_snapshot <&0 > /dev/null &)

lens: aposd-plain · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M5PDPGGF5CBK0B97VB45X6NY · valid: The grounded exec line shows that the watcher detects hook death by EOF on fd 6, a pipe every child inherits. Claim 01M5PDNZ9P independently grounds the git worktree add line ending in '6>&-', which confirms that callers close the fd by hand. The body quotes both governing rules verbatim from comments: the fd-6 close rule above the exec, and the 'assign every path used here before starting it' rule above remove_snapshot_files. It names the places that follow them by hand: run_step's background job, the worktree add line, and the three mktemp calls placed between the trap and the exec. Two watcher-internal decisions (EOF-based liveness and a fork-time copy of the cleanup paths) are therefore constraints every later call site must honor without enforcement, and a slip fails silently on the SIGKILL path only. This matches the Information Leakage and Nonobvious Code red flags and the skill's ranking of hidden required knowledge as the costliest symptom. Claim 01M5PDNZ9P shows how fragile this is: the worktree add call honors the fd rule, yet the watcher does not stop or wait for it before cleanup. The claim correctly scopes itself as a maintenance hazard with no present malfunction, and low severity fits. One remedy caveat: sending each path over the pipe after allocation leaves a small window where a kill between mktemp and the send leaks the path. Allocating and announcing in one helper, or keeping the current fork-time order for paths allocated before the watcher starts, avoids that. The caveat narrows the remedy, not the defect.
disposition: none

The watcher's design leaks two of its internal decisions into the rest of the hook. Nothing enforces either one, and breaking one fails silently. Because the watcher waits for EOF on the fd-6 pipe, every command that can leave a process behind has to add 6>&- itself. Because the watcher keeps the variable values it had when it forked, every path that remove_snapshot_files reads has to be assigned before the exec 6> line. Suppose a future edit adds a step that starts a daemon without going through run_step (for example, another git call that runs a smudge filter or fsmonitor). If that daemon keeps fd 6 open, a SIGKILLed hook leaves the snapshot worktree, the log, and the install-config file on disk until the daemon exits. Or suppose an edit allocates a new temp path after the exec line, the way install_git_config used to come after worktree add before this diff moved it up. The watcher then sees that path as empty and treats it as "never allocated", so the file is never removed. Normal exits and catchable signals still go through remove_snapshot, so neither mistake shows up in ordinary use. Only the SIGKILL path that the watcher exists for breaks.

Right now both rules are written only as comments. Above the exec line: "close it (6>&-) on any command that can leave a process behind, such as a check or a smudge filter's daemon". Above remove_snapshot_files, about 50 lines earlier: "The watcher runs this on the values it inherited when it forked, so assign every path used here before starting it." The hook follows each rule by hand at each place it applies: 6>&- on the background job in run_step and on the git ... worktree add line, and the three mktemp calls moved between trap remove_snapshot EXIT and the exec. The wire protocol has the same split. Its message format is defined in the comment above tell_watcher and parsed in watch_snapshot, more than 100 lines later.

Correction: move these rules inside the watcher module so callers no longer carry them. Send each allocated path to the watcher over the pipe it already reads, the same way PIDs are sent now (e.g. a path <p> message, with watch_snapshot keeping a list). The fork-time snapshot then stops mattering, and allocation order is free again. Then send every child-spawning command through one helper that applies 6>&-; run_step already does this for checks. Then the fd rule has a single home instead of one per call site. Another design worth comparing: the watcher polls the hook's PID instead of waiting for pipe EOF. That removes the fd-inheritance rule completely, at the cost of a PID-reuse window.

What I examined: the full .githooks/commit-msg in the subject tree and its diff. I checked every command that runs after the exec 6> line: git worktree add (has 6>&-), the git rev-parse / git config / sed calls (synchronous, so they leave nothing behind), and run_step (has 6>&-). No current command breaks either rule, so this is a maintenance hazard, not a present malfunction. I did not run the hook or reproduce a SIGKILL. A SIGKILL test with a step that starts a daemon holding fd 6 would confirm the delayed cleanup.

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (0)

Coverage

Coverage pass: 01M5PDGET597H4KDBW6R4FGB2X
Accounting: complete (all slots terminal; see slot results for failures)
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	claims-emitted	1	no
writing-quality	whole	default	no-claims	1	no
test-trimming	whole	default	no-claims	1	no
restated-sets	whole	default	no-claims	1	no
project-docs	whole	default	no-claims	1	no
unix-cli	whole	default	no-claims	1	no
use-effect	whole	default	no-claims	1	no
module-design	whole	default	no-claims	1	no
aposd-plain	whole	default	claims-emitted	1	no
react	whole	default	no-claims	1	no

**Review** `01M5PDGEM0X15E82HRR1DE664H` — head `58029b8ce21c50383b24ba80ffa17f1613b51597` # Review — j4k-oss/askpplx @ 67d084827e36 Scope: diff against base tree `7b1cb72307af` Review finished: 10 succeeded, 0 failed Triage: settled Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v5", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (2) ### medium — The cleanup watcher can finish before worktree creation stops - claim: `01M5PDNZ9PAAZ3C3Y06BSYHWGD` - anchor: `.githooks/commit-msg` (snippet) ``` git --git-dir="$git_common_dir" -c worktree.useRelativePaths=false -c core.hooksPath=/dev/null worktree add --quiet --detach "$snapshot_dir" "$snapshot_commit" 6>&- ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M5PDPGGF5CBK0B97VB45X6NY` · valid: The grounded anchor shows git worktree add running in the foreground with '6>&-', so it does not hold the watcher's pipe open. Claim 01M5PDMVF4 grounds the watcher, which waits for EOF on fd 6, and describes run_step as the path that sends PIDs. Worktree add sends no PID. If only the hook process is SIGKILLed during worktree add, its last write end closes and the watcher gets EOF right away with an empty process group. It then runs remove_snapshot_files while the orphaned git child keeps going: a parent's SIGKILL does not end the child. Git can then recreate the directory contents and the worktree registration after cleanup. The trace is internally consistent, and nothing in the batch suggests the watcher waits for or kills that child. The reported reproduction gives specific conditions (Bash 5.2, Git 2.47, a PATH shim delaying only worktree add before it execs the real git, and SIGKILL to the hook PID only). Its result, a leftover pre-commit-snapshot.* directory and a worktree-list entry, follows from the mechanism. The shim widens a window that real checkouts also have. The defect needs a kill aimed at the hook PID alone, such as a supervisor timeout, rather than a process-group kill. That narrows its reach, but SIGKILL is exactly the case the watcher exists to handle. Medium severity is defensible, though low-to-medium would also be reasonable given the narrow trigger. - disposition: none > Killing the hook during worktree creation can leave an orphaned snapshot directory and registered worktree despite the new SIGKILL cleanup. The foreground Git child can keep creating the worktree after the watcher has removed it and exited. > > This invocation closes fd 6 but never sends its PID to the watcher. Consequently, when the hook dies, watch_snapshot receives EOF with an empty process_group and calls remove_snapshot_files without stopping or waiting for the still-running worktree add. The process-group protocol in run_step protects the checks, but does not cover this setup command. Track worktree creation in a separate process group and stop it before cleanup, while continuing to close fd 6 in children that might spawn daemons. > > I reproduced this with the subject hook under Bash 5.2 and Git 2.47 in a temporary repository, using a pnpm stub. Running `node /app/probe_add.cjs` put a Git shim on PATH that delayed only worktree add for one second before executing /usr/bin/git with the original arguments. After the shim signaled that add had started, the harness sent SIGKILL only to the hook. Two seconds later, the temporary directory still contained pre-commit-snapshot.* and `git worktree list --porcelain` listed its detached worktree. The delay forces the scheduling window; it does not replace Git's worktree operation. Normal completion and SIGKILL during run_step both cleaned up in the companion probe. > > The demonstrated failure depends on worktree add continuing after its parent dies. Ensuring that this child is terminated and reaped before the watcher removes the snapshot would close the race; a cancellation probe at the start of worktree add should then leave neither the directory nor its registration. ### low — The snapshot watcher's SIGKILL cleanup only works if every later command closes fd 6 by hand and every cleanup path is assigned before the watcher forks - claim: `01M5PDMVF4V7DTA4JCQ3RWQ7KJ` - anchor: `.githooks/commit-msg` (snippet) ``` exec 6> >(set -m; watch_snapshot <&0 > /dev/null &) ``` - lens: aposd-plain · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M5PDPGGF5CBK0B97VB45X6NY` · valid: The grounded exec line shows that the watcher detects hook death by EOF on fd 6, a pipe every child inherits. Claim 01M5PDNZ9P independently grounds the git worktree add line ending in '6>&-', which confirms that callers close the fd by hand. The body quotes both governing rules verbatim from comments: the fd-6 close rule above the exec, and the 'assign every path used here before starting it' rule above remove_snapshot_files. It names the places that follow them by hand: run_step's background job, the worktree add line, and the three mktemp calls placed between the trap and the exec. Two watcher-internal decisions (EOF-based liveness and a fork-time copy of the cleanup paths) are therefore constraints every later call site must honor without enforcement, and a slip fails silently on the SIGKILL path only. This matches the Information Leakage and Nonobvious Code red flags and the skill's ranking of hidden required knowledge as the costliest symptom. Claim 01M5PDNZ9P shows how fragile this is: the worktree add call honors the fd rule, yet the watcher does not stop or wait for it before cleanup. The claim correctly scopes itself as a maintenance hazard with no present malfunction, and low severity fits. One remedy caveat: sending each path over the pipe after allocation leaves a small window where a kill between mktemp and the send leaks the path. Allocating and announcing in one helper, or keeping the current fork-time order for paths allocated before the watcher starts, avoids that. The caveat narrows the remedy, not the defect. - disposition: none > The watcher's design leaks two of its internal decisions into the rest of the hook. Nothing enforces either one, and breaking one fails silently. Because the watcher waits for EOF on the fd-6 pipe, every command that can leave a process behind has to add `6>&-` itself. Because the watcher keeps the variable values it had when it forked, every path that `remove_snapshot_files` reads has to be assigned before the `exec 6>` line. Suppose a future edit adds a step that starts a daemon without going through `run_step` (for example, another `git` call that runs a smudge filter or fsmonitor). If that daemon keeps fd 6 open, a SIGKILLed hook leaves the snapshot worktree, the log, and the install-config file on disk until the daemon exits. Or suppose an edit allocates a new temp path after the `exec` line, the way `install_git_config` used to come after `worktree add` before this diff moved it up. The watcher then sees that path as empty and treats it as "never allocated", so the file is never removed. Normal exits and catchable signals still go through `remove_snapshot`, so neither mistake shows up in ordinary use. Only the SIGKILL path that the watcher exists for breaks. > > Right now both rules are written only as comments. Above the exec line: "close it (`6>&-`) on any command that can leave a process behind, such as a check or a smudge filter's daemon". Above `remove_snapshot_files`, about 50 lines earlier: "The watcher runs this on the values it inherited when it forked, so assign every path used here before starting it." The hook follows each rule by hand at each place it applies: `6>&-` on the background job in `run_step` and on the `git ... worktree add` line, and the three `mktemp` calls moved between `trap remove_snapshot EXIT` and the `exec`. The wire protocol has the same split. Its message format is defined in the comment above `tell_watcher` and parsed in `watch_snapshot`, more than 100 lines later. > > Correction: move these rules inside the watcher module so callers no longer carry them. Send each allocated path to the watcher over the pipe it already reads, the same way PIDs are sent now (e.g. a <code>path <p></code> message, with `watch_snapshot` keeping a list). The fork-time snapshot then stops mattering, and allocation order is free again. Then send every child-spawning command through one helper that applies `6>&-`; `run_step` already does this for checks. Then the fd rule has a single home instead of one per call site. Another design worth comparing: the watcher polls the hook's PID instead of waiting for pipe EOF. That removes the fd-inheritance rule completely, at the cost of a PID-reuse window. > > What I examined: the full `.githooks/commit-msg` in the subject tree and its diff. I checked every command that runs after the `exec 6>` line: `git worktree add` (has `6>&-`), the `git rev-parse` / `git config` / `sed` calls (synchronous, so they leave nothing behind), and `run_step` (has `6>&-`). No current command breaks either rule, so this is a maintenance hazard, not a present malfunction. I did not run the hook or reproduce a SIGKILL. A SIGKILL test with a step that starts a daemon holding fd 6 would confirm the delayed cleanup. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M5PDGET597H4KDBW6R4FGB2X Accounting: complete (all slots terminal; see slot results for failures) Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | claims-emitted | 1 | no | | writing-quality | whole | default | no-claims | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | no-claims | 1 | no | | project-docs | whole | default | no-claims | 1 | no | | unix-cli | whole | default | no-claims | 1 | no | | use-effect | whole | default | no-claims | 1 | no | | module-design | whole | default | no-claims | 1 | no | | aposd-plain | whole | default | claims-emitted | 1 | no | | react | whole | default | no-claims | 1 | no |

🚀 1

forgejo-actions reviewed

2026-10-24 07:42:56 +00:00

.githooks/commit-msg

					
				@ -146,0 +202,4 @@

				# Job control gives the watcher its own process group; its standard input is the pipe. The watcher

				# sees EOF only once every holder of fd 6 has exited, so close it (`6>&-`) on any command that can

				# leave a process behind, such as a check or a smudge filter's daemon.

				exec 6> >(set -m; watch_snapshot <&0 > /dev/null &)

low — The snapshot watcher's SIGKILL cleanup only works if every later command closes fd 6 by hand and every cleanup path is assigned before the watcher forks

The watcher's design leaks two of its internal decisions into the rest of the hook. Nothing enforces either one, and breaking one fails silently. Because the watcher waits for EOF on the fd-6 pipe, every command that can leave a process behind has to add 6>&- itself. Because the watcher keeps the variable values it had when it forked, every path that remove_snapshot_files reads has to be assigned before the exec 6> line. Suppose a future edit adds a step that starts a daemon without going through run_step (for example, another git call that runs a smudge filter or fsmonitor). If that daemon keeps fd 6 open, a SIGKILLed hook leaves the snapshot worktree, the log, and the install-config file on disk until the daemon exits. Or suppose an edit allocates a new temp path after the exec line, the way install_git_config used to come after worktree add before this diff moved it up. The watcher then sees that path as empty and treats it as "never allocated", so the file is never removed. Normal exits and catchable signals still go through remove_snapshot, so neither mistake shows up in ordinary use. Only the SIGKILL path that the watcher exists for breaks.

Right now both rules are written only as comments. Above the exec line: "close it (6>&-) on any command that can leave a process behind, such as a check or a smudge filter's daemon". Above remove_snapshot_files, about 50 lines earlier: "The watcher runs this on the values it inherited when it forked, so assign every path used here before starting it." The hook follows each rule by hand at each place it applies: 6>&- on the background job in run_step and on the git ... worktree add line, and the three mktemp calls moved between trap remove_snapshot EXIT and the exec. The wire protocol has the same split. Its message format is defined in the comment above tell_watcher and parsed in watch_snapshot, more than 100 lines later.

Correction: move these rules inside the watcher module so callers no longer carry them. Send each allocated path to the watcher over the pipe it already reads, the same way PIDs are sent now (e.g. a path <p> message, with watch_snapshot keeping a list). The fork-time snapshot then stops mattering, and allocation order is free again. Then send every child-spawning command through one helper that applies 6>&-; run_step already does this for checks. Then the fd rule has a single home instead of one per call site. Another design worth comparing: the watcher polls the hook's PID instead of waiting for pipe EOF. That removes the fd-inheritance rule completely, at the cost of a PID-reuse window.

What I examined: the full .githooks/commit-msg in the subject tree and its diff. I checked every command that runs after the exec 6> line: git worktree add (has 6>&-), the git rev-parse / git config / sed calls (synchronous, so they leave nothing behind), and run_step (has 6>&-). No current command breaks either rule, so this is a maintenance hazard, not a present malfunction. I did not run the hook or reproduce a SIGKILL. A SIGKILL test with a step that starts a daemon holding fd 6 would confirm the delayed cleanup.

lens aposd-plain · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M5PDMVF4V7DTA4JCQ3RWQ7KJ of review 01M5PDGEM0X15E82HRR1DE664H

**low** — The snapshot watcher's SIGKILL cleanup only works if every later command closes fd 6 by hand and every cleanup path is assigned before the watcher forks > The watcher's design leaks two of its internal decisions into the rest of the hook. Nothing enforces either one, and breaking one fails silently. Because the watcher waits for EOF on the fd-6 pipe, every command that can leave a process behind has to add `6>&-` itself. Because the watcher keeps the variable values it had when it forked, every path that `remove_snapshot_files` reads has to be assigned before the `exec 6>` line. Suppose a future edit adds a step that starts a daemon without going through `run_step` (for example, another `git` call that runs a smudge filter or fsmonitor). If that daemon keeps fd 6 open, a SIGKILLed hook leaves the snapshot worktree, the log, and the install-config file on disk until the daemon exits. Or suppose an edit allocates a new temp path after the `exec` line, the way `install_git_config` used to come after `worktree add` before this diff moved it up. The watcher then sees that path as empty and treats it as "never allocated", so the file is never removed. Normal exits and catchable signals still go through `remove_snapshot`, so neither mistake shows up in ordinary use. Only the SIGKILL path that the watcher exists for breaks. > > Right now both rules are written only as comments. Above the exec line: "close it (`6>&-`) on any command that can leave a process behind, such as a check or a smudge filter's daemon". Above `remove_snapshot_files`, about 50 lines earlier: "The watcher runs this on the values it inherited when it forked, so assign every path used here before starting it." The hook follows each rule by hand at each place it applies: `6>&-` on the background job in `run_step` and on the `git ... worktree add` line, and the three `mktemp` calls moved between `trap remove_snapshot EXIT` and the `exec`. The wire protocol has the same split. Its message format is defined in the comment above `tell_watcher` and parsed in `watch_snapshot`, more than 100 lines later. > > Correction: move these rules inside the watcher module so callers no longer carry them. Send each allocated path to the watcher over the pipe it already reads, the same way PIDs are sent now (e.g. a <code>path <p></code> message, with `watch_snapshot` keeping a list). The fork-time snapshot then stops mattering, and allocation order is free again. Then send every child-spawning command through one helper that applies `6>&-`; `run_step` already does this for checks. Then the fd rule has a single home instead of one per call site. Another design worth comparing: the watcher polls the hook's PID instead of waiting for pipe EOF. That removes the fd-inheritance rule completely, at the cost of a PID-reuse window. > > What I examined: the full `.githooks/commit-msg` in the subject tree and its diff. I checked every command that runs after the `exec 6>` line: `git worktree add` (has `6>&-`), the `git rev-parse` / `git config` / `sed` calls (synchronous, so they leave nothing behind), and `run_step` (has `6>&-`). No current command breaks either rule, so this is a maintenance hazard, not a present malfunction. I did not run the hook or reproduce a SIGKILL. A SIGKILL test with a step that starts a daemon holding fd 6 would confirm the delayed cleanup. lens `aposd-plain` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M5PDMVF4V7DTA4JCQ3RWQ7KJ` of review `01M5PDGEM0X15E82HRR1DE664H`

👎 1

The fork-time path allocation and fd-6 closure rules are real maintenance constraints. Current paths are allocated before the watcher forks; the finding itself confirms current call sites obey the rules. The concrete checkout race is separately owned by Align #826. For these controlled repositories we decline dynamic path messages, PID polling, and a general spawning abstraction: they add machinery without another demonstrated current failure. This is an accepted maintenance cost, not a claim that the coupling is absent or fixed.

The fork-time path allocation and fd-6 closure rules are real maintenance constraints. Current paths are allocated before the watcher forks; the finding itself confirms current call sites obey the rules. The concrete checkout race is separately owned by Align #826. For these controlled repositories we decline dynamic path messages, PID polling, and a general spawning abstraction: they add machinery without another demonstrated current failure. This is an accepted maintenance cost, not a claim that the coupling is absent or fixed.

jercik marked this conversation as resolved

.githooks/commit-msg

					
				@ -146,2 +205,3 @@

				exec 6> >(set -m; watch_snapshot <&0 > /dev/null &)

				# Relative worktree paths would upgrade the repository format; post-checkout hooks must not run here.

				git --git-dir="$git_common_dir" -c worktree.useRelativePaths=false -c core.hooksPath=/dev/null worktree add --quiet --detach "$snapshot_dir" "$snapshot_commit"

				git --git-dir="$git_common_dir" -c worktree.useRelativePaths=false -c core.hooksPath=/dev/null worktree add --quiet --detach "$snapshot_dir" "$snapshot_commit" 6>&-

medium — The cleanup watcher can finish before worktree creation stops

Killing the hook during worktree creation can leave an orphaned snapshot directory and registered worktree despite the new SIGKILL cleanup. The foreground Git child can keep creating the worktree after the watcher has removed it and exited.

This invocation closes fd 6 but never sends its PID to the watcher. Consequently, when the hook dies, watch_snapshot receives EOF with an empty process_group and calls remove_snapshot_files without stopping or waiting for the still-running worktree add. The process-group protocol in run_step protects the checks, but does not cover this setup command. Track worktree creation in a separate process group and stop it before cleanup, while continuing to close fd 6 in children that might spawn daemons.

I reproduced this with the subject hook under Bash 5.2 and Git 2.47 in a temporary repository, using a pnpm stub. Running node /app/probe_add.cjs put a Git shim on PATH that delayed only worktree add for one second before executing /usr/bin/git with the original arguments. After the shim signaled that add had started, the harness sent SIGKILL only to the hook. Two seconds later, the temporary directory still contained pre-commit-snapshot.* and git worktree list --porcelain listed its detached worktree. The delay forces the scheduling window; it does not replace Git's worktree operation. Normal completion and SIGKILL during run_step both cleaned up in the companion probe.

The demonstrated failure depends on worktree add continuing after its parent dies. Ensuring that this child is terminated and reaped before the watcher removes the snapshot would close the race; a cancellation probe at the start of worktree add should then leave neither the directory nor its registration.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M5PDNZ9PAAZ3C3Y06BSYHWGD of review 01M5PDGEM0X15E82HRR1DE664H

**medium** — The cleanup watcher can finish before worktree creation stops > Killing the hook during worktree creation can leave an orphaned snapshot directory and registered worktree despite the new SIGKILL cleanup. The foreground Git child can keep creating the worktree after the watcher has removed it and exited. > > This invocation closes fd 6 but never sends its PID to the watcher. Consequently, when the hook dies, watch_snapshot receives EOF with an empty process_group and calls remove_snapshot_files without stopping or waiting for the still-running worktree add. The process-group protocol in run_step protects the checks, but does not cover this setup command. Track worktree creation in a separate process group and stop it before cleanup, while continuing to close fd 6 in children that might spawn daemons. > > I reproduced this with the subject hook under Bash 5.2 and Git 2.47 in a temporary repository, using a pnpm stub. Running `node /app/probe_add.cjs` put a Git shim on PATH that delayed only worktree add for one second before executing /usr/bin/git with the original arguments. After the shim signaled that add had started, the harness sent SIGKILL only to the hook. Two seconds later, the temporary directory still contained pre-commit-snapshot.* and `git worktree list --porcelain` listed its detached worktree. The delay forces the scheduling window; it does not replace Git's worktree operation. Normal completion and SIGKILL during run_step both cleaned up in the companion probe. > > The demonstrated failure depends on worktree add continuing after its parent dies. Ensuring that this child is terminated and reaped before the watcher removes the snapshot would close the race; a cancellation probe at the start of worktree add should then leave neither the directory nor its registration. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M5PDNZ9PAAZ3C3Y06BSYHWGD` of review `01M5PDGEM0X15E82HRR1DE664H`

🚀 1

jercik marked this conversation as resolved

jercik commented

2026-10-24 08:31:55 +00:00

Replying to review comment #183798

Confirmed with real Git on macOS, without filters: killing only the hook during delayed snapshot checkout can leave a temporary snapshot and worktree registration. The current hook retains this documented limitation; this PR does not fix it. The producer correction and focused regression are owned by j4k/align#826, which routes checkout through the existing run_step helper. Consumers will receive the generated correction after that release.

> Replying to review comment #183798 Confirmed with real Git on macOS, without filters: killing only the hook during delayed snapshot checkout can leave a temporary snapshot and worktree registration. The current hook retains this documented limitation; this PR does not fix it. The producer correction and focused regression are owned by https://code.j4k.dev/j4k/align/pulls/826, which routes checkout through the existing run_step helper. Consumers will receive the generated correction after that release.

jercik merged commit 68c1ad4c99 into main

2026-10-24 08:33:27 +00:00

jercik deleted branch chore/align-local-forge

2026-10-24 08:33:27 +00:00

jercik referenced this pull request from a commit

2026-10-24 08:33:29 +00:00

chore: sync align local command templates (#6)

Sign in to join this conversation.

No reviewers

No labels

No milestone