mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-11-02 08:41:03 +00:00
rcourtman
a4cdb4af87
## What was wrong
`actionlifecycle.Service.Execute` read the operator remediation lock
inside `ValidateCurrentReadiness` and committed the `executing`
transition afterwards, with nothing holding the two together. A "Never
auto-remediate" (or Retired) save that landed between the readiness read
and `RecordActionExecutionAdmission` was acknowledged to the operator
and the approved action still reached the executor. `ExecuteUnderPolicy`
already held the admission read lock across the same steps, and
operator-state PUT/DELETE already take the write side through
`WithPolicyMutation`, so only the human path was outside the boundary.
No downstream gate closed it: the HTTP handler delegates straight to
`Execute`, none of the typed executors (`routedActionExecutor`, Docker,
Proxmox guest, host update, host storage cleanup) read the lock, and
neither store's admission write consults operator state.
## Change
`Execute` now runs its gates and the admission commit in
`beginHumanExecution` under `coordinator.mu.RLock()` and releases the
lock before `dispatchCommitted` calls the executor. A lock save either
lands before the readiness check and refuses the action, or queues
behind the committed admission.
An admitted dispatch is deliberately not recalled by a later lock,
including an attempt that was admitted but never sent and is resumed by
`RecoverExecutingActions`. Recalling known-unsent work needs an atomic,
conditional pre-send terminalization that neither store has
(`RecordActionExecutionRefusal` only accepts planned/pending/approved
rows; `ForceFail` records inconclusive truth). The contract now says so,
and notes that operator-facing copy should say an already-admitted
action can still run.
## Proof
-
`TestExecuteAdmissionLinearizesWithOperatorLockSave{MemoryStore,SQLite}`
starts a real policy writer inside the window (through the executor's
availability hook, after the lock read and before admission), asserts
the coordinator excludes a writer there, and orders the save
acknowledgement against the admission commit. Against the old
`service.go` it fails on both assertions: ack seq 1, admission seq 2,
executor called.
- `TestExecuteAdmittedDispatchIsNotRecalledByLaterOperatorLock` pins
release-before-dispatch (a save during `ExecuteAction` must not block)
and no recall.
-
`TestRecoverExecutingActionsDoesNotRecallQueuedAttemptAfterOperatorLock`
pins the unsent-attempt boundary.
- The whole `internal/actionlifecycle` package passes under `-race` on
pulse-dev.
Hooks now fire under the read lock on the human path as they already did
on the automatic path. `WithPolicyMutation`'s comment and
`api-contracts.md` say hooks must not request a policy write or re-enter
`Execute`/`ExecuteUnderPolicy`.
## Not changed
`PulseToolExecutor.recordActionExecutionStart`
(`internal/ai/tools/action_audit.go`) has the same check-then-record
shape outside the coordinator. I did not find a live route into it: its
mutation helpers (`executeRunCommand`, `executeControlGuest`,
`executeDockerControl`, `executeKubernetesScale`,
`executeNativeAppContainerControl`) have no non-test callers, and the
file write/append helpers sit behind `pulse_file_edit`, which the
mutation registry records as `retired_denied` /
`denied_before_transport`. Removing that retained code belongs with the
dead Assistant handler cleanup.