fix(runtime): verify-hook-blocked done escalates to needs-input (#571) - #579
Conversation
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.
|
@carlospedreira Ready for review. |
carlospedreira
left a comment
There was a problem hiding this comment.
Approved. I reviewed the diff and ran the full Layer 1-3 suite on this head; all tests passed.
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