diff --git a/cmd/mavend/actionresolve.go b/cmd/mavend/actionresolve.go index b0e012f..9dfbcc3 100644 --- a/cmd/mavend/actionresolve.go +++ b/cmd/mavend/actionresolve.go @@ -54,22 +54,23 @@ func noteActionResolution(ctx context.Context, source, fn string, resolved bool) } // noteActionValidation records the structural validation outcome in the -// decision trace. Three outcomes: unresolved (matcher miss), valid -// (structurally admissible), or invalid (structurally malformed). +// decision trace. Five outcomes: unresolved (matcher miss), valid +// (structurally admissible), invalid_argument, missing_argument, or +// ambiguous_target (structurally malformed). func noteActionValidation(ctx context.Context, v router.ActionValidationResult) { rec := decision.From(ctx) if rec == nil { return } - switch { - case v.Unresolved: + switch v.Status { + case router.ActionUnresolved: rec.Note(decision.Claim{ Stage: decision.StageAction, Claimant: "action-validation", Outcome: decision.Declined, Reason: "unresolved", }) - case v.Valid: + case router.ActionValid: rec.Note(decision.Claim{ Stage: decision.StageAction, Claimant: "action-validation", @@ -77,9 +78,9 @@ func noteActionValidation(ctx context.Context, v router.ActionValidationResult) Reason: "valid", }) default: - reason := "invalid" + reason := string(v.Status) if len(v.Issues) > 0 { - reason = "invalid:" + v.Issues[0].Reason + reason = string(v.Status) + ":" + v.Issues[0].Reason } rec.Note(decision.Claim{ Stage: decision.StageAction, diff --git a/cmd/mavend/actionresolve_test.go b/cmd/mavend/actionresolve_test.go index 40f0728..50b377f 100644 --- a/cmd/mavend/actionresolve_test.go +++ b/cmd/mavend/actionresolve_test.go @@ -385,3 +385,172 @@ func TestActExecutionFromCandidateNotSlots(t *testing.T) { t.Errorf("execution from candidate replied %q; want tool success", reply) } } + +// --- validation status boundary tests --- + +// TestActValidation_StatusValidRoute pins that a route-resolved valid action +// produces ActionValid status and reaches execution. +func TestActValidation_StatusValidRoute(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "status", []string{"true"}, false, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "status", + Slots: router.Slots{Fn: "status", HasFn: true}, + }) + if !strings.Contains(reply, "готово") { + t.Errorf("valid route act replied %q; want tool success", reply) + } +} + +// TestActValidation_StatusValidMatcher pins that a matcher-resolved valid +// action produces ActionValid status and reaches execution. +func TestActValidation_StatusValidMatcher(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "status", []string{"true"}, false, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "check status", + Slots: router.Slots{Text: "status"}, + }) + if !strings.Contains(reply, "готово") { + t.Errorf("valid matcher act replied %q; want tool success", reply) + } +} + +// TestActValidation_StatusUnresolved pins that an unresolved candidate +// produces ActionUnresolved status and flows to proposeGap. +func TestActValidation_StatusUnresolved(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "status", []string{"true"}, false, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "deploy everything", + Slots: router.Slots{Text: "deploy everything"}, + }) + if !strings.Contains(strings.ToLower(reply), "предлож") { + t.Errorf("unresolved act replied %q; want propose-gap", reply) + } +} + +// TestActValidation_StatusInvalid pins that a structurally invalid candidate +// produces ActionInvalidArgument status and refuses execution. +func TestActValidation_StatusInvalid(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "status", []string{"true"}, false, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "status", + Slots: router.Slots{Fn: " ", HasFn: true}, + }) + if reply == "" { + t.Error("expected a response for invalid candidate") + } + if strings.Contains(reply, "готово") { + t.Error("invalid candidate should not reach tool execution") + } +} + +// TestActValidation_DestructiveValidStatus pins that a destructive but +// structurally valid action still produces ActionValid status and reaches +// the confirmation path (not validation failure). +func TestActValidation_DestructiveValidStatus(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "restart", []string{"true"}, true, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "restart", + Slots: router.Slots{Fn: "restart", HasFn: true}, + }) + if !strings.Contains(reply, "да или нет") { + t.Errorf("destructive valid act replied %q; want confirm turn", reply) + } +} + +// TestActValidation_ConfirmationUnchanged pins that the confirmation flow +// is unchanged by validation. +func TestActValidation_ConfirmationUnchanged(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "restart", []string{"echo", "ok"}, true, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "restart nginx", + Slots: router.Slots{Fn: "restart", Args: []string{"nginx"}, HasFn: true}, + }) + if !strings.Contains(reply, "да или нет") { + t.Errorf("confirmation act replied %q; want confirm turn", reply) + } +} + +// TestActValidation_TaskStatusInterceptUnchanged pins that task_status +// interception is unchanged by validation. +func TestActValidation_TaskStatusInterceptUnchanged(t *testing.T) { + h, _ := newActHandler(t) + ctx := context.Background() + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "task status", + Slots: router.Slots{Fn: router.TaskStatusFn, HasFn: true, Text: "task status"}, + }) + if strings.Contains(reply, "готово") { + t.Errorf("task_status was not intercepted, got %q", reply) + } +} + +// TestActValidation_NoExecutionOnFailure pins that validation failure +// prevents downstream execution. +func TestActValidation_NoExecutionOnFailure(t *testing.T) { + h, st := newActHandler(t) + ctx := context.Background() + now := h.now() + + if err := st.EnableTool(ctx, "status", []string{"true"}, false, "test", now); err != nil { + t.Fatal(err) + } + + reply := h.actionAct(ctx, router.Decision{ + Intent: router.IntentAct, + Utterance: "status", + Slots: router.Slots{Fn: " ", HasFn: true}, + }) + if strings.Contains(reply, "готово") { + t.Error("validation failure should not reach tool execution") + } +} diff --git a/cmd/mavend/actions_act.go b/cmd/mavend/actions_act.go index 4c8e1fa..f1ba222 100644 --- a/cmd/mavend/actions_act.go +++ b/cmd/mavend/actions_act.go @@ -39,7 +39,7 @@ func (h *reactiveHandler) actionAct(ctx context.Context, dec router.Decision) st validation := router.ValidateActionCandidate(candidate) noteActionValidation(ctx, validation) - if !validation.Unresolved && !validation.Valid { + if !validation.Unresolved() && !validation.Valid() { // Resolved but structurally malformed: refuse execution. return phraser.A(phraser.ActFail, nil) } diff --git a/internal/router/actioncandidate.go b/internal/router/actioncandidate.go index 3cf9663..b34a54d 100644 --- a/internal/router/actioncandidate.go +++ b/internal/router/actioncandidate.go @@ -58,6 +58,38 @@ const ( FieldArgs ActionField = "args" ) +// ActionValidationStatus is the typed classification of a validation outcome. +// Five disjoint values: exactly one is set on every ActionValidationResult. +type ActionValidationStatus string + +const ( + // ActionValid — Fn is non-empty and the candidate is structurally + // admissible. The caller may proceed to risk policy and execution. + ActionValid ActionValidationStatus = "valid" + + // ActionUnresolved — Fn is empty. The matcher did not match, or the + // decision was not an act. The caller routes to proposeGap / + // clarification. This is NOT a structural error. + ActionUnresolved ActionValidationStatus = "unresolved" + + // ActionMissingArgument — Fn is present but required arguments are + // absent. Currently unused: no tool declares required args at the + // candidate level. Preserved for future use without semantic change. + ActionMissingArgument ActionValidationStatus = "missing_argument" + + // ActionInvalidArgument — Fn is present and arguments are present but + // structurally malformed (e.g. wrong shape for a known tool). Currently + // unused: argument shape is validated by the tool executor, not the + // candidate validator. Preserved for future use. + ActionInvalidArgument ActionValidationStatus = "invalid_argument" + + // ActionAmbiguousTarget — Fn is present but the entity/target reference + // is ambiguous (multiple candidates, or a demonstrative with no + // referent). Currently handled downstream by ecosystem handlers, not + // the candidate validator. Preserved for future centralization. + ActionAmbiguousTarget ActionValidationStatus = "ambiguous_target" +) + // ActionValidationIssue records one structural problem found by // ValidateActionCandidate. Field names which field; Reason is a short // machine-readable tag, not a human sentence. @@ -67,20 +99,26 @@ type ActionValidationIssue struct { } // ActionValidationResult is the typed output of ValidateActionCandidate. -// Three disjoint outcomes: +// Exactly one of the five Status values is set. Issues carries detail for +// invalid statuses; it is nil for valid and unresolved. // -// - Unresolved: Fn is empty — the matcher did not match. The caller should -// route to proposeGap / clarification, not treat this as a structural error. -// - Valid: Fn is non-empty and the candidate is structurally admissible for -// the current execution path. -// - Invalid: Fn is non-empty but the candidate is structurally malformed. -// The caller should refuse execution. +// The result answers only shape/completeness — not trust, confidence, risk, +// or semantic correctness. Risk policy, confirmation, voice authority, and +// irreversible-action policy remain downstream and unchanged. type ActionValidationResult struct { - Unresolved bool - Valid bool - Issues []ActionValidationIssue + Status ActionValidationStatus + Issues []ActionValidationIssue + Missing []string `json:"Missing,omitempty"` } +// Unresolved reports whether the candidate was not resolved (Fn empty). +// Kept as a method for backward compatibility with existing call sites. +func (r ActionValidationResult) Unresolved() bool { return r.Status == ActionUnresolved } + +// Valid reports whether the candidate is structurally admissible. +// Kept as a method for backward compatibility with existing call sites. +func (r ActionValidationResult) Valid() bool { return r.Status == ActionValid } + // ResolveActionCandidate produces an ActionCandidate from a routing decision. // It is the single boundary between routing and action resolution: everything // downstream consumes the candidate rather than re-resolving the function. @@ -135,12 +173,17 @@ func ResolveActionCandidate(dec Decision, m ActMatcher) ActionCandidate { // structurally admissible for the current execution path. It answers only // shape/completeness — not trust, confidence, risk, or semantic correctness. // -// Three outcomes: +// Five outcomes (exactly one): // - Unresolved (Fn empty): the matcher missed. Caller routes to // proposeGap / clarification. This is NOT a validation error. // - Valid (Fn non-empty, structure sound): candidate may proceed to // risk policy and execution. -// - Invalid (Fn non-empty but malformed): candidate must not execute. +// - InvalidArgument (Fn present but args malformed): candidate must not +// execute. Currently unused; preserved for future use. +// - MissingArgument (Fn present but required args absent): candidate must +// not execute. Currently unused; preserved for future use. +// - AmbiguousTarget (Fn present but target ambiguous): candidate must not +// execute. Currently handled downstream; preserved for future use. // // Validation does not re-run routing, intent classification, matcher // resolution, language parsing, or entity inference. @@ -149,7 +192,7 @@ func ValidateActionCandidate(c ActionCandidate) ActionValidationResult { // was not an act. This flows into the existing propose-gap / proposal // path and must NOT be treated as a structural error. if c.Fn == "" { - return ActionValidationResult{Unresolved: true} + return ActionValidationResult{Status: ActionUnresolved} } var issues []ActionValidationIssue @@ -165,7 +208,7 @@ func ValidateActionCandidate(c ActionCandidate) ActionValidationResult { } if len(issues) > 0 { - return ActionValidationResult{Issues: issues} + return ActionValidationResult{Status: ActionInvalidArgument, Issues: issues} } - return ActionValidationResult{Valid: true} + return ActionValidationResult{Status: ActionValid} } diff --git a/internal/router/actioncandidate_test.go b/internal/router/actioncandidate_test.go index 845088e..3847312 100644 --- a/internal/router/actioncandidate_test.go +++ b/internal/router/actioncandidate_test.go @@ -157,10 +157,10 @@ func TestResolveActionCandidate_AliasMatch(t *testing.T) { func TestValidateActionCandidate_UnresolvedEmptyFn(t *testing.T) { c := ActionCandidate{} v := ValidateActionCandidate(c) - if !v.Unresolved { - t.Error("expected unresolved for empty Fn") + if v.Status != ActionUnresolved { + t.Errorf("Status = %q, want unresolved", v.Status) } - if v.Valid { + if v.Valid() { t.Error("unresolved must not be valid") } } @@ -173,8 +173,8 @@ func TestValidateActionCandidate_UnresolvedMatcherMiss(t *testing.T) { Confidence: 0.5, } v := ValidateActionCandidate(c) - if !v.Unresolved { - t.Error("expected unresolved for matcher miss") + if v.Status != ActionUnresolved { + t.Errorf("Status = %q, want unresolved", v.Status) } } @@ -187,11 +187,11 @@ func TestValidateActionCandidate_ValidRoute(t *testing.T) { Source: ActionSourceRoute, } v := ValidateActionCandidate(c) - if v.Unresolved { + if v.Unresolved() { t.Error("expected resolved, not unresolved") } - if !v.Valid { - t.Errorf("expected valid, got issues: %v", v.Issues) + if v.Status != ActionValid { + t.Errorf("Status = %q, want valid; issues: %v", v.Status, v.Issues) } } @@ -203,11 +203,11 @@ func TestValidateActionCandidate_ValidMatcher(t *testing.T) { Source: ActionSourceMatcher, } v := ValidateActionCandidate(c) - if v.Unresolved { + if v.Unresolved() { t.Error("expected resolved, not unresolved") } - if !v.Valid { - t.Errorf("expected valid, got issues: %v", v.Issues) + if v.Status != ActionValid { + t.Errorf("Status = %q, want valid; issues: %v", v.Status, v.Issues) } } @@ -218,11 +218,11 @@ func TestValidateActionCandidate_ValidNoArgs(t *testing.T) { Source: ActionSourceRoute, } v := ValidateActionCandidate(c) - if v.Unresolved { + if v.Unresolved() { t.Error("expected resolved, not unresolved") } - if !v.Valid { - t.Errorf("expected valid, got issues: %v", v.Issues) + if v.Status != ActionValid { + t.Errorf("Status = %q, want valid; issues: %v", v.Status, v.Issues) } } @@ -234,12 +234,15 @@ func TestValidateActionCandidate_BlankFn(t *testing.T) { Source: ActionSourceRoute, } v := ValidateActionCandidate(c) - if v.Unresolved { + if v.Unresolved() { t.Error("blank Fn should be invalid, not unresolved") } - if v.Valid { + if v.Valid() { t.Error("blank Fn should be invalid") } + if v.Status != ActionInvalidArgument { + t.Errorf("Status = %q, want invalid_argument", v.Status) + } if len(v.Issues) != 1 { t.Fatalf("expected 1 issue, got %d", len(v.Issues)) } @@ -247,3 +250,105 @@ func TestValidateActionCandidate_BlankFn(t *testing.T) { t.Errorf("issue field = %q, want fn", v.Issues[0].Field) } } + +// --- additional typed status tests --- + +// TestValidateActionCandidate_StatusConstants pins that the five status +// constants are distinct and non-empty. +func TestValidateActionCandidate_StatusConstants(t *testing.T) { + statuses := []ActionValidationStatus{ + ActionValid, + ActionUnresolved, + ActionMissingArgument, + ActionInvalidArgument, + ActionAmbiguousTarget, + } + seen := make(map[ActionValidationStatus]bool) + for _, s := range statuses { + if s == "" { + t.Error("status constant is empty") + } + if seen[s] { + t.Errorf("status %q appears twice", s) + } + seen[s] = true + } +} + +// TestValidateActionCandidate_UnresolvedBackwardCompat pins that the +// Unresolved() method returns true only for ActionUnresolved status. +func TestValidateActionCandidate_UnresolvedBackwardCompat(t *testing.T) { + tests := []struct { + name string + status ActionValidationStatus + want bool + }{ + {"unresolved", ActionUnresolved, true}, + {"valid", ActionValid, false}, + {"missing_arg", ActionMissingArgument, false}, + {"invalid_arg", ActionInvalidArgument, false}, + {"ambiguous", ActionAmbiguousTarget, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := ActionValidationResult{Status: tt.status} + if got := r.Unresolved(); got != tt.want { + t.Errorf("Unresolved() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestValidateActionCandidate_ValidBackwardCompat pins that the Valid() +// method returns true only for ActionValid status. +func TestValidateActionCandidate_ValidBackwardCompat(t *testing.T) { + tests := []struct { + name string + status ActionValidationStatus + want bool + }{ + {"unresolved", ActionUnresolved, false}, + {"valid", ActionValid, true}, + {"missing_arg", ActionMissingArgument, false}, + {"invalid_arg", ActionInvalidArgument, false}, + {"ambiguous", ActionAmbiguousTarget, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := ActionValidationResult{Status: tt.status} + if got := r.Valid(); got != tt.want { + t.Errorf("Valid() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestValidateActionCandidate_IssuesNilOnValid pins that Valid results have +// nil Issues. +func TestValidateActionCandidate_IssuesNilOnValid(t *testing.T) { + c := ActionCandidate{Fn: "restart", Args: []string{"nginx"}} + v := ValidateActionCandidate(c) + if v.Issues != nil { + t.Errorf("valid result has issues: %v", v.Issues) + } +} + +// TestValidateActionCandidate_IssuesNilOnUnresolved pins that Unresolved +// results have nil Issues. +func TestValidateActionCandidate_IssuesNilOnUnresolved(t *testing.T) { + c := ActionCandidate{} + v := ValidateActionCandidate(c) + if v.Issues != nil { + t.Errorf("unresolved result has issues: %v", v.Issues) + } +} + +// TestValidateActionCandidate_IssuesPopulatedOnInvalid pins that Invalid +// results carry populated Issues. +func TestValidateActionCandidate_IssuesPopulatedOnInvalid(t *testing.T) { + c := ActionCandidate{Fn: "\t\n"} + v := ValidateActionCandidate(c) + if len(v.Issues) == 0 { + t.Error("invalid result has no issues") + } +}