fix(hooks): resolve verify chain cwd to task worktree/project (#628) - #642
Conversation
…harpe#628) enter-done hook ran `git status` in whatever directory the runtime process was launched from, not the task's project or worktree. Resolves cwd via worktree registry -> Task.working_directory -> BotRoot fallback before running the verify chain, with Test-Path guards against stale registry entries. Adds regression coverage in tests/Test-Hooks.ps1 for all five resolution scenarios (valid worktree, PSCustomObject-provenance variant, stale worktree registry entry, no run_id, and full fallback to BotRoot). Fixes andresharpe#628 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@elmaljevo this fix is scoped only to enter-done hook. As I understamd other status transition hooks will still have cwd where server started, right? |
There was a problem hiding this comment.
Pull request overview
Fixes the enter-done verify hook chain running in the runtime’s launch directory by explicitly resolving and switching to a task-appropriate working directory before executing verify scripts, addressing issue #628.
Changes:
- Resolve verify-chain cwd in
enter-doneusing: task worktree (when available) →Task.working_directory→BotRoot, thenPush-Locationfor the duration of the chain. - Add regression coverage that runs
enter-doneout-of-process from arbitrary launch directories and verifies cwd selection across multiple scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/runtime/Plugins/Hooks/Transitions/enter-done/script.ps1 | Resolves and applies a deterministic working directory for verify hooks; adds optional worktree lookup/import support. |
| tests/Test-Hooks.ps1 | Adds out-of-process regression tests to ensure verify hooks run in the resolved cwd instead of the runtime launch cwd. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mity1982 You're right, and it's worth being explicit about the scope. The other four shipped transition hooks (enter-in-progress, enter-failed, enter-skipped, enter-cancelled) don't actually read or write anything relative to cwd today: enter-in-progress only does a worktree registry lookup + session registration, enter-failed writes diagnostics via absolute Join-Path paths, and enter-skipped/enter-cancelled are no-ops. So none of them are exposed to this bug in practice right now. But the underlying mechanism is generic — Invoke-SingleTransitionHook in Dispatch.psm1 never sets a cwd for any hook's runspace, it's whatever the runtime process happened to be launched from. enter-done is the only one affected today only because it's the only one that shells out to a cwd-sensitive child process (the verify chain's git status). Any future or project-custom hook that shells out similarly would hit the same issue. I scoped this fix to enter-done to keep the PR focused on the reported bug, but happy to push the resolution up into Dispatch.psm1 instead (or in addition), so every hook gets a correctly-resolved cwd threaded through RunContext regardless of what it does. Let me know which you'd prefer and I'll follow up. |
- Don't hard-fail enter-done over an unresolved frameworkRoot when the content resolver (Get-DotbotHookChain) is already available and the only missing piece is the optional worktree lookup — that path is best-effort and already falls through to working_directory/BotRoot. - Validate the BotRoot fallback with Test-Path before Push-Location, and return a targeted error when no candidate directory is valid, instead of letting Push-Location throw a generic exception. Addresses Copilot review comments on PR for andresharpe#628.
…t-of-process runner Dot-sourcing script.ps1 gave Export-ModuleMember no real module scope, throwing a non-terminating error that had to be swallowed with 2>$null and didn't mirror how hooks are actually loaded. Build a dynamic module from the file content instead, matching Dispatch.psm1's own Invoke-SingleTransitionHook loading pattern exactly (verified: Import-Module on a bare .ps1 hits the same error — only New-Module -ScriptBlock avoids it). Addresses a Copilot review comment on PR for andresharpe#628.
…resharpe#628 tests) MarkerFile, RunId, WorkingDirectory, TaskId, BotRootDir, repoRoot, and enterDoneScript were interpolated directly into single-quoted PowerShell string literals when building the stub verify scripts and the out-of-process runner. Any value containing an apostrophe (a temp/worktree path can legitimately have one) would break the generated script with a ParserError unrelated to cwd resolution - reproduced and confirmed fixed manually before committing. Adds ConvertTo-PSSingleQuoteLiteral and applies it to every value embedded this way. Addresses Copilot review comments on PR for andresharpe#628.
…re .psm1 Importing Dotbot.Worktree.psm1 directly skipped the manifest's ScriptsToProcess (Dotbot.Core, Dotbot.TaskFile) and NestedModules, unlike the established pattern elsewhere (Invoke-DotbotProcess.ps1, Dotbot.TaskInput.psm1). Dotbot.Worktree's functions call into Dotbot.Core (e.g. Write-BotLog on the Read-WorktreeMap error path), so skipping that risked a confusing secondary failure instead of the intended behavior. Verified the .psd1 exists and all worktree-dependent test scenarios (1, 1b, 2) still pass with the manifest import. Addresses a Copilot review comment on PR for andresharpe#628.
Positional binding to Push-Location's -Path performs wildcard expansion; -LiteralPath is correct for filesystem paths that shouldn't be glob- interpreted. Matches enter-done/script.ps1's own Push-Location call. Fixed both occurrences in this file (the andresharpe#628 out-of-process runner's launch-cwd switch, and the pre-existing end-to-end test's bot fixture). Addresses a Copilot review comment on PR for andresharpe#628.
|
Accepted follow-up scope is now tracked in #669: resolve standalone/stale-worktree fallback to the project root (not .bot) and make transition-hook execution context generic in the dispatcher with schema-valid integration coverage. This keeps #642 focused on the immediate verify-chain failure while preserving concrete acceptance criteria for the broader fix. |
carlospedreira
left a comment
carlospedreira
left a comment
There was a problem hiding this comment.
Approved for the focused #628 fix. The broader project-root fallback and generic dispatcher execution-context work is explicitly tracked in #669. I verified a clean prospective merge onto current main and ran Test-Hooks.ps1 (71/71), Test-ProcessDispatch.ps1 (44/44), and Test-Runtime.ps1 (117 passed, 1 POSIX-only skip). No new security or data-loss concern found.
Linked issue
Closes #628
Summary of changes
enter-done's verify hook chain (e.g.01-git-clean.ps1) rangit statusrelative to whatever directory the runtime process was launched from, instead of the task's actual project/worktree. This meant the check's outcome depended on the launch folder, not the task - a dirty launch folder wrongly failed the task, and a non-repo launch folder wrongly passed it without checking anything.Resolves the verify chain's working directory before running it, in this order: task's worktree (via the worktree registry, for WorkflowRun tasks) ->
Task.working_directory->BotRootas a last resort. Each candidate is validated withTest-Pathbefore use, so a stale/deleted worktree registry entry falls through to the next option instead of erroring.Screenshots / recordings
N/A - backend/runtime fix, no UI changes.
Testing notes
Added 5 new regression scenarios to
tests/Test-Hooks.ps1(20 assertions total), each driving the realenter-done/script.ps1as a fresh, unimportedpwshchild process launched from an arbitrary directory - reproducing the actual bug conditions (wrong launch cwd + fresh runspace with no preloaded modules):