fix: stop rework loops on Git publication authorization denials - #45
Conversation
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
src/workflow_execute.rs:445(execute_workflow_with_hub) —PreparedEnvironment::prepare_off_runtime(...)is still called unconditionally whenever a resumed workflow resolves a runner-owned environment. The new denial path persistsenvironment.handle()and then returns from the runner, but this startup path never reads that retained binding or reattaches to its handle. Consequently, resuming publication starts a second empty node rather than the node containing the unpublishedHEAD/tree; the exact work this task is intended to preserve is not actually available to the resumed publication attempt. REQUIRED CHANGE: before preparing a runner-owned environment, look up the workflow's retained publication binding (using the persisted workflow/environment identity), probe and reattach to that existingEnvironmentHandlethrough aPreparedEnvironmentattach/reuse API, and skipprepare_off_runtimewhen it is live. Keep that binding until durable publication or explicit cancellation; only prepare a replacement when no retained publication hold exists (a dead held node should produce a loud operator-facing failure, not silently continue from an empty workspace). Verification: add a restart-boundary integration test that (a) prepares node A, creates a known commit/tree, (b) injects a workflow-permission denial and lets the first runner return paused, (c) resumes with a fresh runner instance, and asserts there is no second prepare, the resumed node reports the exact originalHEAD^{commit}andHEAD^{tree}, publication succeeds, and teardown occurs exactly once.
The denial classification is correctly scoped to code-open-pr, the diagnostic redaction/remediation text is useful, transient/non-fast-forward failures retain their existing path, and the Rust quality gate is green. The remaining restart/reattachment gap is central to the task's preservation requirement, so this cannot be merged yet.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
-
src/workflow_execute.rs:1374(execute_workflow_with_hub) —if reported_workflow_status == WorkflowStatus::Cancelled { cleanup_allowed.store(true, ...) }. This only releases the environment when cancellation is observed before the current runner invocation exits. The authorization-denial path pauses, returns from this invocation, and drops the guard with cleanup disabled; a later explicitworkflow canceldoes not re-enter this branch, so the persisted retained node is never torn down even though the task explicitly makes cancellation the cleanup escape hatch. REQUIRED CHANGE: extend the external workflow-cancellation path to load the persisted publication-hold binding, resolve/attach to that exact environment handle, call teardown once, and mark or clear the persisted binding only after teardown succeeds; keep this in-run branch as an idempotent fast path if useful. Verify with an integration test that runs process A through publication denial and pause, destroys process A, cancels the workflow through the normal control API in process B, and asserts node A is torn down exactly once and its hold is no longer reusable. -
src/workflow_execute.rs(persist_runner_publication_hold) —if let Err(err) = persist_runner_publication_binding(...) { eprintln!("warning: failed to persist held publication environment ...") }. A failed checkpoint write is swallowed, after which the workflow still reports a normal durable human pause and the resident host is released; on restart there is no authoritative handle to reattach, so the exact unpublished commit/tree cannot be recovered by this implementation. REQUIRED CHANGE: make publication-hold persistence return aResultand propagate failure into a loud operator-visible blocked/escalated outcome before claiming that the environment is durably retained; include the redacted persistence failure and handle identity in the durable fallback record/event, or otherwise make the hold write atomic and mandatory. Verify with a test using an unwritable/corrupt scoped state root: the runner must not emit the successful retained-environment pause, must surface the persistence error, must not prepare a replacement node on restart, and must not tear down the original node.
The denial classifier is correctly scoped to code-open-pr, preserves non-fast-forward/transport handling, and the redaction plus exact commit/tree diagnostics are strong. The new reattach liveness probe also correctly fails closed instead of silently creating an empty replacement. CI is currently configured but still pending (Rust quality gates in progress), so this verdict is based on the lifecycle defects above rather than the pending signal.
There was a problem hiding this comment.
VERDICT: CHANGES REQUESTED
src/workflow_execute.rs:1394(execute_workflow_with_hub) —if reported_workflow_status == WorkflowStatus::Cancelled { cleanup_allowed.store(true, ...) }. The latest synchronize commit makes publication-hold persistence fail closed, but it does not address the other outstanding lifecycle defect: this branch only tears down when cancellation is observed inside the same runner invocation. After an authorization denial has paused and returned, a later cancellation through the normal control API does not execute this code, so the persisted node remains retained indefinitely. REQUIRED CHANGE: modify the external workflow-cancellation/control path to load the persisted blocked publication binding, resolve and attach to that exactEnvironmentHandle, call teardown exactly once, and mark the bindingtorn_downonly after teardown succeeds; preserve the binding on teardown failure so reconciliation can retry. Keep this in-run branch as an idempotent fast path if desired. Verification: add a process-boundary integration test in which process A persists a publication denial hold and exits, process B cancels the workflow through the normal control API, and assertions prove node A is torn down exactly once, no replacement node is prepared, andfind_retained_publication_bindingno longer returns that hold; also inject teardown failure and assert the binding remains reusable for retry.
The new Result propagation from persist_runner_publication_hold correctly removes the false durable-pause behavior, includes the handle identity in the failure chain, and the corrupt-state-root regression test is a useful fail-closed check. Reattachment and dead-node handling also remain sound. CI is configured and currently pending (Rust quality gates in progress); this verdict is based on the still-unimplemented external-cancellation cleanup path, not the pending signal.
There was a problem hiding this comment.
VERDICT: APPROVE
No blocking findings.
I re-reviewed exact head f2d1cd5c65088e7867c893ade1f1ee127fe1b810 against TASK-1121 and the coordinated Animus core lifecycle at merged commit 7b4c89ce6d910358470dbc3bb03d2db3b57523dd (PR #354).
The runner now correctly scopes deterministic authorization/policy classification to code-open-pr, converts those failures to a durable manual hold before rework routing, redacts diagnostics, records the exact unpublished commit/tree, fail-closes hold persistence, retains the runner-owned environment, and reattaches the exact live handle without a replacement prepare. Existing non-fast-forward and transient failure recovery remains outside the non-retryable classifier. The regression coverage exercises one publication attempt, zero code reworks, no denial-path teardown, redaction, exact-handle restart reattachment, fail-loud dead-handle behavior, and exactly-once successful-publication teardown.
The companion core change resolves the previously open external-cancellation edge through both normal CLI and control-wire cancellation paths. It scans all workflow phase checkpoints rather than only the current phase, groups identical environment/plugin handles, tears each distinct handle down once, marks all references only after successful cleanup, preserves failed cleanup for retry, and includes the earlier-phase/current-phase-without-binding regression plus repeated-call idempotency.
Verification: runner CI Rust quality gates is green at the exact PR head. Companion PR #354 is merged to main at 7b4c89ce, and its CI checks are green. The coordinated lifecycle is complete and consistent across the shared checkpoint wire shape.
What is especially good: the change preserves unpublished work by making the retained handle authoritative, while keeping publication proof as the sole normal teardown gate and explicit cancellation as the deliberate escape hatch.
Summary
Validation
Production evidence
TASK-956 workflow 07dc2f5b-9712-4410-9da1-fdff87aa48f0 encountered a GitHub App workflow-permission denial and exhausted ten code reworks. This change turns that deterministic denial into one durable human escalation without discarding the environment or unpublished work.
Tracks TASK-1121.