From 16d5cb3d6259303c41d3d560c410b629fc6e2e55 Mon Sep 17 00:00:00 2001 From: ShocOne <62835948+ShocOne@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:11:23 +0100 Subject: [PATCH] fix: the audit reads the refusals it is given Every entity blocked on its first create. The refusal reader took the first string-valued key it recognised, so an envelope carrying both a summary sentence and a list of per-field complaints yielded the summary: {"detail":"There are invalid or missing fields", "errors":[{"field":"testName","message":"must not be null"}]} Nothing in the grammar matches "There are invalid or missing fields", so the adjustment loop stopped with the one field it needed named in the part it never read. The listed complaints are now read first, in string and object form, and two grammars are added for the shapes a validation framework wraps in prose: a field named mid-sentence before its complaint, and the bare English that names only the field it wanted. A dotted path reduces to its last segment, which is the name the request body spells. Re-creating a parent so a child has something to address now heals like any other create, and does so without recording: the fields a parent needs are facts about the parent, and its own steps record them. Recorded here they would be attributed to whichever child asked for it first. Measured against the live document: adjustments per run rise from one to four, and requiredByAPI observations from 78 to 81. Co-Authored-By: Claude Opus 5 (1M context) --- internal/audit/run/adjust.go | 125 +++++++++++++++++++++++-- internal/audit/run/adjust_unit_test.go | 76 +++++++++++++++ internal/audit/run/entity.go | 6 +- internal/audit/run/steps_update.go | 2 +- 4 files changed, 201 insertions(+), 8 deletions(-) diff --git a/internal/audit/run/adjust.go b/internal/audit/run/adjust.go index e299a32..402cb39 100644 --- a/internal/audit/run/adjust.go +++ b/internal/audit/run/adjust.go @@ -103,6 +103,23 @@ var ( reNotValid = regexp.MustCompile(`field (\S+) is not valid(?: when (\w+)=(\S+))?`) reReference = regexp.MustCompile(`field (\S+) must reference an existing (\w+)`) reRequired = regexp.MustCompile(`field (\S+) is required(?: when (\w+)=(\S+))?`) + + // reFieldNamed matches a refusal that names its field mid-sentence and + // states the complaint after a separator, which is how a framework that + // wraps its validation errors in prose reports them. + reFieldNamed = regexp.MustCompile(`(?i)\bfield\s+([\w.\[\]-]+)\s*[:\-]\s*(.+)`) + // reTheRequired matches the bare English an API writes when it names the + // field it wanted and nothing else about it. + reTheRequired = regexp.MustCompile(`(?i)\bthe\s+([\w.]+)\s+(?:is|are)\s+required\b`) + // reFieldSaid matches the field-prefixed refusal a validation framework + // emits: the property it rejected, a colon, then its complaint. The field + // is a dotted path when the API validates a nested request object, and the + // last segment is the name the request body spells. + reFieldSaid = regexp.MustCompile(`^\s*([\w.\[\]-]+)\s*:\s*(.+)$`) + // reAbsent matches the complaints that mean "you sent nothing for this", + // as distinct from "what you sent is wrong": only absence is healed by + // adding a value. + reAbsent = regexp.MustCompile(`(?i)\b(?:is required|is mandatory|must not be (?:null|empty|blank)|may not be (?:null|empty|blank)|cannot be (?:null|empty|blank)|must be (?:provided|specified|present)|missing)\b`) ) // classifyRefusal reads a 4xx response body and decides what to change. The @@ -126,9 +143,32 @@ func classifyRefusal(res *httpResult) refusalAction { if m := reRequired.FindStringSubmatch(msg); m != nil { return refusalAction{kind: actAdd, field: cleanField(m[1]), condGate: m[2], condVal: cleanField(m[3])} } + if m := reFieldNamed.FindStringSubmatch(msg); m != nil && reAbsent.MatchString(m[2]) { + return refusalAction{kind: actAdd, field: leafField(m[1])} + } + if m := reTheRequired.FindStringSubmatch(msg); m != nil { + return refusalAction{kind: actAdd, field: leafField(m[1])} + } + // Checked last, because it is the loosest: any sentence at all can be read + // as ": ", so it must not pre-empt a grammar that names + // two fields or a gate. + if m := reFieldSaid.FindStringSubmatch(msg); m != nil && reAbsent.MatchString(m[2]) { + return refusalAction{kind: actAdd, field: leafField(m[1])} + } return refusalAction{kind: actStop} } +// leafField is the last segment of a dotted refusal path, which is the name +// the request body spells: an API that validates its own nested request object +// reports "endpoint.url" for a property the document declares as "url". +func leafField(s string) string { + s = cleanField(s) + if i := strings.LastIndex(s, "."); i >= 0 { + s = s[i+1:] + } + return s +} + // refusalMessage pulls the human-legible sentence out of whichever error // envelope the API used — problem+json's detail, an oauth error_description, a // legacy errorMessage — falling back to the raw body when it is not JSON. It @@ -137,6 +177,12 @@ func classifyRefusal(res *httpResult) refusalAction { func refusalMessage(raw []byte) string { var m map[string]any if err := json.Unmarshal(raw, &m); err == nil { + // The listed complaints come first: an envelope that carries both + // names the field it rejected in the list and only summarises it in + // the sentence, and a summary heals nothing. + if listed := firstListed(m); listed != "" { + return listed + } for _, k := range []string{"detail", "message", "error_description", "errorMessage", "error", "title"} { if s, ok := m[k].(string); ok && s != "" { return s @@ -147,6 +193,54 @@ func refusalMessage(raw []byte) string { return string(raw) } +// firstListed pulls the first complaint out of an envelope that carries them +// as a list rather than a sentence, which is how an API that validates every +// property before answering reports what it rejected. +// +// Only the first is read: one refusal heals one field, and the next attempt +// re-reads whatever the API then complains about, so taking them one at a time +// converges without assuming the list is ordered or complete. +func firstListed(m map[string]any) string { + for _, k := range []string{"errors", "messages", "details", "errorMessages", "validationErrors"} { + listed, ok := m[k].([]any) + if !ok { + continue + } + for _, entry := range listed { + switch e := entry.(type) { + case string: + if e != "" { + return e + } + case map[string]any: + // An entry that names the field separately is spelled back + // into the ": " shape the grammar reads. + field, _ := firstString(e, "field", "name", "property", "path", "pointer", "code") + complaint, found := firstString(e, "message", "defaultMessage", "detail", "description", "error", "reason") + if !found { + continue + } + if field != "" { + return field + ": " + complaint + } + return complaint + } + } + } + return "" +} + +// firstString returns the first of the named keys the map carries as a +// non-empty string, and whether it found one. +func firstString(m map[string]any, keys ...string) (string, bool) { + for _, k := range keys { + if s, ok := m[k].(string); ok && s != "" { + return s, true + } + } + return "", false +} + // cleanField strips the trailing punctuation a refusal sentence might carry // after a field name. func cleanField(s string) string { @@ -162,6 +256,17 @@ func cleanField(s string) string { // for a per-enum-value create — so cycling searches the other enum fields for a // body the API accepts without abandoning what the step is exercising. func (r *runner) adjustCreate(ctx context.Context, ent *entityState, rec *entityRecipe, body map[string]any, held string) (adjustResult, error) { + return r.adjustCreateRecording(ctx, ent, rec, body, held, true) +} + +// adjustCreateRecording is adjustCreate, and also says whether the healing it +// does is a fact about the entity. +// +// Re-creating a parent so a child has something to address is a means to an +// end: the fields that create needs are facts about the parent, and the +// parent's own steps record them against the parent. Recorded here they would +// be attributed to whichever child happened to need the parent first. +func (r *runner) adjustCreateRecording(ctx context.Context, ent *entityState, rec *entityRecipe, body map[string]any, held string, record bool) (adjustResult, error) { applied := map[string]bool{} var last *httpResult adjusted := false @@ -177,7 +282,7 @@ func (r *runner) adjustCreate(ctx context.Context, ent *entityState, rec *entity if res == nil || !res.refused() { return adjustResult{res: res, body: body, adjusted: adjusted, gaveUp: true}, nil } - if r.applyAdjustment(ctx, ent, body, res, applied) { + if r.applyAdjustment(ctx, ent, body, res, applied, record) { adjusted = true continue } @@ -204,7 +309,7 @@ func (r *runner) adjustCreate(ctx context.Context, ent *entityState, rec *entity // already present, a remove of a field already absent, a nested "a.b" target // it cannot synthesise, or a borrow that returns the same value all stop the // loop rather than spin it. -func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map[string]any, res *httpResult, applied map[string]bool) bool { +func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map[string]any, res *httpResult, applied map[string]bool, record bool) bool { act := classifyRefusal(res) switch act.kind { case actAdd: @@ -213,7 +318,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } body[act.field] = r.synthField(ent, act.field) applied["a:"+act.field] = true - r.recordAdjustAdd(ent, act.field, act.condGate, act.condVal, res.excerpt) + if record { + r.recordAdjustAdd(ent, act.field, act.condGate, act.condVal, res.excerpt) + } return true case actRequires: if strings.Contains(act.field, ".") || applied["a:"+act.field] || present(body, act.field) { @@ -221,7 +328,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } body[act.field] = r.synthField(ent, act.field) applied["a:"+act.field] = true - r.recordAdjustment(ent, infer.AdjustRequires, act.field, act.trigger, "") + if record { + r.recordAdjustment(ent, infer.AdjustRequires, act.field, act.trigger, "") + } return true case actRemove: if !present(body, act.field) || applied["r:"+act.field] { @@ -229,7 +338,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } delete(body, act.field) applied["r:"+act.field] = true - r.recordAdjustment(ent, infer.AdjustRemove, act.field, act.condGate, act.condVal) + if record { + r.recordAdjustment(ent, infer.AdjustRemove, act.field, act.condGate, act.condVal) + } return true case actBorrow: id, ok := r.borrow(ctx, ent, act.collection) @@ -238,7 +349,9 @@ func (r *runner) applyAdjustment(ctx context.Context, ent *entityState, body map } body[act.field] = id applied["b:"+act.field] = true - r.recordAdjustment(ent, infer.AdjustBorrow, act.field, act.collection, "") + if record { + r.recordAdjustment(ent, infer.AdjustBorrow, act.field, act.collection, "") + } return true default: return false diff --git a/internal/audit/run/adjust_unit_test.go b/internal/audit/run/adjust_unit_test.go index 074a347..0622de9 100644 --- a/internal/audit/run/adjust_unit_test.go +++ b/internal/audit/run/adjust_unit_test.go @@ -63,6 +63,56 @@ func TestUnit_Adjust_ClassifyRefusalGrammar(t *testing.T) { {"plain-text", `field interval is required`, actAdd, "interval", ""}, {"empty", ``, actStop, "", ""}, {"unparseable", `{"weird":true}`, actStop, "", ""}, + + // An envelope that lists its complaints rather than stating one, and + // spells the rejected property as a path into its own request object. + {"listed-strings", + `{"errors":["endpoint.streamEndpointUrl: Endpoint URL cannot be blank"],"httpStatus":"400 BAD_REQUEST"}`, + actAdd, "streamEndpointUrl", ""}, + {"listed-objects", + `{"errors":[{"field":"interval","message":"must not be null"}]}`, + actAdd, "interval", ""}, + {"listed-under-messages", + `{"messages":["interval: is required"]}`, + actAdd, "interval", ""}, + {"listed-empty", `{"errors":[]}`, actStop, "", ""}, + // A field-prefixed complaint about the value that was sent, rather + // than about its absence: adding a value cannot heal it. + {"field-said-not-absence", + `{"errors":["interval: must be one of 60, 120, 300"]}`, + actStop, "", ""}, + // A sentence that merely contains a colon is not a field complaint. + {"prose-with-colon", + `{"detail":"Validation failed: the request was rejected"}`, + actStop, "", ""}, + // Both shapes at once: the sentence only summarises, the list names + // the field, so the list is what the loop must act on. + {"summary-beside-listed", + `{"detail":"There are invalid or missing fields","errors":[{"field":"testName","message":"must not be null"}],"title":"Request validation failed"}`, + actAdd, "testName", ""}, + // A validation framework's own error object, which spells the field + // as a code and the complaint as a default message. + {"listed-code-and-default-message", + `{"errors":[{"code":"name","defaultMessage":"must not be blank"}]}`, + actAdd, "name", ""}, + // A refusal that names its field mid-sentence, wrapped in prose. + {"field-named-in-prose", + `{"title":"There were some errors in your request, please correct them before trying again. Error in field roleName : must not be null."}`, + actAdd, "roleName", ""}, + // The same shape, but complaining about the value rather than its + // absence: adding one cannot heal it. + {"field-named-in-prose-not-absence", + `{"title":"Error in field roleName : must be one of a, b"}`, + actStop, "", ""}, + // Bare English naming only the field it wanted. + {"the-field-is-required", + `{"title":"The loginAccountGroup is required"}`, + actAdd, "loginAccountGroup", ""}, + // "field X is required" still wins over the bare-English reading, so + // the field is X and not the word "field". + {"field-keyword-beats-bare-english", + `{"detail":"the field interval is required"}`, + actAdd, "interval", ""}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -213,3 +263,29 @@ func TestUnit_Borrow_CollectionPaths(t *testing.T) { t.Errorf("collectionPaths(agents) = %v", got) } } + +// TestUnit_Adjust_ParentRecreationHealsWithoutRecording pins the split +// between healing and recording. A parent re-created so a child has something +// to address must still be healed — otherwise every child of an understated +// parent blocks — but the fields it needed are facts about the parent, and +// recording them here would attribute them to the child that asked. +func TestUnit_Adjust_ParentRecreationHealsWithoutRecording(t *testing.T) { + t.Parallel() + + refusal := &httpResult{body: []byte( + `{"detail":"There are invalid or missing fields","errors":[{"field":"testName","message":"must not be null"}]}`)} + + r := &runner{opts: Options{NamePrefix: "tfpfgen"}} + ent := &entityState{plan: &plan.EntityPlan{Entity: "scheduled_test"}} + body := map[string]any{} + + if !r.applyAdjustment(context.Background(), ent, body, refusal, map[string]bool{}, false) { + t.Fatal("a listed field complaint did not heal the body") + } + if _, added := body["testName"]; !added { + t.Errorf("the named field was not added: %#v", body) + } + if len(r.adjustments) != 0 { + t.Errorf("a silent heal recorded %d adjustment(s)", len(r.adjustments)) + } +} diff --git a/internal/audit/run/entity.go b/internal/audit/run/entity.go index d1c4b86..1c291f8 100644 --- a/internal/audit/run/entity.go +++ b/internal/audit/run/entity.go @@ -247,10 +247,14 @@ func (r *runner) resolveCreated(ctx context.Context, ent *entityState, entity st if !ok || rec.minimalBody == nil { return "", blockedError{reason: fmt.Sprintf("no created %s object exists and its entity has no create recipe", entity)} } - obj, _, err := r.createObject(ctx, ent, rec, rec.minimalBody) + // Healed like any other create: a parent the document understates is + // refused the same way its own create was, and without the loop every + // child of it blocks on a refusal the loop can read. + rr, err := r.adjustCreateRecording(ctx, ent, rec, cloneAnyMap(rec.minimalBody), "", false) if err != nil { return "", err } + obj := rr.obj if obj == nil { return "", blockedError{reason: fmt.Sprintf("re-creating the %s parent object was refused", entity)} } diff --git a/internal/audit/run/steps_update.go b/internal/audit/run/steps_update.go index 7459b71..1213293 100644 --- a/internal/audit/run/steps_update.go +++ b/internal/audit/run/steps_update.go @@ -33,7 +33,7 @@ func (r *runner) adjustUpdate(ctx context.Context, ent *entityState, step *plan. if res.ok() || !res.refused() { return res, sent, nil } - if !r.applyAdjustment(ctx, ent, body, res, applied) { + if !r.applyAdjustment(ctx, ent, body, res, applied, true) { return res, sent, nil } }