diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 633cfb4..f607abc 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -403,23 +403,28 @@ func newServer(pool *db.Pool, agentID uuid.UUID) *mcp.Server { if assentWindowActive(ctx, pool, agentID) { pool.Exec(ctx, `UPDATE executions SET status='pending_approval', risk_class='config_mutation' WHERE entity_id=$1`, id) createApproval(ctx, pool, id, targetID, "apt_upgrade", params, "config_mutation") - if approved := autoApprove(ctx, pool, id); approved { - // context.Background(), NOT ctx: ctx is scoped to this - // MCP tool call, which ends (and cancels) as soon as the - // chat turn's HTTP response completes — normal, expected, - // happens on every turn. A goroutine meant to outlive the - // request must not inherit its context, or the async work - // dies silently the instant the turn ends. Found live: - // every assent-window auto-approved pct_create failed - // with "context canceled" the moment the triggering - // /chat request finished — exactly the same context- - // lifetime class of bug, in the one place it had been - // missed (httpapi's own approval goroutine already used - // context.Background() correctly). - go executeApprovedViaAPI(context.Background(), id, targetSlug, "apt_upgrade:"+params) - slog.Info("mcp: apt_upgrade auto-approved via assent window", "execution_id", id) - return textResult(fmt.Sprintf("apt_upgrade on %s auto-approved via assent window — execution %s running.", targetSlug, id)), nil - } + // Do NOT pre-flip approvals/executions status here (that was + // the previous, broken "autoApprove" helper). DecideApproval + // (invoked below) is the ONE place that transitions + // pending_approval -> approved and dispatches the real SSH + // work — it specifically looks for status='pending_approval' + // to find what to run. Pre-flipping the status past that + // state meant DecideApproval's own lookup found nothing, + // silently no-opped, and the execution sat at 'approved' + // forever with nothing actually running. Found live: every + // assent-window auto-approved pct_create/apt_upgrade has + // never actually executed, via this exact bug. Calling + // executeApprovedViaAPI directly against the untouched + // pending_approval row makes this identical to the manual + // Approve-button path, just without a human click. + // + // context.Background(), NOT ctx: ctx is scoped to this MCP + // tool call, cancelled the instant the chat turn's HTTP + // response completes (every normal turn) — a goroutine + // meant to outlive the request must not inherit its context. + go executeApprovedViaAPI(context.Background(), id, targetSlug, "apt_upgrade:"+params) + slog.Info("mcp: apt_upgrade auto-approved via assent window", "execution_id", id) + return textResult(fmt.Sprintf("apt_upgrade on %s auto-approved via assent window — execution %s running.", targetSlug, id)), nil } // upgrade requires approval — queue pool.Exec(ctx, `UPDATE executions SET status='pending_approval', risk_class='config_mutation' WHERE entity_id=$1`, id) @@ -432,11 +437,12 @@ func newServer(pool *db.Pool, agentID uuid.UUID) *mcp.Server { if assentWindowActive(ctx, pool, agentID) { pool.Exec(ctx, `UPDATE executions SET status='pending_approval', risk_class='config_mutation' WHERE entity_id=$1`, id) createApproval(ctx, pool, id, targetID, "pct_create", params, "config_mutation") - if approved := autoApprove(ctx, pool, id); approved { - go executeApprovedViaAPI(context.Background(), id, targetSlug, "pct_create:"+params) // see context.Background() comment above (apt_upgrade case) — same bug, same fix - slog.Info("mcp: pct_create auto-approved via assent window", "execution_id", id) - return textResult(fmt.Sprintf("pct_create on %s auto-approved via assent window — execution %s running. The LXC is being provisioned now.", targetSlug, id)), nil - } + // See the apt_upgrade case above for why there's no + // pre-flip-status "autoApprove" step here anymore, and why + // this uses context.Background(). + go executeApprovedViaAPI(context.Background(), id, targetSlug, "pct_create:"+params) + slog.Info("mcp: pct_create auto-approved via assent window", "execution_id", id) + return textResult(fmt.Sprintf("pct_create on %s auto-approved via assent window — execution %s running. The LXC is being provisioned now.", targetSlug, id)), nil } pool.Exec(ctx, `UPDATE executions SET status='pending_approval', risk_class='config_mutation' WHERE entity_id=$1`, id) createApproval(ctx, pool, id, targetID, "pct_create", params, "config_mutation") @@ -1345,23 +1351,6 @@ func resolveExecTarget(ctx context.Context, pool *db.Pool, targetSlug string) (h // mirroring what DecideApproval does. Returns true on success. This is used // by the assent-window path to skip the operator-approval queue when the // operator already approved the overall plan via chat assent. -func autoApprove(ctx context.Context, pool *db.Pool, execID uuid.UUID) bool { - _, err := pool.Exec(ctx, ` - UPDATE approvals SET status='approved', decided_at=now(), decided_by=$1 - WHERE entity_id=$2 AND status='pending'`, - execID, execID) - if err != nil { - slog.Error("mcp: autoApprove update approval", "error", err, "execution", execID) - return false - } - _, err = pool.Exec(ctx, `UPDATE executions SET status='approved' WHERE entity_id=$1`, execID) - if err != nil { - slog.Error("mcp: autoApprove update execution", "error", err, "execution", execID) - return false - } - return true -} - // executeApprovedViaAPI calls the HTTP API's approval-decision endpoint to // trigger the actual execution. The API server (phase3.executeApprovedAction) // handles the real SSH work (pct create, apt upgrade, etc.) in a goroutine. @@ -1388,10 +1377,14 @@ func executeApprovedViaAPI(ctx context.Context, execID uuid.UUID, targetSlug, ac } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - // The autoApprove DB update may have already marked it approved, - // and the API found no pending approval to decide — that's fine, - // the execution was already triggered by the DB state change. - slog.Info("mcp: executeApprovedViaAPI non-200 (likely already decided)", "status", resp.StatusCode, "execution", execID) + // A non-200 here means the real SSH work was never dispatched — this + // is the call that actually triggers executeApprovedAction via + // DecideApproval. (A previous version of this comment claimed a + // non-200 was fine because a since-removed "autoApprove" step had + // already triggered execution via a raw DB update — it hadn't; that + // was the bug where auto-approved pct_create/apt_upgrade never + // actually ran. There is no other path that dispatches the work.) + slog.Error("mcp: executeApprovedViaAPI non-200 — execution was NOT dispatched", "status", resp.StatusCode, "execution", execID) } }