fix(automation): close PR2 review must-fixes on hook policy layer (MUL-4332 PR2)

Address Elon's four must-fixes on the hook CRUD layer. All four become real authorization / correctness issues once PR3 starts consuming saved rules. 1. [P0] Edit authorization gate. Any workspace member could PATCH/enable/disable/ delete any user hook while it kept running under the original principal (authorization-principal substitution). Now only the hook's original authorization principal OR a workspace owner/admin may modify it; the principal is NOT transferred on edit. UpdateHook/SetEnabled/ArchiveHook enforce authorizeHookEdit. (Deny-by-default per the proposed semantic, pending Bohan's final confirmation; easy to switch to principal-transfer.) 2. [P0] Fail-closed target/workspace validation. Create/update now validate, in the write transaction under the current principal, that every referenced target exists in the hook's workspace: issue scope, condition issue ids, and action issue/agent/member/autopilot targets — with the trigger_agent target also checked for archived/runtime and canInvokeAgent admission. The resolved principal must still be a workspace member. A missing target now yields 400, not a persisted 201 (§13). Adds GetMemberInWorkspace. 3. [P1] Strict schema. The handler decodes with DisallowUnknownFields (rejects unknown top-level/nested keys); the validator rejects per-action disallowed-but-known fields (e.g. agent_id on add_comment), validates the issue-status enum on set_issue_status / issues_status / issue_field(status), and bounds id-set and message sizes. 4. [P1] Concurrent PATCH. UpdateHook now SELECT ... FOR UPDATE locks the hook row and re-checks archived/origin/authorization inside the lock before allocating MAX(revision)+1, so concurrent edits serialize instead of colliding on idx_hook_revision_unique (was 23505 → 500). Adds GetHookForUpdate. Tests: strict-schema rejections (unknown field, disallowed action field, bad status enum), fail-closed targets (nonexistent issue/member/agent/autopilot → 400; real invokable agent → 201), edit authorization (non-principal non-admin → 403 on patch/disable/delete; principal and owner/admin → 200; principal never transferred), and 8-way concurrent PATCH asserting contiguous revisions 1..9 with no error. dry-run/explain + event-correlation queries are re-sliced to PR3 (they need the matcher's evaluation), pending Bohan's confirmation. Co-authored-by: multica-agent <github@multica.ai>
2026-07-31 00:40:46 +02:00 · 2026-07-16 19:10:24 +08:00
parent 500914052c
commit 06f6b85ca5
10 changed files with 804 additions and 148 deletions
--- a/server/internal/automation/schema.go
+++ b/server/internal/automation/schema.go
@@ -135,6 +135,23 @@ var validIssueFields = map[string]bool{
 	IssueFieldParentIssueID: true,
 }

+// validIssueStatuses mirrors the issue.status DB CHECK (migrations/001) and the
+// handler's validIssueStatuses. A status-valued condition or action is rejected
+// unless its value is one of these, so a hook can never persist an unreachable
+// status (MUL-4332 PR2 review point 3).
+var validIssueStatuses = map[string]bool{
+	"backlog":     true,
+	"todo":        true,
+	"in_progress": true,
+	"in_review":   true,
+	"done":        true,
+	"blocked":     true,
+	"cancelled":   true,
+}
+
+// isValidIssueStatus reports whether s is a persistable issue status.
+func isValidIssueStatus(s string) bool { return validIssueStatuses[s] }
+
 // conditionDependencyEvent maps a condition to the single v1 domain event that
 // can change its truth value. A rising_edge hook must listen to exactly that
 // event so its latch can be re-evaluated (§5.2). In the v1 fixed vocabulary
--- a/server/internal/automation/validate.go
+++ b/server/internal/automation/validate.go
@@ -12,6 +12,9 @@ import (
 const (
 	MaxActionsPerHook = 8
 	MaxNameLength     = 200
+	MaxConditionIDs   = 100
+	MaxMatchSetSize   = 100
+	MaxMessageLength  = 4000
 )

 // ValidationError is a user-fixable problem with a hook spec. Handlers map it to
@@ -20,6 +23,14 @@ type ValidationError struct{ msg string }

 func (e *ValidationError) Error() string { return e.msg }

+// NewValidationError builds a ValidationError. Exported so callers that extend
+// author-time validation with checks this pure package cannot do (e.g. the
+// service's workspace-scoped target existence checks) surface the same typed
+// error and share the handler's single 400 mapping.
+func NewValidationError(format string, args ...any) *ValidationError {
+	return &ValidationError{msg: fmt.Sprintf(format, args...)}
+}
+
 func verr(format string, args ...any) error {
 	return &ValidationError{msg: fmt.Sprintf(format, args...)}
 }
@@ -117,6 +128,9 @@ func validateMatch(raw []byte, schema EventSchema) error {
 }

 func validateClauseValues(field string, kind FieldKind, clause MatchClause) error {
+	if clause.Op == MatchIn && len(clause.Set) > MaxMatchSetSize {
+		return verr("match field %q has %d values, at most %d allowed", field, len(clause.Set), MaxMatchSetSize)
+	}
 	if kind != FieldUUID {
 		return nil // string fields accept any scalar; existence needs no value check
 	}
@@ -156,6 +170,9 @@ func validateIssuesStatus(c IssuesStatusCond) error {
 	if len(c.IDs) == 0 {
 		return verr("issues_status.ids must not be empty")
 	}
+	if len(c.IDs) > MaxConditionIDs {
+		return verr("issues_status.ids has %d ids, at most %d allowed", len(c.IDs), MaxConditionIDs)
+	}
 	for _, id := range c.IDs {
 		if !validUUID(id) {
 			return verr("issues_status.ids must be uuids, got %q", id)
@@ -165,6 +182,13 @@ func validateIssuesStatus(c IssuesStatusCond) error {
 	if hasAll == hasAny {
 		return verr("exactly one of issues_status.all or issues_status.any must be set")
 	}
+	status := c.All
+	if hasAny {
+		status = c.Any
+	}
+	if !isValidIssueStatus(status) {
+		return verr("issues_status status %q is not a valid issue status", status)
+	}
 	return nil
 }

@@ -179,8 +203,19 @@ func validateIssueField(c IssueFieldCond) error {
 	if hasEq == hasIn {
 		return verr("exactly one of issue_field.eq or issue_field.in must be set")
 	}
-	// Field-typed values: status is a free string; the id-shaped fields require uuids.
-	if c.Field != IssueFieldStatus {
+	if len(c.In) > MaxConditionIDs {
+		return verr("issue_field.in has %d values, at most %d allowed", len(c.In), MaxConditionIDs)
+	}
+	switch c.Field {
+	case IssueFieldStatus:
+		// status is a free string but must be a persistable issue status.
+		for _, v := range collectValues(c.Eq, c.In) {
+			if !isValidIssueStatus(v) {
+				return verr("issue_field status %q is not a valid issue status", v)
+			}
+		}
+	default:
+		// id-shaped fields (assignee_id / parent_issue_id) require uuids.
 		if hasEq && !validUUID(c.Eq) {
 			return verr("issue_field.eq must be a uuid for field %q", c.Field)
 		}
@@ -193,6 +228,16 @@ func validateIssueField(c IssueFieldCond) error {
 	return nil
 }

+// collectValues returns eq (if set) plus the in slice as one list.
+func collectValues(eq string, in []string) []string {
+	out := make([]string, 0, len(in)+1)
+	if eq != "" {
+		out = append(out, eq)
+	}
+	out = append(out, in...)
+	return out
+}
+
 func validateFire(spec HookSpec) error {
 	switch spec.Fire.Mode {
 	case FirePerEvent:
@@ -225,6 +270,18 @@ func validateRisingEdgeCoverage(spec HookSpec) error {
 	return nil
 }

+// actionAllowedFields declares the exact set of ActionSpec parameter fields each
+// action type may set. Any other non-empty field is rejected so a stray param
+// (e.g. an agent_id smuggled onto add_comment) can never be persisted onto a
+// revision (MUL-4332 PR2 review point 3).
+var actionAllowedFields = map[string]map[string]bool{
+	ActionSetIssueStatus: {"issue_id": true, "status": true},
+	ActionTriggerAgent:   {"issue_id": true, "agent_id": true},
+	ActionAddComment:     {"issue_id": true, "message": true},
+	ActionSendInbox:      {"member_id": true, "message": true},
+	ActionRunAutopilot:   {"autopilot_id": true},
+}
+
 func validateAction(a ActionSpec) error {
 	if systemActionTypes[a.Type] {
 		return verr("action type %q is reserved for system hooks", a.Type)
@@ -232,6 +289,12 @@ func validateAction(a ActionSpec) error {
 	if !userActionTypes[a.Type] {
 		return verr("unknown action type %q", a.Type)
 	}
+	if err := rejectUnexpectedActionFields(a); err != nil {
+		return err
+	}
+	if a.Message != "" && len(a.Message) > MaxMessageLength {
+		return verr("%s message must be at most %d characters", a.Type, MaxMessageLength)
+	}
 	switch a.Type {
 	case ActionSetIssueStatus:
 		if !validUUID(a.IssueID) {
@@ -240,6 +303,9 @@ func validateAction(a ActionSpec) error {
 		if a.Status == "" {
 			return verr("set_issue_status requires status")
 		}
+		if !isValidIssueStatus(a.Status) {
+			return verr("set_issue_status status %q is not a valid issue status", a.Status)
+		}
 	case ActionTriggerAgent:
 		if !validUUID(a.IssueID) {
 			return verr("trigger_agent requires a valid issue_id")
@@ -269,6 +335,26 @@ func validateAction(a ActionSpec) error {
 	return nil
 }

+// rejectUnexpectedActionFields fails if the action sets any parameter field not
+// allowed for its type — strict "exactly the allowed fields" enforcement.
+func rejectUnexpectedActionFields(a ActionSpec) error {
+	allowed := actionAllowedFields[a.Type]
+	present := map[string]string{
+		"issue_id":     a.IssueID,
+		"status":       a.Status,
+		"agent_id":     a.AgentID,
+		"member_id":    a.MemberID,
+		"message":      a.Message,
+		"autopilot_id": a.AutopilotID,
+	}
+	for field, value := range present {
+		if value != "" && !allowed[field] {
+			return verr("%s does not accept field %q", a.Type, field)
+		}
+	}
+	return nil
+}
+
 func validUUID(s string) bool {
 	if s == "" {
 		return false
--- a/server/internal/automation/validate_test.go
+++ b/server/internal/automation/validate_test.go
@@ -79,6 +79,17 @@ func TestValidateRejectsInvalidSpecs(t *testing.T) {
 		{"set_issue_status missing status", func(s *HookSpec) {
 			s.Do = []ActionSpec{{Type: ActionSetIssueStatus, IssueID: uuidC}}
 		}, "requires status"},
+		{"set_issue_status invalid status enum", func(s *HookSpec) {
+			s.Do = []ActionSpec{{Type: ActionSetIssueStatus, IssueID: uuidC, Status: "ascended"}}
+		}, "not a valid issue status"},
+		{"add_comment with disallowed agent_id field", func(s *HookSpec) {
+			s.Do = []ActionSpec{{Type: ActionAddComment, IssueID: uuidC, Message: "hi", AgentID: uuidA}}
+		}, "does not accept field"},
+		{"issues_status invalid status enum", func(s *HookSpec) {
+			s.When = WhenSpec{Event: "issue.status_changed"}
+			s.Fire = FireSpec{Mode: FirePerEvent}
+			s.If = []ConditionSpec{{IssuesStatus: &IssuesStatusCond{IDs: []string{uuidA}, All: "ascended"}}}
+		}, "not a valid issue status"},
 		{"trigger_agent bad agent", func(s *HookSpec) {
 			s.Do = []ActionSpec{{Type: ActionTriggerAgent, IssueID: uuidC, AgentID: "x"}}
 		}, "valid agent_id"},
--- a/server/internal/handler/hook.go
+++ b/server/internal/handler/hook.go
@@ -129,18 +129,17 @@ func (h *Handler) CreateHook(w http.ResponseWriter, r *http.Request) {
 		return
 	}

-	var spec automation.HookSpec
-	if err := json.NewDecoder(r.Body).Decode(&spec); err != nil {
-		writeError(w, http.StatusBadRequest, "invalid request body")
-		return
-	}
-
-	author, ok := h.resolveHookAuthor(w, r, userID, workspaceID)
+	spec, ok := decodeHookSpec(w, r)
 	if !ok {
 		return
 	}

-	result, err := h.HookService.CreateHook(r.Context(), workspaceUUID, spec, author)
+	author, canInvoke, ok := h.resolveHookWriter(w, r, userID, workspaceID)
+	if !ok {
+		return
+	}
+
+	result, err := h.HookService.CreateHook(r.Context(), workspaceUUID, spec, author, canInvoke)
 	if err != nil {
 		h.writeHookError(w, err)
 		return
@@ -201,16 +200,15 @@ func (h *Handler) UpdateHook(w http.ResponseWriter, r *http.Request) {
 	if !ok {
 		return
 	}
-	var spec automation.HookSpec
-	if err := json.NewDecoder(r.Body).Decode(&spec); err != nil {
-		writeError(w, http.StatusBadRequest, "invalid request body")
-		return
-	}
-	author, ok := h.resolveHookAuthor(w, r, userID, workspaceID)
+	spec, ok := decodeHookSpec(w, r)
 	if !ok {
 		return
 	}
-	result, err := h.HookService.UpdateHook(r.Context(), workspaceUUID, hookUUID, spec, author)
+	author, canInvoke, ok := h.resolveHookWriter(w, r, userID, workspaceID)
+	if !ok {
+		return
+	}
+	result, err := h.HookService.UpdateHook(r.Context(), workspaceUUID, hookUUID, spec, author, canInvoke)
 	if err != nil {
 		h.writeHookError(w, err)
 		return
@@ -232,6 +230,11 @@ func (h *Handler) setHookEnabled(w http.ResponseWriter, r *http.Request, enabled
 	if !h.hookEnabled(w, r) {
 		return
 	}
+	workspaceID := h.resolveWorkspaceID(r)
+	userID, ok := requireUserID(w, r)
+	if !ok {
+		return
+	}
 	workspaceUUID, hookUUID, ok := h.hookPathParams(w, r)
 	if !ok {
 		return
@@ -247,7 +250,11 @@ func (h *Handler) setHookEnabled(w http.ResponseWriter, r *http.Request, enabled
 		}
 		reason = body.Reason
 	}
-	result, err := h.HookService.SetEnabled(r.Context(), workspaceUUID, hookUUID, enabled, reason)
+	author, _, ok := h.resolveHookWriter(w, r, userID, workspaceID)
+	if !ok {
+		return
+	}
+	result, err := h.HookService.SetEnabled(r.Context(), workspaceUUID, hookUUID, enabled, reason, author)
 	if err != nil {
 		h.writeHookError(w, err)
 		return
@@ -260,11 +267,20 @@ func (h *Handler) DeleteHook(w http.ResponseWriter, r *http.Request) {
 	if !h.hookEnabled(w, r) {
 		return
 	}
+	workspaceID := h.resolveWorkspaceID(r)
+	userID, ok := requireUserID(w, r)
+	if !ok {
+		return
+	}
 	workspaceUUID, hookUUID, ok := h.hookPathParams(w, r)
 	if !ok {
 		return
 	}
-	if err := h.HookService.ArchiveHook(r.Context(), workspaceUUID, hookUUID); err != nil {
+	author, _, ok := h.resolveHookWriter(w, r, userID, workspaceID)
+	if !ok {
+		return
+	}
+	if err := h.HookService.ArchiveHook(r.Context(), workspaceUUID, hookUUID, author); err != nil {
 		h.writeHookError(w, err)
 		return
 	}
@@ -306,28 +322,61 @@ func (h *Handler) hookPathParams(w http.ResponseWriter, r *http.Request) (pgtype
 	return workspaceUUID, hookUUID, true
 }

-// resolveHookAuthor derives the audit creator actor and the accountable human
-// principal (§8). A member acts under their own authority; an agent must resolve
-// to the human behind its current task, otherwise creation is refused — an agent
-// UUID is never a substitute for a human authorization principal.
-func (h *Handler) resolveHookAuthor(w http.ResponseWriter, r *http.Request, userID, workspaceID string) (service.HookAuthor, bool) {
+// decodeHookSpec strictly decodes the request body into a HookSpec, rejecting
+// unknown fields so a stray top-level or nested key can never be silently
+// accepted (MUL-4332 PR2 review point 3). Per-action disallowed fields are
+// rejected by the automation validator.
+func decodeHookSpec(w http.ResponseWriter, r *http.Request) (automation.HookSpec, bool) {
+	dec := json.NewDecoder(r.Body)
+	dec.DisallowUnknownFields()
+	var spec automation.HookSpec
+	if err := dec.Decode(&spec); err != nil {
+		writeError(w, http.StatusBadRequest, "invalid request body: "+err.Error())
+		return automation.HookSpec{}, false
+	}
+	return spec, true
+}
+
+// resolveHookWriter derives the audit creator actor, the accountable human
+// principal (§8), and whether that principal is a workspace owner/admin, plus the
+// agent-admission callback used for fail-closed trigger_agent validation. A
+// member acts under their own authority; an agent must resolve to the human
+// behind its current task. The resolved principal must still be a workspace
+// member (review point 2) — an agent UUID is never a substitute for a human, and
+// a departed principal can no longer author hooks.
+func (h *Handler) resolveHookWriter(w http.ResponseWriter, r *http.Request, userID, workspaceID string) (service.HookAuthor, service.CanInvokeAgent, bool) {
 	actorType, actorID := h.resolveActor(r, userID, workspaceID)
 	actorUUID, err := util.ParseUUID(actorID)
 	if err != nil {
 		writeError(w, http.StatusBadRequest, "invalid actor id")
-		return service.HookAuthor{}, false
+		return service.HookAuthor{}, nil, false
 	}
 	principal := h.invokeOriginatorFromRequest(r, actorType, actorID)
 	if principal == "" {
 		h.writeDispatchBlocked(w, http.StatusForbidden, ReasonInvocationNotAllowed)
-		return service.HookAuthor{}, false
+		return service.HookAuthor{}, nil, false
 	}
 	principalUUID, err := util.ParseUUID(principal)
 	if err != nil {
 		writeError(w, http.StatusForbidden, "no accountable authorization principal")
-		return service.HookAuthor{}, false
+		return service.HookAuthor{}, nil, false
 	}
-	return service.HookAuthor{ActorType: actorType, ActorID: actorUUID, PrincipalUserID: principalUUID}, true
+	// The accountable principal must still be a member of this workspace.
+	member, err := h.getWorkspaceMember(r.Context(), principal, workspaceID)
+	if err != nil {
+		writeError(w, http.StatusForbidden, "authorization principal is not a member of this workspace")
+		return service.HookAuthor{}, nil, false
+	}
+	author := service.HookAuthor{
+		ActorType:        actorType,
+		ActorID:          actorUUID,
+		PrincipalUserID:  principalUUID,
+		IsWorkspaceAdmin: roleAllowed(member.Role, "owner", "admin"),
+	}
+	canInvoke := func(agent db.Agent) bool {
+		return h.canInvokeAgent(r.Context(), agent, actorType, actorID, principal, workspaceID)
+	}
+	return author, canInvoke, true
 }

 func (h *Handler) writeHookError(w http.ResponseWriter, err error) {
@@ -340,6 +389,8 @@ func (h *Handler) writeHookError(w http.ResponseWriter, err error) {
 		writeError(w, http.StatusNotFound, "hook not found")
 	case errors.Is(err, service.ErrHookSystemManaged):
 		writeError(w, http.StatusForbidden, err.Error())
+	case errors.Is(err, service.ErrHookForbidden):
+		writeError(w, http.StatusForbidden, err.Error())
 	case errors.Is(err, service.ErrHookNoPrincipal):
 		writeError(w, http.StatusForbidden, "no accountable authorization principal")
 	default:
--- a/server/internal/handler/hook_test.go
+++ b/server/internal/handler/hook_test.go
@@ -4,14 +4,28 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"sync"
+	"sync/atomic"
 	"testing"

+	"github.com/multica-ai/multica/server/internal/automation"
 	"github.com/multica-ai/multica/server/internal/featureflags"
+	"github.com/multica-ai/multica/server/internal/service"
 	"github.com/multica-ai/multica/server/pkg/featureflag"
 )

+// hookSpecFromMap round-trips a test spec map through JSON into a typed HookSpec,
+// for tests that drive the service layer directly.
+func hookSpecFromMap(m map[string]any) automation.HookSpec {
+	buf, _ := json.Marshal(m)
+	var spec automation.HookSpec
+	json.Unmarshal(buf, &spec)
+	return spec
+}
+
 // enableHooksFlag flips automation_event_hooks on for the shared test handler and
 // restores the previous flag service when the test ends.
 func enableHooksFlag(t *testing.T) {
@@ -23,11 +37,14 @@ func enableHooksFlag(t *testing.T) {
 	t.Cleanup(func() { testHandler.FeatureFlags = prev })
 }

-// newMemberHookRequest builds a member-authenticated request (X-User-ID +
-// X-Workspace-ID), the same identity a member hits the REST API with.
 func newMemberHookRequest(method, path string, body any) *http.Request {
+	return newUserHookRequest(method, path, body, testUserID)
+}
+
+// newUserHookRequest builds a member-authenticated request for the given user.
+func newUserHookRequest(method, path string, body any, userID string) *http.Request {
 	req := newJSONRequest(method, path, body)
-	req.Header.Set("X-User-ID", testUserID)
+	req.Header.Set("X-User-ID", userID)
 	req.Header.Set("X-Workspace-ID", testWorkspaceID)
 	return req
 }
@@ -44,9 +61,74 @@ func newJSONRequest(method, path string, body any) *http.Request {
 	return r
 }

-// A minimal valid per_event spec; action targets are arbitrary uuids (PR2
-// validates uuid shape, not existence — matching/execution is a later slice).
-func sampleHookSpec(name, message string) map[string]any {
+// seedHookIssue inserts a real issue in the test workspace and returns its id.
+func seedHookIssue(t *testing.T) string {
+	t.Helper()
+	var id string
+	if err := testPool.QueryRow(context.Background(), `
+		INSERT INTO issue (workspace_id, title, status, priority, creator_type, creator_id, number)
+		VALUES ($1, 'hook target issue', 'todo', 'medium', 'member', $2,
+		        COALESCE((SELECT MAX(number) FROM issue WHERE workspace_id = $1), 0) + 1)
+		RETURNING id`, testWorkspaceID, testUserID).Scan(&id); err != nil {
+		t.Fatalf("seed issue: %v", err)
+	}
+	t.Cleanup(func() { testPool.Exec(context.Background(), `DELETE FROM issue WHERE id = $1`, id) })
+	return id
+}
+
+// seededHookAgentID returns the workspace-visible agent seeded by the fixture.
+func seededHookAgentID(t *testing.T) string {
+	t.Helper()
+	var id string
+	if err := testPool.QueryRow(context.Background(),
+		`SELECT id FROM agent WHERE workspace_id = $1 AND name = 'Handler Test Agent' LIMIT 1`,
+		testWorkspaceID).Scan(&id); err != nil {
+		t.Fatalf("load seeded agent: %v", err)
+	}
+	return id
+}
+
+// testOwnerMemberID returns the member row id of the fixture owner (testUserID).
+func testOwnerMemberID(t *testing.T) string {
+	t.Helper()
+	var id string
+	if err := testPool.QueryRow(context.Background(),
+		`SELECT id FROM member WHERE workspace_id = $1 AND user_id = $2`,
+		testWorkspaceID, testUserID).Scan(&id); err != nil {
+		t.Fatalf("load owner member: %v", err)
+	}
+	return id
+}
+
+// hookSeedCounter makes each seeded user's email unique across calls.
+var hookSeedCounter atomic.Int64
+
+// seedHookMember inserts a user + workspace member with the given role and
+// returns the user id.
+func seedHookMember(t *testing.T, role string) string {
+	t.Helper()
+	ctx := context.Background()
+	var userID string
+	email := fmt.Sprintf("hook-%s-%d-%d@test.local", role, hookSeedCounter.Add(1), len(t.Name()))
+	if err := testPool.QueryRow(ctx,
+		`INSERT INTO "user" (name, email) VALUES ($1, $2) RETURNING id`,
+		"Hook "+role, email).Scan(&userID); err != nil {
+		t.Fatalf("seed user: %v", err)
+	}
+	if _, err := testPool.Exec(ctx,
+		`INSERT INTO member (workspace_id, user_id, role) VALUES ($1, $2, $3)`,
+		testWorkspaceID, userID, role); err != nil {
+		t.Fatalf("seed member: %v", err)
+	}
+	t.Cleanup(func() {
+		testPool.Exec(context.Background(), `DELETE FROM member WHERE workspace_id = $1 AND user_id = $2`, testWorkspaceID, userID)
+		testPool.Exec(context.Background(), `DELETE FROM "user" WHERE id = $1`, userID)
+	})
+	return userID
+}
+
+// sampleHookSpec is a minimal valid per_event spec: comment an existing issue.
+func sampleHookSpec(name, message, issueID string) map[string]any {
 	return map[string]any{
 		"name": name,
 		"when": map[string]any{
@@ -55,42 +137,45 @@ func sampleHookSpec(name, message string) map[string]any {
 		},
 		"fire": map[string]any{"mode": "per_event"},
 		"do": []any{
-			map[string]any{"type": "add_comment", "issue_id": "55555555-5555-5555-5555-555555555555", "message": message},
+			map[string]any{"type": "add_comment", "issue_id": issueID, "message": message},
 		},
 	}
 }

+func createHookAs(t *testing.T, userID string, spec map[string]any) HookResponse {
+	t.Helper()
+	w := httptest.NewRecorder()
+	testHandler.CreateHook(w, newUserHookRequest(http.MethodPost, "/api/hooks", spec, userID))
+	if w.Code != http.StatusCreated {
+		t.Fatalf("create hook as %s: status %d: %s", userID, w.Code, w.Body.String())
+	}
+	var resp HookResponse
+	if err := json.NewDecoder(w.Body).Decode(&resp); err != nil {
+		t.Fatalf("decode create: %v", err)
+	}
+	t.Cleanup(func() {
+		testPool.Exec(context.Background(), `DELETE FROM hook_revision WHERE hook_id = $1`, resp.ID)
+		testPool.Exec(context.Background(), `DELETE FROM hook WHERE id = $1`, resp.ID)
+	})
+	return resp
+}
+
 func TestHookCRUDLifecycle(t *testing.T) {
 	if testPool == nil {
 		t.Skip("database unavailable")
 	}
 	enableHooksFlag(t)
 	ctx := context.Background()
+	issueID := seedHookIssue(t)

-	// Create → revision #1.
-	w := httptest.NewRecorder()
-	testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("lifecycle hook", "first")))
-	if w.Code != http.StatusCreated {
-		t.Fatalf("create: status %d: %s", w.Code, w.Body.String())
-	}
-	var created HookResponse
-	if err := json.NewDecoder(w.Body).Decode(&created); err != nil {
-		t.Fatalf("decode create: %v", err)
-	}
-	if created.ID == "" || created.Revision.Revision != 1 || !created.Enabled {
+	created := createHookAs(t, testUserID, sampleHookSpec("lifecycle hook", "first", issueID))
+	if created.Revision.Revision != 1 || !created.Enabled || created.Revision.Event != "issue.status_changed" {
 		t.Fatalf("unexpected create response: %+v", created)
 	}
-	if created.Revision.Event != "issue.status_changed" || created.Revision.FireMode != "per_event" {
-		t.Errorf("revision fields wrong: %+v", created.Revision)
-	}
 	hookID := created.ID
-	t.Cleanup(func() {
-		testPool.Exec(context.Background(), `DELETE FROM hook_revision WHERE hook_id = $1`, hookID)
-		testPool.Exec(context.Background(), `DELETE FROM hook WHERE id = $1`, hookID)
-	})

 	// Get.
-	w = httptest.NewRecorder()
+	w := httptest.NewRecorder()
 	testHandler.GetHook(w, withURLParam(newMemberHookRequest(http.MethodGet, "/api/hooks/"+hookID, nil), "id", hookID))
 	if w.Code != http.StatusOK {
 		t.Fatalf("get: status %d: %s", w.Code, w.Body.String())
@@ -98,31 +183,24 @@ func TestHookCRUDLifecycle(t *testing.T) {

 	// Update → new immutable revision #2, active pointer moves, name updated.
 	w = httptest.NewRecorder()
-	testHandler.UpdateHook(w, withURLParam(newMemberHookRequest(http.MethodPatch, "/api/hooks/"+hookID, sampleHookSpec("renamed hook", "second")), "id", hookID))
+	testHandler.UpdateHook(w, withURLParam(newMemberHookRequest(http.MethodPatch, "/api/hooks/"+hookID, sampleHookSpec("renamed hook", "second", issueID)), "id", hookID))
 	if w.Code != http.StatusOK {
 		t.Fatalf("update: status %d: %s", w.Code, w.Body.String())
 	}
 	var updated HookResponse
 	json.NewDecoder(w.Body).Decode(&updated)
-	if updated.Revision.Revision != 2 || updated.Name != "renamed hook" {
-		t.Fatalf("update did not append revision / rename: %+v", updated)
+	if updated.Revision.Revision != 2 || updated.Name != "renamed hook" || updated.Revision.ID == created.Revision.ID {
+		t.Fatalf("update did not append a new revision / rename: %+v", updated)
 	}
-	if updated.Revision.ID == created.Revision.ID {
-		t.Errorf("update must create a NEW revision id, got same %s", updated.Revision.ID)
-	}
-	// The original revision row is retained (immutable history).
 	var revCount int
 	testPool.QueryRow(ctx, `SELECT count(*) FROM hook_revision WHERE hook_id = $1`, hookID).Scan(&revCount)
 	if revCount != 2 {
-		t.Errorf("hook_revision count = %d, want 2 (revisions are immutable, never overwritten)", revCount)
+		t.Errorf("hook_revision count = %d, want 2 (revisions are immutable)", revCount)
 	}

-	// List includes it.
+	// List.
 	w = httptest.NewRecorder()
 	testHandler.ListHooks(w, newMemberHookRequest(http.MethodGet, "/api/hooks", nil))
-	if w.Code != http.StatusOK {
-		t.Fatalf("list: status %d", w.Code)
-	}
 	var list []HookResponse
 	json.NewDecoder(w.Body).Decode(&list)
 	if !containsHook(list, hookID) {
@@ -132,9 +210,6 @@ func TestHookCRUDLifecycle(t *testing.T) {
 	// Disable → enabled=false with reason.
 	w = httptest.NewRecorder()
 	testHandler.DisableHook(w, withURLParam(newMemberHookRequest(http.MethodPost, "/api/hooks/"+hookID+"/disable", map[string]any{"reason": "paused"}), "id", hookID))
-	if w.Code != http.StatusOK {
-		t.Fatalf("disable: status %d: %s", w.Code, w.Body.String())
-	}
 	var disabled HookResponse
 	json.NewDecoder(w.Body).Decode(&disabled)
 	if disabled.Enabled || disabled.DisabledReason != "paused" {
@@ -150,17 +225,12 @@ func TestHookCRUDLifecycle(t *testing.T) {
 		t.Errorf("enable did not take: %+v", reenabled)
 	}

-	// Executions trace is empty (no matcher runs in PR2) but the endpoint works.
+	// Executions trace is empty in store-only PR2 but the endpoint works.
 	w = httptest.NewRecorder()
 	testHandler.ListHookExecutions(w, withURLParam(newMemberHookRequest(http.MethodGet, "/api/hooks/"+hookID+"/executions", nil), "id", hookID))
 	if w.Code != http.StatusOK {
 		t.Fatalf("executions: status %d", w.Code)
 	}
-	var execs []HookExecutionResponse
-	json.NewDecoder(w.Body).Decode(&execs)
-	if len(execs) != 0 {
-		t.Errorf("executions = %d, want 0 in store-only PR2", len(execs))
-	}

 	// Delete (soft archive) → subsequent get 404s.
 	w = httptest.NewRecorder()
@@ -173,15 +243,8 @@ func TestHookCRUDLifecycle(t *testing.T) {
 	if w.Code != http.StatusNotFound {
 		t.Errorf("get after archive: status %d, want 404", w.Code)
 	}
-	// The row is soft-archived, not physically deleted.
-	var archived bool
-	testPool.QueryRow(ctx, `SELECT archived_at IS NOT NULL FROM hook WHERE id = $1`, hookID).Scan(&archived)
-	if !archived {
-		t.Errorf("hook should be soft-archived, not deleted")
-	}
 }

-// The whole surface is invisible unless the feature flag is on.
 func TestHookRequiresFeatureFlag(t *testing.T) {
 	if testPool == nil {
 		t.Skip("database unavailable")
@@ -191,24 +254,211 @@ func TestHookRequiresFeatureFlag(t *testing.T) {
 	t.Cleanup(func() { testHandler.FeatureFlags = prev })

 	w := httptest.NewRecorder()
-	testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("blocked", "x")))
+	testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("blocked", "x", "55555555-5555-5555-5555-555555555555")))
 	if w.Code != http.StatusNotFound {
 		t.Errorf("create with flag off: status %d, want 404", w.Code)
 	}
 }

-// A bad spec is rejected at the API boundary with 400, never reaching the store.
-func TestHookCreateRejectsInvalidSpec(t *testing.T) {
+// Strict schema: unknown fields and per-action disallowed fields and bad status
+// are all rejected at the boundary with 400, never persisted (review point 3).
+func TestHookStrictSchemaRejections(t *testing.T) {
 	if testPool == nil {
 		t.Skip("database unavailable")
 	}
 	enableHooksFlag(t)
-	bad := sampleHookSpec("bad", "x")
-	bad["do"] = []any{map[string]any{"type": "set_issue_status_many"}} // system-only
+	issueID := seedHookIssue(t)
+
+	cases := map[string]func() map[string]any{
+		"unknown top-level field": func() map[string]any {
+			s := sampleHookSpec("x", "hi", issueID)
+			s["unexpected"] = true
+			return s
+		},
+		"disallowed action field": func() map[string]any {
+			s := sampleHookSpec("x", "hi", issueID)
+			s["do"] = []any{map[string]any{"type": "add_comment", "issue_id": issueID, "message": "hi", "agent_id": "66666666-6666-6666-6666-666666666666"}}
+			return s
+		},
+		"invalid status enum": func() map[string]any {
+			s := sampleHookSpec("x", "hi", issueID)
+			s["do"] = []any{map[string]any{"type": "set_issue_status", "issue_id": issueID, "status": "ascended"}}
+			return s
+		},
+		"system-only action": func() map[string]any {
+			s := sampleHookSpec("x", "hi", issueID)
+			s["do"] = []any{map[string]any{"type": "set_issue_status_many"}}
+			return s
+		},
+	}
+	for name, build := range cases {
+		t.Run(name, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", build()))
+			if w.Code != http.StatusBadRequest {
+				t.Errorf("status %d, want 400: %s", w.Code, w.Body.String())
+			}
+		})
+	}
+}
+
+// Fail-closed target validation: a spec that references a target absent from the
+// workspace is rejected with 400, not persisted (review point 2).
+func TestHookFailClosedTargets(t *testing.T) {
+	if testPool == nil {
+		t.Skip("database unavailable")
+	}
+	enableHooksFlag(t)
+	realIssue := seedHookIssue(t)
+	const ghost = "77777777-7777-7777-7777-777777777777"
+
+	action := func(a map[string]any) map[string]any {
+		s := sampleHookSpec("targets", "hi", realIssue)
+		s["do"] = []any{a}
+		return s
+	}
+	cases := map[string]map[string]any{
+		"nonexistent issue":     action(map[string]any{"type": "add_comment", "issue_id": ghost, "message": "hi"}),
+		"nonexistent member":    action(map[string]any{"type": "send_inbox", "member_id": ghost, "message": "hi"}),
+		"nonexistent agent":     action(map[string]any{"type": "trigger_agent", "issue_id": realIssue, "agent_id": ghost}),
+		"nonexistent autopilot": action(map[string]any{"type": "run_autopilot", "autopilot_id": ghost}),
+	}
+	for name, spec := range cases {
+		t.Run(name, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", spec))
+			if w.Code != http.StatusBadRequest {
+				t.Errorf("status %d, want 400 (fail-closed): %s", w.Code, w.Body.String())
+			}
+		})
+	}
+
+	// A real, invokable agent target is accepted.
+	t.Run("real invokable agent accepted", func(t *testing.T) {
+		spec := action(map[string]any{"type": "trigger_agent", "issue_id": realIssue, "agent_id": seededHookAgentID(t)})
+		createHookAs(t, testUserID, spec) // fails the test if not 201
+	})
+}
+
+// Only the hook's principal or a workspace owner/admin may edit it; an arbitrary
+// member cannot rewrite a rule that keeps running under someone else's authority
+// (review point 1).
+func TestHookEditAuthorization(t *testing.T) {
+	if testPool == nil {
+		t.Skip("database unavailable")
+	}
+	enableHooksFlag(t)
+	issueID := seedHookIssue(t)
+
+	author := seedHookMember(t, "member") // principal, non-admin
+	other := seedHookMember(t, "member")  // non-principal, non-admin
+
+	hook := createHookAs(t, author, sampleHookSpec("owned by author", "hi", issueID))
+
+	// A different non-admin member cannot edit / disable / delete it.
+	patch := withURLParam(newUserHookRequest(http.MethodPatch, "/api/hooks/"+hook.ID, sampleHookSpec("hijacked", "x", issueID), other), "id", hook.ID)
 	w := httptest.NewRecorder()
-	testHandler.CreateHook(w, newMemberHookRequest(http.MethodPost, "/api/hooks", bad))
-	if w.Code != http.StatusBadRequest {
-		t.Errorf("create with system-only action: status %d, want 400", w.Code)
+	testHandler.UpdateHook(w, patch)
+	if w.Code != http.StatusForbidden {
+		t.Errorf("other member PATCH: status %d, want 403", w.Code)
+	}
+	w = httptest.NewRecorder()
+	testHandler.DisableHook(w, withURLParam(newUserHookRequest(http.MethodPost, "/api/hooks/"+hook.ID+"/disable", nil, other), "id", hook.ID))
+	if w.Code != http.StatusForbidden {
+		t.Errorf("other member disable: status %d, want 403", w.Code)
+	}
+	w = httptest.NewRecorder()
+	testHandler.DeleteHook(w, withURLParam(newUserHookRequest(http.MethodDelete, "/api/hooks/"+hook.ID, nil, other), "id", hook.ID))
+	if w.Code != http.StatusForbidden {
+		t.Errorf("other member delete: status %d, want 403", w.Code)
+	}
+
+	// The principal can edit their own hook.
+	w = httptest.NewRecorder()
+	testHandler.UpdateHook(w, withURLParam(newUserHookRequest(http.MethodPatch, "/api/hooks/"+hook.ID, sampleHookSpec("by principal", "x", issueID), author), "id", hook.ID))
+	if w.Code != http.StatusOK {
+		t.Errorf("principal PATCH: status %d, want 200: %s", w.Code, w.Body.String())
+	}
+
+	// A workspace owner/admin (the fixture owner) can edit any hook.
+	w = httptest.NewRecorder()
+	testHandler.UpdateHook(w, withURLParam(newMemberHookRequest(http.MethodPatch, "/api/hooks/"+hook.ID, sampleHookSpec("by admin", "x", issueID)), "id", hook.ID))
+	if w.Code != http.StatusOK {
+		t.Errorf("admin PATCH: status %d, want 200: %s", w.Code, w.Body.String())
+	}
+
+	// The principal was NOT transferred by any edit — it is still the author.
+	var principal string
+	testPool.QueryRow(context.Background(),
+		`SELECT authorization_principal_user_id FROM hook WHERE id = $1`, hook.ID).Scan(&principal)
+	if principal != author {
+		t.Errorf("principal = %s, want %s (edits must not transfer the principal)", principal, author)
+	}
+}
+
+// Concurrent PATCHes on the same hook must each append a distinct, contiguous
+// revision without a MAX+1 unique-index collision surfacing as a 500 (review
+// point 4). Driven at the service layer so all workers hit the pool concurrently.
+func TestHookConcurrentPatchAppendsContiguousRevisions(t *testing.T) {
+	if testPool == nil {
+		t.Skip("database unavailable")
+	}
+	enableHooksFlag(t)
+	ctx := context.Background()
+	issueID := seedHookIssue(t)
+	hook := createHookAs(t, testUserID, sampleHookSpec("concurrent", "hi", issueID))
+
+	admin := service.HookAuthor{
+		ActorType:        "member",
+		ActorID:          parseUUID(testUserID),
+		PrincipalUserID:  parseUUID(testUserID),
+		IsWorkspaceAdmin: true,
+	}
+	hookUUID := parseUUID(hook.ID)
+	wsUUID := parseUUID(testWorkspaceID)
+
+	const workers = 8
+	start := make(chan struct{})
+	errs := make(chan error, workers)
+	var wg sync.WaitGroup
+	for i := 0; i < workers; i++ {
+		wg.Add(1)
+		go func(n int) {
+			defer wg.Done()
+			<-start
+			spec := hookSpecFromMap(sampleHookSpec(fmt.Sprintf("rev-%d", n), "x", issueID))
+			_, err := testHandler.HookService.UpdateHook(ctx, wsUUID, hookUUID, spec, admin, nil)
+			errs <- err
+		}(i)
+	}
+	close(start)
+	wg.Wait()
+	close(errs)
+	for err := range errs {
+		if err != nil {
+			t.Fatalf("concurrent PATCH errored (revision race not serialized): %v", err)
+		}
+	}
+
+	// Revisions must be exactly 1..(workers+1), contiguous and unique.
+	rows, err := testPool.Query(ctx, `SELECT revision FROM hook_revision WHERE hook_id = $1 ORDER BY revision`, hook.ID)
+	if err != nil {
+		t.Fatalf("query revisions: %v", err)
+	}
+	defer rows.Close()
+	var revs []int
+	for rows.Next() {
+		var r int
+		rows.Scan(&r)
+		revs = append(revs, r)
+	}
+	if len(revs) != workers+1 {
+		t.Fatalf("revision count = %d, want %d", len(revs), workers+1)
+	}
+	for i, r := range revs {
+		if r != i+1 {
+			t.Fatalf("revisions not contiguous at index %d: got %d, want %d (revs=%v)", i, r, i+1, revs)
+		}
 	}
 }

@@ -218,7 +468,8 @@ func TestHookAgentRequiresPrincipal(t *testing.T) {
 		t.Skip("database unavailable")
 	}
 	enableHooksFlag(t)
-	req := newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("agent hook", "x"))
+	issueID := seedHookIssue(t)
+	req := newMemberHookRequest(http.MethodPost, "/api/hooks", sampleHookSpec("agent hook", "x", issueID))
 	// Trusted agent identity (task_token), valid agent uuid, but no X-Task-ID means
 	// no originator can be resolved → no accountable principal.
 	req.Header.Set("X-Actor-Source", "task_token")
--- a/server/internal/service/hook.go
+++ b/server/internal/service/hook.go
@@ -15,22 +15,32 @@ import (
 )

 // Hook CRUD errors surfaced to the handler for status mapping. Validation
-// problems flow through as automation.ValidationError (→ 400).
+// problems (shape or unresolvable target) flow through as
+// *automation.ValidationError (→ 400).
 var (
 	ErrHookNotFound      = errors.New("hook not found")
 	ErrHookSystemManaged = errors.New("system-managed hooks cannot be modified through this API")
 	ErrHookNoPrincipal   = errors.New("no accountable authorization principal for this hook")
+	ErrHookForbidden     = errors.New("only the hook's principal or a workspace admin may modify it")
 )

-// HookAuthor carries the resolved identity for a create/update: who is acting
-// (creator, pure audit) and the accountable human whose authority the hook runs
-// under (§8). An agent author must resolve to a real member principal.
+// HookAuthor carries the resolved identity for a hook write: who is acting
+// (creator, pure audit), the accountable human whose authority the hook runs
+// under (§8), and whether that human is a workspace owner/admin. An agent author
+// must resolve to a real member principal.
 type HookAuthor struct {
-	ActorType       string // member | agent
-	ActorID         pgtype.UUID
-	PrincipalUserID pgtype.UUID
+	ActorType        string // member | agent
+	ActorID          pgtype.UUID
+	PrincipalUserID  pgtype.UUID
+	IsWorkspaceAdmin bool
 }

+// CanInvokeAgent is the admission predicate the handler supplies (wrapping
+// Handler.canInvokeAgent) so the service can fail-closed on a trigger_agent
+// target without importing request context. A nil predicate denies every
+// trigger_agent target (fail-closed).
+type CanInvokeAgent func(agent db.Agent) bool
+
 // HookWithRevision pairs a hook row with its active revision so the handler can
 // render one complete view. The service returns db rows; the handler shapes JSON.
 type HookWithRevision struct {
@@ -52,10 +62,11 @@ func NewHookService(q *db.Queries, tx TxStarter) *HookService {
 	return &HookService{Queries: q, TxStarter: tx}
 }

-// CreateHook validates the spec, resolves scope + principal, and inserts the
-// hook together with revision #1 in one transaction. The two rows reference
-// each other, so both ids are generated up front.
-func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, spec automation.HookSpec, author HookAuthor) (HookWithRevision, error) {
+// CreateHook validates the spec (shape + workspace-scoped targets), resolves
+// scope + principal, and inserts the hook together with revision #1 in one
+// transaction. The two rows reference each other, so both ids are generated up
+// front.
+func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, spec automation.HookSpec, author HookAuthor, canInvoke CanInvokeAgent) (HookWithRevision, error) {
 	if err := automation.Validate(spec); err != nil {
 		return HookWithRevision{}, err
 	}
@@ -76,6 +87,9 @@ func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, s

 	var out HookWithRevision
 	err = s.inTx(ctx, func(qtx *db.Queries) error {
+		if err := validateTargets(ctx, qtx, workspaceID, spec, canInvoke); err != nil {
+			return err
+		}
 		hook, err := qtx.CreateHook(ctx, db.CreateHookParams{
 			ID:                           hookID,
 			WorkspaceID:                  workspaceID,
@@ -117,36 +131,41 @@ func (s *HookService) CreateHook(ctx context.Context, workspaceID pgtype.UUID, s
 }

 // UpdateHook appends a new immutable revision from the spec and repoints the
-// hook's active revision (§5.1). Existing executions stay pinned to their
-// original revision. System-managed hooks are not editable here.
-func (s *HookService) UpdateHook(ctx context.Context, workspaceID, hookID pgtype.UUID, spec automation.HookSpec, author HookAuthor) (HookWithRevision, error) {
+// hook's active revision (§5.1). It locks the hook row first so concurrent
+// PATCHes serialize and MAX(revision)+1 can never collide (review point 4), and
+// re-checks archived/origin/authorization inside the lock. Only the hook's
+// principal or a workspace admin may edit (review point 1). Scope is immutable.
+func (s *HookService) UpdateHook(ctx context.Context, workspaceID, hookID pgtype.UUID, spec automation.HookSpec, author HookAuthor, canInvoke CanInvokeAgent) (HookWithRevision, error) {
 	if err := automation.Validate(spec); err != nil {
 		return HookWithRevision{}, err
 	}
-	// Scope is immutable after creation; UpdateHook only replaces the revision
-	// config and the display name.
 	match, conditions, actions, err := marshalRevisionConfig(spec)
 	if err != nil {
 		return HookWithRevision{}, err
 	}

-	existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID})
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return HookWithRevision{}, ErrHookNotFound
-		}
-		return HookWithRevision{}, err
-	}
-	if existing.ArchivedAt.Valid {
-		return HookWithRevision{}, ErrHookNotFound
-	}
-	if existing.Origin == "system" {
-		return HookWithRevision{}, ErrHookSystemManaged
-	}
-
 	revisionID := util.NewUUID()
 	var out HookWithRevision
 	err = s.inTx(ctx, func(qtx *db.Queries) error {
+		existing, err := qtx.GetHookForUpdate(ctx, db.GetHookForUpdateParams{ID: hookID, WorkspaceID: workspaceID})
+		if err != nil {
+			if errors.Is(err, pgx.ErrNoRows) {
+				return ErrHookNotFound
+			}
+			return err
+		}
+		if existing.ArchivedAt.Valid {
+			return ErrHookNotFound
+		}
+		if existing.Origin == "system" {
+			return ErrHookSystemManaged
+		}
+		if err := authorizeHookEdit(existing, author); err != nil {
+			return err
+		}
+		if err := validateTargets(ctx, qtx, workspaceID, spec, canInvoke); err != nil {
+			return err
+		}
 		maxRev, err := qtx.GetMaxHookRevision(ctx, hookID)
 		if err != nil {
 			return err
@@ -223,23 +242,12 @@ func (s *HookService) ListHooks(ctx context.Context, workspaceID pgtype.UUID) ([
 }

 // SetEnabled enables/disables a hook. Disable only blocks future matches; it
-// does not cancel queued/running executions (§5.1).
-func (s *HookService) SetEnabled(ctx context.Context, workspaceID, hookID pgtype.UUID, enabled bool, reason string) (HookWithRevision, error) {
-	existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID})
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return HookWithRevision{}, ErrHookNotFound
-		}
+// does not cancel queued/running executions (§5.1). Only the principal or a
+// workspace admin may toggle it.
+func (s *HookService) SetEnabled(ctx context.Context, workspaceID, hookID pgtype.UUID, enabled bool, reason string, author HookAuthor) (HookWithRevision, error) {
+	if _, err := s.loadEditableHook(ctx, workspaceID, hookID, author); err != nil {
 		return HookWithRevision{}, err
 	}
-	if existing.ArchivedAt.Valid {
-		return HookWithRevision{}, ErrHookNotFound
-	}
-	if existing.Origin == "system" {
-		// Enabling/disabling a system hook is an admin-only lifecycle op reserved
-		// for the PR5 system-hook management path, not this user CRUD API.
-		return HookWithRevision{}, ErrHookSystemManaged
-	}
 	disabledReason := pgtype.Text{}
 	if !enabled && reason != "" {
 		disabledReason = pgtype.Text{String: reason, Valid: true}
@@ -264,20 +272,11 @@ func (s *HookService) SetEnabled(ctx context.Context, workspaceID, hookID pgtype
 }

 // ArchiveHook soft-deletes a hook (§5.1); revisions/executions/effects are kept.
-func (s *HookService) ArchiveHook(ctx context.Context, workspaceID, hookID pgtype.UUID) error {
-	existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID})
-	if err != nil {
-		if errors.Is(err, pgx.ErrNoRows) {
-			return ErrHookNotFound
-		}
+// Only the principal or a workspace admin may archive it.
+func (s *HookService) ArchiveHook(ctx context.Context, workspaceID, hookID pgtype.UUID, author HookAuthor) error {
+	if _, err := s.loadEditableHook(ctx, workspaceID, hookID, author); err != nil {
 		return err
 	}
-	if existing.ArchivedAt.Valid {
-		return ErrHookNotFound
-	}
-	if existing.Origin == "system" {
-		return ErrHookSystemManaged
-	}
 	if _, err := s.Queries.ArchiveHook(ctx, db.ArchiveHookParams{ID: hookID, WorkspaceID: workspaceID}); err != nil {
 		if errors.Is(err, pgx.ErrNoRows) {
 			return ErrHookNotFound
@@ -299,6 +298,47 @@ func (s *HookService) ListExecutions(ctx context.Context, workspaceID, hookID pg
 	return s.Queries.ListHookExecutionsByHook(ctx, db.ListHookExecutionsByHookParams{HookID: hookID, Limit: limit})
 }

+// loadEditableHook loads a non-archived, non-system hook and enforces the edit
+// authorization gate, for the enable/disable/archive paths.
+func (s *HookService) loadEditableHook(ctx context.Context, workspaceID, hookID pgtype.UUID, author HookAuthor) (db.Hook, error) {
+	existing, err := s.Queries.GetHookInWorkspace(ctx, db.GetHookInWorkspaceParams{ID: hookID, WorkspaceID: workspaceID})
+	if err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return db.Hook{}, ErrHookNotFound
+		}
+		return db.Hook{}, err
+	}
+	if existing.ArchivedAt.Valid {
+		return db.Hook{}, ErrHookNotFound
+	}
+	if existing.Origin == "system" {
+		return db.Hook{}, ErrHookSystemManaged
+	}
+	if err := authorizeHookEdit(existing, author); err != nil {
+		return db.Hook{}, err
+	}
+	return existing, nil
+}
+
+// authorizeHookEdit implements the edit gate (review point 1): a workspace
+// owner/admin may edit any hook; otherwise only the hook's original
+// authorization principal may. The principal is NOT transferred on edit, so an
+// arbitrary member can never rewrite a rule that keeps running under someone
+// else's authority.
+func authorizeHookEdit(hook db.Hook, author HookAuthor) error {
+	if author.IsWorkspaceAdmin {
+		return nil
+	}
+	if principalMatches(hook.AuthorizationPrincipalUserID, author.PrincipalUserID) {
+		return nil
+	}
+	return ErrHookForbidden
+}
+
+func principalMatches(a, b pgtype.UUID) bool {
+	return a.Valid && b.Valid && a.Bytes == b.Bytes
+}
+
 func (s *HookService) inTx(ctx context.Context, fn func(qtx *db.Queries) error) error {
 	tx, err := s.TxStarter.Begin(ctx)
 	if err != nil {
@@ -342,3 +382,120 @@ func marshalRevisionConfig(spec automation.HookSpec) (match, conditions, actions
 	}
 	return match, conditions, actions, nil
 }
+
+// validateTargets fail-closed checks that every id the spec references exists in
+// the hook's workspace under the current principal (review point 2). §13 requires
+// this at create/update time so an illegal configuration never enters the store
+// and never reaches a worker. Uses the tx-bound queries so the checks share the
+// write transaction.
+func validateTargets(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, spec automation.HookSpec, canInvoke CanInvokeAgent) error {
+	if spec.Scope != nil && spec.Scope.Type == automation.ScopeIssue {
+		if err := requireIssue(ctx, qtx, workspaceID, spec.Scope.ID, "scope.id"); err != nil {
+			return err
+		}
+	}
+	for i, cond := range spec.If {
+		if cond.IssuesStatus != nil {
+			for _, id := range cond.IssuesStatus.IDs {
+				if err := requireIssue(ctx, qtx, workspaceID, id, fmt.Sprintf("if[%d].issues_status.ids", i)); err != nil {
+					return err
+				}
+			}
+		}
+		if cond.IssueField != nil {
+			if err := requireIssue(ctx, qtx, workspaceID, cond.IssueField.ID, fmt.Sprintf("if[%d].issue_field.id", i)); err != nil {
+				return err
+			}
+		}
+	}
+	for i, action := range spec.Do {
+		if err := validateActionTargets(ctx, qtx, workspaceID, i, action, canInvoke); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func validateActionTargets(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, i int, a automation.ActionSpec, canInvoke CanInvokeAgent) error {
+	where := fmt.Sprintf("do[%d].%s", i, a.Type)
+	switch a.Type {
+	case automation.ActionSetIssueStatus, automation.ActionAddComment:
+		return requireIssue(ctx, qtx, workspaceID, a.IssueID, where+".issue_id")
+	case automation.ActionTriggerAgent:
+		if err := requireIssue(ctx, qtx, workspaceID, a.IssueID, where+".issue_id"); err != nil {
+			return err
+		}
+		return requireInvokableAgent(ctx, qtx, workspaceID, a.AgentID, where+".agent_id", canInvoke)
+	case automation.ActionSendInbox:
+		return requireMember(ctx, qtx, workspaceID, a.MemberID, where+".member_id")
+	case automation.ActionRunAutopilot:
+		return requireAutopilot(ctx, qtx, workspaceID, a.AutopilotID, where+".autopilot_id")
+	}
+	return nil
+}
+
+func requireIssue(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string) error {
+	uid, err := util.ParseUUID(id)
+	if err != nil {
+		return automation.NewValidationError("%s must be a uuid", field)
+	}
+	if _, err := qtx.GetIssueInWorkspace(ctx, db.GetIssueInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}); err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return automation.NewValidationError("%s references issue %s which does not exist in this workspace", field, id)
+		}
+		return err
+	}
+	return nil
+}
+
+func requireMember(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string) error {
+	uid, err := util.ParseUUID(id)
+	if err != nil {
+		return automation.NewValidationError("%s must be a uuid", field)
+	}
+	if _, err := qtx.GetMemberInWorkspace(ctx, db.GetMemberInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}); err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return automation.NewValidationError("%s references member %s which is not in this workspace", field, id)
+		}
+		return err
+	}
+	return nil
+}
+
+func requireAutopilot(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string) error {
+	uid, err := util.ParseUUID(id)
+	if err != nil {
+		return automation.NewValidationError("%s must be a uuid", field)
+	}
+	if _, err := qtx.GetAutopilotInWorkspace(ctx, db.GetAutopilotInWorkspaceParams{ID: uid, WorkspaceID: workspaceID}); err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return automation.NewValidationError("%s references autopilot %s which does not exist in this workspace", field, id)
+		}
+		return err
+	}
+	return nil
+}
+
+// requireInvokableAgent confirms the agent exists in the workspace, is not
+// archived, has a runtime, and is invokable by the current principal — the same
+// admission the interactive trigger path enforces, applied fail-closed at save.
+func requireInvokableAgent(ctx context.Context, qtx *db.Queries, workspaceID pgtype.UUID, id, field string, canInvoke CanInvokeAgent) error {
+	uid, err := util.ParseUUID(id)
+	if err != nil {
+		return automation.NewValidationError("%s must be a uuid", field)
+	}
+	agent, err := qtx.GetAgentInWorkspace(ctx, db.GetAgentInWorkspaceParams{ID: uid, WorkspaceID: workspaceID})
+	if err != nil {
+		if errors.Is(err, pgx.ErrNoRows) {
+			return automation.NewValidationError("%s references agent %s which does not exist in this workspace", field, id)
+		}
+		return err
+	}
+	if agent.ArchivedAt.Valid || !agent.RuntimeID.Valid {
+		return automation.NewValidationError("%s references agent %s which is archived or has no runtime", field, id)
+	}
+	if canInvoke == nil || !canInvoke(agent) {
+		return automation.NewValidationError("%s references agent %s which the hook's principal may not invoke", field, id)
+	}
+	return nil
+}
--- a/server/pkg/db/generated/hook.sql.go
+++ b/server/pkg/db/generated/hook.sql.go
@@ -179,6 +179,47 @@ func (q *Queries) CreateHookRevision(ctx context.Context, arg CreateHookRevision
 	return i, err
 }

+const getHookForUpdate = `-- name: GetHookForUpdate :one
+SELECT id, workspace_id, name, enabled, active_revision_id, scope_type, scope_id, retire_after_event_seq, origin, system_key, system_version, creator_actor_type, creator_actor_id, authorization_principal_user_id, disabled_reason, created_at, updated_at, archived_at FROM hook
+WHERE id = $1 AND workspace_id = $2
+FOR UPDATE
+`
+
+type GetHookForUpdateParams struct {
+	ID          pgtype.UUID `json:"id"`
+	WorkspaceID pgtype.UUID `json:"workspace_id"`
+}
+
+// Row-locking load used by PATCH so concurrent edits to the same hook serialize:
+// the lock holder allocates the next revision and repoints the active pointer
+// before the next waiter reads MAX(revision), so idx_hook_revision_unique can
+// never be violated by a MAX+1 race (MUL-4332 PR2 review point 4).
+func (q *Queries) GetHookForUpdate(ctx context.Context, arg GetHookForUpdateParams) (Hook, error) {
+	row := q.db.QueryRow(ctx, getHookForUpdate, arg.ID, arg.WorkspaceID)
+	var i Hook
+	err := row.Scan(
+		&i.ID,
+		&i.WorkspaceID,
+		&i.Name,
+		&i.Enabled,
+		&i.ActiveRevisionID,
+		&i.ScopeType,
+		&i.ScopeID,
+		&i.RetireAfterEventSeq,
+		&i.Origin,
+		&i.SystemKey,
+		&i.SystemVersion,
+		&i.CreatorActorType,
+		&i.CreatorActorID,
+		&i.AuthorizationPrincipalUserID,
+		&i.DisabledReason,
+		&i.CreatedAt,
+		&i.UpdatedAt,
+		&i.ArchivedAt,
+	)
+	return i, err
+}
+
 const getHookInWorkspace = `-- name: GetHookInWorkspace :one
 SELECT id, workspace_id, name, enabled, active_revision_id, scope_type, scope_id, retire_after_event_seq, origin, system_key, system_version, creator_actor_type, creator_actor_id, authorization_principal_user_id, disabled_reason, created_at, updated_at, archived_at FROM hook
 WHERE id = $1 AND workspace_id = $2
--- a/server/pkg/db/generated/member.sql.go
+++ b/server/pkg/db/generated/member.sql.go
@@ -86,6 +86,32 @@ func (q *Queries) GetMemberByUserAndWorkspace(ctx context.Context, arg GetMember
 	return i, err
 }

+const getMemberInWorkspace = `-- name: GetMemberInWorkspace :one
+SELECT id, workspace_id, user_id, role, created_at FROM member
+WHERE id = $1 AND workspace_id = $2
+`
+
+type GetMemberInWorkspaceParams struct {
+	ID          pgtype.UUID `json:"id"`
+	WorkspaceID pgtype.UUID `json:"workspace_id"`
+}
+
+// Workspace-scoped member existence check by member row id. Used to fail-closed
+// validate a send_inbox action target belongs to the hook's workspace
+// (MUL-4332 PR2 review point 2).
+func (q *Queries) GetMemberInWorkspace(ctx context.Context, arg GetMemberInWorkspaceParams) (Member, error) {
+	row := q.db.QueryRow(ctx, getMemberInWorkspace, arg.ID, arg.WorkspaceID)
+	var i Member
+	err := row.Scan(
+		&i.ID,
+		&i.WorkspaceID,
+		&i.UserID,
+		&i.Role,
+		&i.CreatedAt,
+	)
+	return i, err
+}
+
 const listMembers = `-- name: ListMembers :many
 SELECT id, workspace_id, user_id, role, created_at FROM member
 WHERE workspace_id = $1
--- a/server/pkg/db/queries/hook.sql
+++ b/server/pkg/db/queries/hook.sql
@@ -21,6 +21,15 @@ RETURNING *;
 SELECT * FROM hook
 WHERE id = $1 AND workspace_id = $2;

+-- name: GetHookForUpdate :one
+-- Row-locking load used by PATCH so concurrent edits to the same hook serialize:
+-- the lock holder allocates the next revision and repoints the active pointer
+-- before the next waiter reads MAX(revision), so idx_hook_revision_unique can
+-- never be violated by a MAX+1 race (MUL-4332 PR2 review point 4).
+SELECT * FROM hook
+WHERE id = $1 AND workspace_id = $2
+FOR UPDATE;
+
 -- name: ListHooksByWorkspace :many
 SELECT * FROM hook
 WHERE workspace_id = $1 AND archived_at IS NULL
--- a/server/pkg/db/queries/member.sql
+++ b/server/pkg/db/queries/member.sql
@@ -7,6 +7,13 @@ ORDER BY created_at ASC;
 SELECT * FROM member
 WHERE id = $1;

+-- name: GetMemberInWorkspace :one
+-- Workspace-scoped member existence check by member row id. Used to fail-closed
+-- validate a send_inbox action target belongs to the hook's workspace
+-- (MUL-4332 PR2 review point 2).
+SELECT * FROM member
+WHERE id = $1 AND workspace_id = $2;
+
 -- name: GetMemberByUserAndWorkspace :one
 SELECT * FROM member
 WHERE user_id = $1 AND workspace_id = $2;