fix(hitl): address PR #128 review blockers

Propagate invalid decision kinds from buildEngineHitlOptions, reject
--decision without --resume, and error when interrupted checkpoints
lack pendingHitl. Auto-approve now emits approval trace events; document
interruptOn semantics; validate interruptOn tool refs at load time.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: leo-aa88

README.md

@@ -49,7 +49,7 @@ The full product vision, YAML spec v0, and architecture are documented in [**`do
 - **`agentctl validate`** — load project, apply **project defaults** (`spec.defaults`), then **environment overlays** (`-e` / `--env`, `Environment` resources §7.6), then validate graph, schemas, and references  
 - **`agentctl plan`** — diff desired graph vs SQLite **deployment** state; risk hints; JSON/YAML output includes a **`deploymentBaseline`** digest for the store snapshot  
 - **`agentctl apply`** — persist plan (TTY confirm or `--auto-approve` / `AGENTCTL_AUTO_APPROVE`); **optimistic concurrency** — if the deployment store changed after the plan snapshot (e.g. another process applied the same `--state` file while this run waited at the prompt), apply fails with **exit code 3**; re-run **plan** then **apply**  
-- **`agentctl run`** — execute a workflow locally; JSON Schema for inputs where configured; policy gates  
+- **`agentctl run`** — execute a workflow locally; JSON Schema for inputs where configured; policy gates pause for **human-in-the-loop (HITL)** approval when a tool call requires it  
 - **`agentctl logs`** — read **trace events** from SQLite (`--run`, `--workflow`, or recent runs)  
 - **Tools** — **`native`**, **`http`**, **`mock`**, and **`mcp`** — MCP supports **stdio** (subprocess) or **streamable HTTP** (`spec.mcp.transport: http`, `url`, optional `headers` with `env:` tokens)  
 - **Project defaults** — besides **`model`** and **`policy`**, optional **`runtime`** flows to **`spec.runtime`** on agents/workflows when omitted (MVP: **`local`** or unset; see spec validation)  
@@ -142,6 +142,8 @@ Notes:
 
 - **`init`** creates `my-agent-system/` with `apiVersion: agentic.dev/v0` resources and a **`hello`** workflow (native `echo` tool only — **no network**).  
 - **`apply`** in non-interactive environments needs **`--auto-approve`** or **`AGENTCTL_AUTO_APPROVE=1`**.  
+- **`run`** HITL: gated tool calls exit with **`Status: interrupted`** (exit **0**). Resume with **`--resume <run-id> --decision approve|reject|edit|switch`** (use **`--decision-edit-json`** / **`--decision-switch-target`** when needed), or skip prompts with **`--auto-approve`** / **`AGENTCTL_AUTO_APPROVE=1`**. Pre-approve a specific call with repeated **`--approve <uses>`**. Set **`AGENTCTL_HITL_ACTOR`** to attribute decisions in trace logs.  
+- **`Policy.spec.hitl.interruptOn`** keys are **Tool metadata.name** values; they configure review options (edit rules, switch targets) for calls already gated by **`approvals.requiredFor`** or safety metadata — they do not gate tools on their own.  
 - **`run`** stores traces in the **same** SQLite file used for plan/apply (default **`.agentic/state.db`** under `--project`).  
 - If **`spec.traces.retentionDays`** is a positive integer, runs older than that many **UTC calendar days** (by `runs.started_at`) are deleted lazily on **`run`** and **`logs`** (child trace rows cascade). Unset or non-positive means no pruning.  
 - Use **`logs --run <id>`** after a run if you want a single run’s trace (IDs are printed by **`run`**).  

internal/cli/hitl.go

@@ -2,55 +2,76 @@ package cli
 
 import (
 	"bufio"
+	"context"
 	"encoding/json"
 	"fmt"
 	"io"
 	"os"
 	"strings"
 
+	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/engine"
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/policy"
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/runtime"
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/spec"
+	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/state"
 	"github.com/mattn/go-isatty"
 )
 
 // EnvHitlActor overrides the actor recorded on approval trace events.
 const EnvHitlActor = "AGENTCTL_HITL_ACTOR"
 
+// maxDecisionEditJSONBytes caps --decision-edit-json size (well below checkpoint limits).
+const maxDecisionEditJSONBytes = 1 << 20
+
 func hitlActorFromEnv() string {
 	if v := strings.TrimSpace(os.Getenv(EnvHitlActor)); v != "" {
 		return v
 	}
 	if u := strings.TrimSpace(os.Getenv("USER")); u != "" {
 		return u
 	}
-	return "operator"
+	return policy.DefaultHitlActor
 }
 
-func applyHitlRunOptions(opts *runtime.WorkflowRunOptions, autoApprove bool, decision string, editJSON string, switchTarget string) error {
+func applyHitlRunOptions(opts *runtime.WorkflowRunOptions, resuming bool, autoApprove bool, decision string, editJSON string, switchTarget string) error {
 	opts.AutoApprove = autoApprove || envAutoApproveEnabled()
 	opts.HitlActor = hitlActorFromEnv()
 	decision = strings.TrimSpace(decision)
+	editJSON = strings.TrimSpace(editJSON)
+	switchTarget = strings.TrimSpace(switchTarget)
+
 	if decision == "" {
+		if editJSON != "" || switchTarget != "" {
+			return fmt.Errorf("run: --decision-edit-json and --decision-switch-target require --decision")
+		}
 		return nil
 	}
+	if !resuming {
+		return fmt.Errorf("run: --decision requires --resume <run-id>")
+	}
 	kind, err := spec.ParseHitlDecisionKind(decision)
 	if err != nil {
 		return err
 	}
-	hd := &runtime.HitlDecisionOptions{Kind: string(kind)}
+	hd := &runtime.HitlDecisionOptions{Kind: kind}
 	switch kind {
 	case spec.HitlDecisionEdit:
-		if strings.TrimSpace(editJSON) == "" {
+		if editJSON == "" {
 			return fmt.Errorf("run: --decision edit requires --decision-edit-json")
 		}
+		if len(editJSON) > maxDecisionEditJSONBytes {
+			return fmt.Errorf("run: --decision-edit-json exceeds %d bytes", maxDecisionEditJSONBytes)
+		}
 		var m map[string]any
 		if err := json.Unmarshal([]byte(editJSON), &m); err != nil {
 			return fmt.Errorf("run: --decision-edit-json: %w", err)
 		}
+		if m == nil {
+			return fmt.Errorf("run: --decision-edit-json must be a JSON object")
+		}
 		hd.EditedWith = m
 	case spec.HitlDecisionSwitch:
-		hd.SwitchTarget = strings.TrimSpace(switchTarget)
+		hd.SwitchTarget = switchTarget
 		if hd.SwitchTarget == "" {
 			return fmt.Errorf("run: --decision switch requires --decision-switch-target")
 		}
@@ -82,7 +103,7 @@ func maybePromptHitlDecision(in io.Reader, out io.Writer, gate policy.HitlGate)
 			fmt.Fprintf(out, "Unknown decision %q\n", line)
 			continue
 		}
-		if !decisionAllowed(kind, gate.Review.AllowedDecisions) {
+		if !policy.IsDecisionAllowed(kind, gate.Review.AllowedDecisions) {
 			fmt.Fprintf(out, "Decision %q is not allowed for this call\n", kind)
 			continue
 		}
@@ -94,6 +115,10 @@ func maybePromptHitlDecision(in io.Reader, out io.Writer, gate policy.HitlGate)
 			if err != nil {
 				return nil, err
 			}
+			if len(editLine) > maxDecisionEditJSONBytes {
+				fmt.Fprintf(out, "Edited args exceed %d bytes\n", maxDecisionEditJSONBytes)
+				continue
+			}
 			var m map[string]any
 			if err := json.Unmarshal([]byte(editLine), &m); err != nil {
 				fmt.Fprintf(out, "Invalid JSON: %v\n", err)
@@ -116,15 +141,6 @@ func maybePromptHitlDecision(in io.Reader, out io.Writer, gate policy.HitlGate)
 	}
 }
 
-func decisionAllowed(kind spec.HitlDecisionKind, allowed []spec.HitlDecisionKind) bool {
-	for _, a := range allowed {
-		if a == kind {
-			return true
-		}
-	}
-	return false
-}
-
 func readLine(r io.Reader) (string, error) {
 	sc := bufio.NewScanner(r)
 	if !sc.Scan() {
@@ -135,3 +151,42 @@ func readLine(r io.Reader) (string, error) {
 	}
 	return strings.TrimSpace(sc.Text()), nil
 }
+
+func hitlGateFromCheckpoint(contextJSON string) (*policy.HitlGate, error) {
+	var payload struct {
+		PendingHitl *engine.PendingHitlState `json:"pendingHitl,omitempty"`
+	}
+	if err := json.Unmarshal([]byte(contextJSON), &payload); err != nil {
+		return nil, fmt.Errorf("unmarshal checkpoint: %w", err)
+	}
+	if payload.PendingHitl == nil {
+		return nil, nil
+	}
+	p := payload.PendingHitl
+	return &policy.HitlGate{
+		Uses:   p.Uses,
+		With:   p.With,
+		Review: p.Review,
+	}, nil
+}
+
+// loadPendingHitlGate reads the latest checkpoint for a run awaiting HITL input.
+func loadPendingHitlGate(ctx context.Context, st state.RuntimeStore, runID string) (*policy.HitlGate, error) {
+	cp, err := st.GetLatestCheckpoint(ctx, runID)
+	if err != nil {
+		return nil, err
+	}
+	return hitlGateFromCheckpoint(cp.ContextJSON)
+}
+
+// requirePendingHitlGate returns the pending gate or an error when interrupted without one.
+func requirePendingHitlGate(ctx context.Context, st state.RuntimeStore, runID string) (*policy.HitlGate, error) {
+	gate, err := loadPendingHitlGate(ctx, st, runID)
+	if err != nil {
+		return nil, err
+	}
+	if gate == nil {
+		return nil, fmt.Errorf("run: run %q is interrupted but checkpoint has no pending approval gate", runID)
+	}
+	return gate, nil
+}

internal/cli/hitl_load.go

@@ -225,7 +225,7 @@ func runRun(cmd *cobra.Command, wfName, resumeRunID, inputFile string, inputPair
 			Resume:          resumeID != "",
 			RunID:           resumeID,
 		}
-		if err := applyHitlRunOptions(&opts, autoApprove, decision, decisionEditJSON, decisionSwitchTarget); err != nil {
+		if err := applyHitlRunOptions(&opts, resumeID != "", autoApprove, decision, decisionEditJSON, decisionSwitchTarget); err != nil {
 			return NewExitError(ExitValidationError, err)
 		}
 		if !opts.Resume {
@@ -243,14 +243,17 @@ func runRun(cmd *cobra.Command, wfName, resumeRunID, inputFile string, inputPair
 		if runErr == nil && runID != "" {
 			if r, gerr := st.GetRun(ctx, runID); gerr == nil && r != nil && r.Status == state.RunStatusInterrupted {
 				if opts.AutoApprove || strings.TrimSpace(decision) != "" {
+					if _, gerr := requirePendingHitlGate(ctx, st, runID); gerr != nil {
+						return gerr
+					}
 					resumeID = runID
 					continue
 				}
-				gate, gerr := loadPendingHitlGate(ctx, st, runID)
+				gate, gerr := requirePendingHitlGate(ctx, st, runID)
 				if gerr != nil {
-					return fmt.Errorf("run: load hitl gate: %w", gerr)
+					return gerr
 				}
-				if gate != nil && isatty.IsTerminal(os.Stdin.Fd()) {
+				if isatty.IsTerminal(os.Stdin.Fd()) {
 					dec, perr := maybePromptHitlDecision(cmd.InOrStdin(), cmd.OutOrStdout(), *gate)
 					if perr != nil {
 						return perr
@@ -333,6 +336,9 @@ func writeRunOutput(cmd *cobra.Command, ctx context.Context, st *sqlite.Store, e
 			fmt.Fprintf(&b, "\nRun ID: %s\n", runID)
 			if got != nil {
 				fmt.Fprintf(&b, "Status: %s\n", got.Status)
+				if got.Status == state.RunStatusInterrupted {
+					fmt.Fprintf(&b, "Resume with: agentctl run --resume %s --decision approve|reject|edit|switch ...\n", runID)
+				}
 				if got.ErrorText != "" {
 					fmt.Fprintf(&b, "Error: %s\n", got.ErrorText)
 				}

internal/cli/run.go

@@ -175,6 +175,105 @@ func TestRun_policyGated_interruptThenResumeApprove(t *testing.T) {
 	}
 }
 
+func TestRun_decisionWithoutResume_exit2(t *testing.T) {
+	db := filepath.Join(t.TempDir(), "run-decision.db")
+	root := runPolicyRoot(t)
+	ResetGlobalsForTest()
+	cmd := NewRootCmd()
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	cmd.SetArgs([]string{
+		"run", "workflow/gated",
+		"--project", root,
+		"--state", db,
+		"--input", "topic=x",
+		"--decision", "approve",
+	})
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("expected validation error")
+	}
+	if ExitCodeOf(err) != ExitValidationError {
+		t.Fatalf("exit=%d want %d err=%v", ExitCodeOf(err), ExitValidationError, err)
+	}
+}
+
+func TestRun_hitlRejectViaResume(t *testing.T) {
+	db := filepath.Join(t.TempDir(), "run-reject.db")
+	root := runPolicyRoot(t)
+	runID := runPolicyInterrupted(t, root, db)
+
+	ResetGlobalsForTest()
+	var out bytes.Buffer
+	cmd := NewRootCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	cmd.SetArgs([]string{
+		"run", "--resume", runID,
+		"--project", root,
+		"--state", db,
+		"--decision", "reject",
+	})
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("expected rejection error")
+	}
+	if ExitCodeOf(err) != ExitExecutionError {
+		t.Fatalf("exit=%d want %d err=%v", ExitCodeOf(err), ExitExecutionError, err)
+	}
+}
+
+func TestRun_hitlEditViaResume(t *testing.T) {
+	db := filepath.Join(t.TempDir(), "run-edit.db")
+	root := runPolicyRoot(t)
+	runID := runPolicyInterrupted(t, root, db)
+
+	ResetGlobalsForTest()
+	var out bytes.Buffer
+	cmd := NewRootCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	cmd.SetArgs([]string{
+		"run", "--resume", runID,
+		"--project", root,
+		"--state", db,
+		"--decision", "edit",
+		"--decision-edit-json", `{"topic":"edited"}`,
+	})
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("resume edit: %v\n%s", err, out.String())
+	}
+	if !strings.Contains(out.String(), "Status: succeeded") {
+		t.Fatalf("expected succeeded:\n%s", out.String())
+	}
+}
+
+func runPolicyInterrupted(t *testing.T, root, db string) string {
+	t.Helper()
+	ResetGlobalsForTest()
+	var out bytes.Buffer
+	cmd := NewRootCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	cmd.SetArgs([]string{
+		"run", "workflow/gated",
+		"--project", root,
+		"--state", db,
+		"--input", "topic=x",
+	})
+	if err := cmd.Execute(); err != nil {
+		t.Fatalf("interrupt run: %v\n%s", err, out.String())
+	}
+	for _, line := range strings.Split(out.String(), "\n") {
+		line = strings.TrimSpace(line)
+		if strings.HasPrefix(line, "Run ID:") {
+			return strings.TrimSpace(strings.TrimPrefix(line, "Run ID:"))
+		}
+	}
+	t.Fatal("missing run id")
+	return ""
+}
+
 func TestRun_withApprove_succeeds(t *testing.T) {
 	db := filepath.Join(t.TempDir(), "run-ok.db")
 	root := runPolicyRoot(t)

internal/cli/run_test.go

@@ -6,3 +6,10 @@ spec:
   approvals:
     requiredFor:
       - "tool.helper.echo"
+  hitl:
+    descriptionPrefix: "Echo tool requires operator approval"
+    interruptOn:
+      helper:
+        allowedDecisions: [approve, reject, edit]
+        allowedEditArgs: [topic]
+        description: "Review echo call (${uses})"

internal/cli/testdata/run_policy/policy.yaml

@@ -163,6 +163,7 @@ func (e *Executor) Run(ctx context.Context, in RunInput) error {
 					if gerr != nil {
 						err = gerr
 					} else if gate != nil {
+						e.recordAutoApproveHitl(ctx, in.RunID, step, i, *gate, in.Hitl.Actor)
 						pctx.ApprovedActions = append(append([]string(nil), pctx.ApprovedActions...), uses)
 					}
 				}