test(worktree-helper): strict registration check, positive no-cleanup check, and a real askpass readiness deadline

[Chris0Jeky]

Summary

Three test-only fixes in scripts/git/Test-New-CodexIssueWorktree.ps1, the worktree helper regression suite. No product script changed; scripts/git/New-CodexIssueWorktree.ps1 and scripts/git/Initialize-CodexIssueWorktree.ps1 are untouched, so no test-only marker was needed for item 2.

1. Late-collision registration check made strict. Wait-ForScheduledWorktreeRemoval now takes the repository whose registration list it polls (-RegistrationRepositoryPath) and waits, on the one existing 90 s deadline with the same 100 ms poll, until both the worktree directory is gone and the normalized registration is gone. It returns Removed, Unregistered, Registrations and ElapsedSeconds. Both call sites in the initializer-validation case (the late branch collision and the separate-Git-dir sibling) now assert Removed and Unregistered, each message carrying the elapsed seconds and the final registration list.
2. Refused-cleanup settle replaced with a positive check. The Start-Sleep -Milliseconds 500 before the ignored-content assertions is gone. In its place, a new Assert-NormalizedNotContains helper asserts the initializer's captured output does not contain removal of the unused helper-created worktree was scheduled, so a regression that wrongly started the detached cleanup host fails immediately instead of passing because the host had not got round to deleting the canary yet. The same assertion is applied to the hidden-index (assume-unchanged / skip-worktree) refusal loop, which shares the shape. The existence and registration assertions are kept unchanged.
3. Askpass probe-server readiness deadline raised. The fixed 10 s deadline becomes 90 s, the suite's order for host-start waits, and the failure message now reports the elapsed seconds and whether the probe host had exited. That loop's existing 50 ms poll interval and its early exit on $serverProcess.HasExited are left as they are.

Root cause

Item 1 was vacuous, not merely weak. $initializerWorktree is a backslash-separated Windows path built with Join-Path, while git worktree list --porcelain prints registration paths with forward slashes, so $registrationsAfterCollision.Contains($initializerWorktree) was false whatever the registration state. Proved with a temporary Write-Host of both strings, from a single -Case initializer-validation run:

TEMPPROBE-WORKTREE-PATH: <C:\Users\jekyt\AppData\Local\Temp\taskdeck-worktree-helper-82aaae776695479ba4641d8568cd6a6a\fixture's path with spaces\caller\.worktrees\codex-446-initializer-validation>
TEMPPROBE-REGISTRATIONS: <worktree C:/Users/jekyt/AppData/Local/Temp/taskdeck-worktree-helper-82aaae776695479ba4641d8568cd6a6a/fixture's path with spaces/caller
HEAD 9fea4bc77df13ad76d6943e11dec839ccd5bdc57
branch refs/heads/main>
TEMPPROBE-CONTAINS: <False>

The separate-Git-dir sibling already normalized with .Replace('\', '/'); this site did not. The print was removed before committing.

Making the check strict on its own would have reintroduced the timing red that #2664 fixed, because git worktree remove deletes the working directory before the admin entry under .git/worktrees, and the scenario asserts on the directory first. Waiting for both facts on the same deadline avoids that: a leftover admin entry now fails as a stale registration rather than as a race.

For item 2, a 500 ms sleep cannot observe the failure it guards. The detached cleanup host would delete the canary one host start plus seven Git invocations later, so both Test-Path assertions would still pass. The sleep was dropped, not lengthened; no reason to keep it was found, since the surviving assertions no longer depend on elapsed time.

Verification

All commands run from the worktree root C:/Users/jekyt/source/Taskdeck-Beta/.worktrees/codex-2670-helper-suite-waits in a native Windows PowerShell 5.1 host, not via Git Bash.

Parser check:

[System.Management.Automation.Language.Parser]::ParseFile(<suite path>, [ref]$tokens, [ref]$errors)
PARSE-ERRORS: 0

Full suite:

powershell -NoLogo -NoProfile -NonInteractive -File scripts/git/Test-New-CodexIssueWorktree.ps1
PASS: 29/29 selected worktree helper regression checks
EXIT=0 ELAPSED=254.8

Targeted runs of the two changed cases, before the full run: -Case initializer-validation gave PASS: 1/1, -Case askpass-suppression gave PASS: 1/1, both exit 0.

Red proof, item 1. With the fix in place, a temporary patch re-registered the exact worktree path with Git after the real scheduled removal completed and then deleted only the directory, leaving the stale admin entry the old check could never see, and re-ran the wait with -TimeoutSeconds 5. The old expression was printed alongside the new field:

TEMPRED-OLD-VACUOUS-CHECK-PASSES: <True>
TEMPRED-NEW-UNREGISTERED: <False>
Late branch collision must remove the worktree registration for 'C:\...\caller\.worktrees\codex-446-initializer-validation' (waited 5 s for the scheduled removal).
worktree C:/.../caller
HEAD 535c787508262fbbe41cf4ac5e1cd307d0882b4b
branch refs/heads/main
worktree C:/.../caller/.worktrees/codex-446-initializer-validation
HEAD 535c787508262fbbe41cf4ac5e1cd307d0882b4b
detached
prunable gitdir file points to non-existent location
EXIT=1

So the same stale registration that the old assertion passed through now fails the case. The temporary patch was reverted with git checkout -- and the working tree confirmed clean.

Red proof, item 2. The new Assert-NormalizedNotContains was temporarily pointed at a diagnostic the refusal path does emit (cleanup was refused because the helper-created worktree contains tracked, untracked, or ignored content) and -Case initializer-validation failed with EXIT=1 and the assertion's Forbidden substring: report, confirming the assertion is live and would fire if a scheduling diagnostic appeared on this path. This proves the assertion mechanism against real initializer output; it does not simulate an initializer that actually schedules the cleanup host.

Whitespace: git diff --check 140213288c07a1d96d48ecc9f810e23040b74ca9...HEAD is clean. Diff is one file, 59 insertions and 13 deletions.

Not verified

• No run on the hosted Windows runner. The 90 s askpass deadline and the 90 s registration wait were never exercised near their limits locally; both readiness paths completed well inside the old bounds on this box, so only the shorter path is covered by direct evidence.
• The item-3 change has no red proof. Raising a deadline that is not being hit locally cannot be made to fail on this machine; the new elapsed-seconds text in that message was not observed in a real failure.
• Item 2's not-contains assertion was not exercised against an initializer that genuinely schedules the cleanup host on a refused path, because that would require modifying scripts/git/Initialize-CodexIssueWorktree.ps1.
• No backend or frontend checks were run; nothing outside the one test script changed.

Risk notes

• The wait helper now runs git worktree list --porcelain on every 100 ms poll iteration instead of once after the loop. In the normal case the loop exits on its first or second iteration, and the observed full-suite time (254.8 s) is in line with the suite's usual cost. The helper uses Invoke-Git, so a genuine Git failure during the wait throws the suite's standard fixture-git error rather than being silently absorbed.
• -RegistrationRepositoryPath is mandatory, so any future call site must name the repository whose registration list it means. The two existing call sites pass $callerPath and $separateGitCaller respectively.
• The registration comparison is a normalized substring test, matching what the sibling assertions in the same case already do; those positive assertions passing in the same run is the evidence that both repositories' paths normalize to the form Git prints.
• The 90 s askpass deadline only extends the worst case of an already failing scenario; a successful bind still exits the loop as soon as the readiness marker appears.

Closes #2670. Refs #2664, PR #2669, #2425.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Review disposition (beta-platform-integrity lane, T3 gate)

Fresh-context adversarial review at 1381065: SHIP. No CRITICAL or HIGH. Confirmed by the reviewer: test-only change, no production helper touched; the extended wait polls directory-gone and registration-gone under the same 90 s deadline, and since the old collision-site check was vacuously true (backslash path against forward-slash porcelain), the normalized comparison can only detect more, never less; the not-scheduled assertion is non-vacuous by construction (the phrase is printed only after the initializer's scheduling step, and every refusal or could-not-schedule path exits first), so it cannot false-fail on a correct run.

Fixed (78719b5, comment only)

• LOW: the new readiness comment said HttpListener; the probe binds a TcpListener. Corrected; parser 0 errors; no executable line changed, so the 29/29 native run at 1381065 stands.

Declined, with reasoning

• MEDIUM (unverified hypothesis): polling git worktree list --porcelain every 100 ms while the detached cleanup host runs git worktree remove could, on Windows, hold the admin gitdir file open at the instant Git unlinks it and make the removal die, leaving a stale registration. Each list call holds the file for microseconds, Git's Windows unlink retries on sharing violations, and the failure would surface as Unregistered=False with the prunable entry printed in the message, so it is diagnosable rather than silent. Evidence is the hosted Docs Governance / Worktree Helper (Windows PowerShell) job at this head and the runs after it; if it ever fires, the fallback is to poll the admin directory with Test-Path and call git worktree list once at the end.
• LOW: the collision site has no positive registration control for the exact path string before waiting (the separate-Git-dir site has one). The downstream positive controls on other fixture paths cover the comparison in the same environment; not worth a second pre-check here.
• LOW: "no cleanup was scheduled" is asserted through the initializer's output, which models the realistic regression (taking the scheduling path) but not a Start-Process hoisted above the inventory. Accepted as the modelled regression; the comment says what it proves.
• LOW: the reviewer read docs/STATUS.md line 76 (the [CI][Windows] Worktree helper scenario 28 reddens main: the forced 5s timeout lands in the checkout phase, not the hung git add #2425 note that the suite aborts on this box at the invalid-branch case "because the pinned Git rejects the non-ASCII fixture name") against the worker's 29/29. Both runs happened on this box: launched from a Git Bash parent the suite stops at scenario 19 because the child host decodes the superscript argv character differently from Git; launched from a native PowerShell parent, same commit, same Git, it passes 29/29. The cause is the launching host's argv encoding, not the Git binary. That correction goes to the product-trust lane's STATUS lease with the merge packet.

Gate

Not measured locally: either 90 s deadline near its limit (both paths finish well inside the old bounds on this box), and item 3 has no red proof for the same reason. The hosted Windows helper job at the exact head is the proof; merge once exact-head ci-required is green and the three-minute floor after 78719b5 has passed.