fix(runtime): verify-hook-blocked done escalates to needs-input (#571)#579
Open
IBondarenko-iwg wants to merge 2 commits into
Open
fix(runtime): verify-hook-blocked done escalates to needs-input (#571)#579IBondarenko-iwg wants to merge 2 commits into
IBondarenko-iwg wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a runner edge case where a verify-hook abort during an in-progress → done transition was previously indistinguishable from “agent didn’t finish”, causing the retry loop to exhaust and incorrectly finalize the task as skipped(max-retries). It introduces an on-disk breadcrumb for “done-transition blocked by verify hook” and teaches the workflow runner to escalate that condition to needs-input without consuming retries.
Changes:
- Stamp
extensions.runner.done_transition_blockon tasks when adonetransition is aborted by a transition hook, and clear the marker on subsequent transitions. - Update the workflow runner retry loop to detect that breadcrumb and park the task to
needs-inputinstead of retrying to exhaustion / marking skipped. - Extend hook E2E tests to assert breadcrumb presence after abort and clearing after a successful done.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/Test-Hooks.ps1 |
Adds assertions for breadcrumb persistence after an aborted done-transition and clearing after a later successful done. |
src/runtime/Scripts/Invoke-WorkflowProcess.ps1 |
Reads the breadcrumb during completion checks and escalates to needs-input before error classification/retry exhaustion. |
src/runtime/Modules/Dotbot.Runtime/Private/HttpServer.psm1 |
Clears stale breadcrumb on forward transitions and stamps the breadcrumb on the abort-revert path for to=done. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #571
Closes #557
Summary of changes
When
enter-done's verify chain (e.g.00-privacy-scan.ps1) aborts the done-transition,HttpServerreverts the task toin-progress. The runner's retry loop couldn't tell that apart from "agent didn't finish", so it burned the retry budget and finalizedskipped(max-retries)— mislabeling a task whose external work (branches/PRs/Jira) had actually succeeded.HttpServer.psm1(Invoke-TaskStatusHandler) — on a done-transition abort, stamp a breadcrumbextensions.runner.done_transition_block = @{ hook; message; at }on the reverted task (underextensions.runnerbecause the schema rejects unknown top-level fields; status still reverts toin-progress, preserving the tested contract). Clear it on the next successful transition.Invoke-WorkflowProcess.ps1(retry loop) — read the breadcrumb off the already-loaded task and, when present, park toneeds-inputvia the existingSet-WorkflowTaskNeedsInput(mirrors the AuthError park) — without consuming the retry budget, neverskipped. Checked before the harness-error classification so this definitive on-disk signal isn't pre-empted by heuristic text matching.The operator then sees "verify hook blocked done: ", redacts the flagged content, and re-runs.
Deliberately not changed: the privacy scan still flags absolute
C:\Users\<name>paths in tracked deliverables — that is a genuine leak, and excluding.bot/workspace/product/(the report's suggested fix) would mask real leaks. The fix makes the block recoverable, not silent. (The report's citedoutcomes.mdlands in the target cloned repo, which the dotbot verify-hook scan never sees.)Testing notes
tests/Test-Hooks.ps1(E2E, extends the existing enter-done abort test): asserts the reverted task carriesextensions.runner.done_transition_blockwith the failing hook + message, and that a subsequent successful done clears it. Reviewed with/pwsh-review(diff-bug + idioms + history): ship, 0 blockers/majors — one minor (escalation ordering) applied by moving the breadcrumb check ahead of the error classifier.PARSE OK; PSScriptAnalyzer shows only pre-existing findings.To verify manually: run a task that commits an absolute local path into a tracked
.bot/workspace/product/**deliverable →task_set_status(done)is blocked by the privacy scan → task lands inneeds-input(notskipped), retry budget intact; redact the path and re-run → completes.Checklist