Skip to content

fix(runtime): verify-hook-blocked done escalates to needs-input (#571)#579

Open
IBondarenko-iwg wants to merge 2 commits into
andresharpe:mainfrom
IBondarenko-iwg:fix/571-verify-hook-done-needs-input
Open

fix(runtime): verify-hook-blocked done escalates to needs-input (#571)#579
IBondarenko-iwg wants to merge 2 commits into
andresharpe:mainfrom
IBondarenko-iwg:fix/571-verify-hook-done-needs-input

Conversation

@IBondarenko-iwg

Copy link
Copy Markdown
Contributor

Linked issue

Closes #571
Closes #557

Final child PR of the 8-bug epic #557 (bug 8). Merging Closes #571 and #557.

Summary of changes

When enter-done's verify chain (e.g. 00-privacy-scan.ps1) aborts the done-transition, HttpServer reverts the task to in-progress. The runner's retry loop couldn't tell that apart from "agent didn't finish", so it burned the retry budget and finalized skipped(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 breadcrumb extensions.runner.done_transition_block = @{ hook; message; at } on the reverted task (under extensions.runner because the schema rejects unknown top-level fields; status still reverts to in-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 to needs-input via the existing Set-WorkflowTaskNeedsInput (mirrors the AuthError park) — without consuming the retry budget, never skipped. 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 cited outcomes.md lands 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 carries extensions.runner.done_transition_block with 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 in needs-input (not skipped), retry budget intact; redact the path and re-run → completes.

Checklist

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_block on tasks when a done transition 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-input instead 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.

Comment thread src/runtime/Modules/Dotbot.Runtime/Private/HttpServer.psm1 Outdated
Comment thread src/runtime/Modules/Dotbot.Runtime/Private/HttpServer.psm1 Outdated
Comment thread src/runtime/Scripts/Invoke-WorkflowProcess.ps1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Inbox

2 participants