From 5cab4e512e78140d6a1658b19d06529d738e7f2c Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 28 Aug 2026 14:37:22 +0000 Subject: [PATCH] Improve item occurrence holder corrections --- .../item-occurrences/prompts/instructions.md | 20 ++- .../schemas/dnd_item_occurrences_llm.v1.json | 5 +- docs/internal/dnd.md | 7 + docs/roadmap/future.md | 2 +- .../item-occurrence-holder-reliability.md | 95 ++++++++++ .../extract/itemoccurrences/extractor_test.go | 45 +++++ .../itemoccurrences/prompt_assets_test.go | 11 ++ .../extract/itemoccurrences/schema_test.go | 3 +- .../itemoccurrences/shape/validator.go | 166 ++++++++++++++++-- .../itemoccurrences/shape/validator_test.go | 83 ++++++++- 10 files changed, 410 insertions(+), 27 deletions(-) create mode 100644 docs/roadmap/item-occurrence-holder-reliability.md diff --git a/assets/dnd/item-occurrences/prompts/instructions.md b/assets/dnd/item-occurrences/prompts/instructions.md index 16e8b1e5..f7d56a8e 100644 --- a/assets/dnd/item-occurrences/prompts/instructions.md +++ b/assets/dnd/item-occurrences/prompts/instructions.md @@ -18,12 +18,20 @@ when the transcript explicitly describes it being physically destroyed or expended as a non-payment component. Use `transferred` only when possession moves between two distinct named party members. -Return both `from` and `to` for every occurrence, using `null` when a holder does not -apply. For `discovered`, set both holders to `null`. For `acquired`, set `from` -to `null` and provide `to`; for `lost` and `consumed`, provide `from` and set -`to` to `null`; and for `transferred`, provide both holders. Use `party` only -for collective or unresolved party possession, never for either side of a -transfer. Do not emit a transfer for a gift, sale, or payment outside the party. +Return both `from` and `to` for every occurrence. Use JSON `null`, not an empty +string, whenever a holder does not apply. Follow this holder matrix exactly: + +| `kind` | required `from` | required `to` | +| --- | --- | --- | +| `discovered` | `null` | `null` | +| `acquired` | `null` | `party` or the named party member gaining possession | +| `lost` | `party` or the named party member losing possession | `null` | +| `consumed` | `party` or the named party member consuming the item | `null` | +| `transferred` | one named party member | a different named party member | + +Use `party` only for collective or unresolved party possession, never for +either side of a transfer. Do not emit a transfer for a gift, sale, or payment +outside the party. Ordinary non-depleting use is not an occurrence. Do not infer acquisition from a discovery, or discovery from an acquisition: emit both only when each is diff --git a/assets/dnd/item-occurrences/schemas/dnd_item_occurrences_llm.v1.json b/assets/dnd/item-occurrences/schemas/dnd_item_occurrences_llm.v1.json index 025cc1c5..ae97fe07 100644 --- a/assets/dnd/item-occurrences/schemas/dnd_item_occurrences_llm.v1.json +++ b/assets/dnd/item-occurrences/schemas/dnd_item_occurrences_llm.v1.json @@ -13,7 +13,10 @@ "required": ["name", "kind", "quantity", "from", "to", "source_refs"], "properties": { "name": {"type": "string"}, - "kind": {"type": "string"}, + "kind": { + "type": "string", + "enum": ["discovered", "acquired", "lost", "consumed", "transferred"] + }, "quantity": {"type": ["integer", "null"]}, "from": {"type": ["string", "null"]}, "to": {"type": ["string", "null"]}, diff --git a/docs/internal/dnd.md b/docs/internal/dnd.md index 98aa071d..17892de8 100644 --- a/docs/internal/dnd.md +++ b/docs/internal/dnd.md @@ -201,6 +201,13 @@ hashes, validator module keys, or reason codes. Those identifiers remain in ordinary validation provenance; only the actionable semantic guidance is eligible for the correction prompt. +Item-occurrence shape validation groups repeated holder mistakes by occurrence +kind and gives the producer the required JSON null/non-null relationship. It +identifies affected records by contextual item name and cited transcript range, +never by the deterministically attached durable item ID. Holder mistakes remain +semantic rejections rather than silent rewrites because changing a holder can +also change the meaning of the occurrence kind. + Enemy-event extraction additionally rejects a second `engaged` observation for the same comparison identity within one scene-scoped result. Normalization may combine results from distinct scenes, so it intentionally does not apply that diff --git a/docs/roadmap/future.md b/docs/roadmap/future.md index 8cec390e..3f555d9e 100644 --- a/docs/roadmap/future.md +++ b/docs/roadmap/future.md @@ -15,7 +15,7 @@ diagnostics. The remaining near-term work applies those completed foundations to domain review and empirical evaluation. The active D&D reliability work is defined by -[D&D Source-Reference Endpoint Canonicalization](source-reference-canonicalization.md). +[D&D Item-Occurrence Holder Reliability](item-occurrence-holder-reliability.md). ## Near-Term D&D Pipeline diff --git a/docs/roadmap/item-occurrence-holder-reliability.md b/docs/roadmap/item-occurrence-holder-reliability.md new file mode 100644 index 00000000..ab888232 --- /dev/null +++ b/docs/roadmap/item-occurrence-holder-reliability.md @@ -0,0 +1,95 @@ +# D&D Item-Occurrence Holder Reliability + +## Purpose + +Improve item-occurrence extraction reliability for smaller models by making +holder rules easier to follow and semantic retry feedback specific enough to +correct a rejected response. + +## Problem + +The item-occurrence prompt correctly defines the permitted `from` and `to` +values for every occurrence kind, but presents the rules in dense prose. The +private schema requires both nullable fields but cannot express their +cross-field relationship. The deterministic shape validator correctly rejects +incompatible combinations, yet its model-facing correction guidance only asks +for generally valid holder combinations. Notarius therefore resends the +defective response without telling the model which contextual records failed or +what their corrected holder shape must be. + +## Target State + +- The module instructions present one compact, unambiguous holder matrix: + `discovered` uses two nulls; `acquired` uses a null `from` and non-null `to`; + `lost` and `consumed` use a non-null `from` and null `to`; and `transferred` + uses two distinct named party members. +- The private LLM schema continues requiring `quantity`, `from`, and `to` with + nullable types, and constrains `kind` to the five supported values. The + durable v1 artifact contract remains unchanged. +- A holder-combination rejection supplies bounded, semantically meaningful + correction guidance that identifies affected occurrences by contextual item + name and transcript evidence where useful, states the required JSON + null/non-null shape, and requests a complete corrected replacement. +- Correction guidance never exposes durable item IDs, hashes, validator keys, + reason codes, or other opaque implementation identifiers. It does not rely + solely on mapped array indexes, because canonical extraction ordering may + differ from the raw response order appended to the retry prompt. +- Operator-facing validation messages remain specific and include enough + information to distinguish a missing holder from an extra or misplaced + holder. +- The existing validator remains strict. Holder combinations are not silently + rewritten: unlike reversed evidence endpoints, changing a holder can conceal + a misclassified discovery, acquisition, loss, consumption, or transfer. + +## Required Work + +1. Rewrite the holder paragraph in + `assets/dnd/item-occurrences/prompts/instructions.md` as a compact matrix, + retaining the existing classification and currency rules without adding + contradictory repetition. +2. Add the five supported `kind` values as an enum in the private item-occurrence + response schema. Keep every listed field required and keep `quantity`, + `from`, and `to` explicitly nullable. +3. Refactor the item-occurrence shape validator to build detailed bounded + operator issues and separate actionable correction guidance from the same + evaluated candidate. Group repeated holder failures where practical while + retaining contextual record identification and every distinct correction + rule needed by the model. +4. Preserve the generic validation retry protocol: the complete original + prompt, exact defective assistant response, and one user correction message. + Do not add a module-local retry loop. +5. Ensure the changed prompt and schema produce new computed fingerprints, and + bump the validator policy fingerprint so stale checkpoints cannot be reused + across the behavior change. Update internal documentation where needed; the + external item-occurrence contract requires no semantic change. +6. Add lean offline regression coverage for the holder matrix, schema enum, + contextual correction guidance, bounded aggregation, absence of opaque IDs, + and a rejected-then-corrected extraction attempt. Avoid exact prose snapshots + and live-provider tests. + +## Non-Goals + +- Automatically deleting, moving, or inferring holder values. +- Changing the durable item-occurrence schema or event taxonomy. +- Encoding the complete semantic holder matrix through provider-sensitive + conditional JSON Schema constructs. +- Adding warnings for ordinary semantic rejection or successful correction. +- Tuning unrelated D&D extraction prompts or validators. + +## Acceptance Criteria + +- The maintained item-occurrence prompt shows the clear holder matrix, + and the private schema rejects unsupported kind strings while remaining + compatible with strict structured-output providers. +- A rejected `acquired` occurrence receives guidance that explicitly requires + `from: null` and a non-null `to`; equivalent exact guidance exists for the + other four kinds. +- Multiple invalid occurrences yield one bounded correction request containing + all distinct required fixes and contextual identifiers that can be matched to + the appended raw response. +- No correction message contains an item ID, hash, validator key, or reason + code, and the raw model candidate remains byte-faithful. +- Valid candidates and the durable output contract are unchanged; invalid + holder combinations still fail after configured retries are exhausted. +- Focused tests, `go test ./...`, `go vet ./...`, and `go build ./cmd/notarius` + pass. diff --git a/internal/modules/dnd/extract/itemoccurrences/extractor_test.go b/internal/modules/dnd/extract/itemoccurrences/extractor_test.go index b1dc46c1..81bd7a17 100644 --- a/internal/modules/dnd/extract/itemoccurrences/extractor_test.go +++ b/internal/modules/dnd/extract/itemoccurrences/extractor_test.go @@ -6,7 +6,9 @@ import ( "testing" "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" + "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd" itemidentity "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/items/identity" + itemoccurrenceshape "gitea.maximumdirect.net/eric/notarius/internal/modules/dnd/validate/itemoccurrences/shape" ) func TestExtractGroundsOccurrencesInRequiredRegistry(t *testing.T) { @@ -146,3 +148,46 @@ func TestExtractUsesOnlySupportedPromptInputs(t *testing.T) { t.Fatalf("unexpected prompt input: %#v", client.requests[0].Inputs) } } + +func TestExtractCarriesRejectedHolderGuidanceIntoCorrectedAttempt(t *testing.T) { + references := itemRegistryReferences(t) + req := extractionRequest() + req.References = references + defectiveResponse := []byte(`{"occurrences":[{"name":"Torch","kind":"acquired","quantity":null,"from":"Chest","to":"party","source_refs":[{"start_unit_id":2,"end_unit_id":2}]}]}`) + defective, err := newExtractor(t, &fakeItemOccurrencesLLMClient{content: defectiveResponse}, references).Extract(context.Background(), req) + if err != nil { + t.Fatal(err) + } + validation, err := itemoccurrenceshape.New(itemoccurrenceshape.Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.ItemOccurrenceList]{Value: defective.Value}) + if err != nil || validation.Approved { + t.Fatalf("holder validation = %#v, %v; want rejection", validation, err) + } + for _, fragment := range []string{"For `acquired` occurrences", "`from` to JSON null", "item \"Torch\"", "source unit 2"} { + if !strings.Contains(validation.CorrectionGuidance, fragment) { + t.Fatalf("CorrectionGuidance = %q, want %q", validation.CorrectionGuidance, fragment) + } + } + for _, forbidden := range []string{defective.Value.Occurrences[0].ItemID, "item_id", "sha256", itemoccurrenceshape.Key, itemoccurrenceshape.ReasonCode} { + if strings.Contains(validation.CorrectionGuidance, forbidden) { + t.Fatalf("CorrectionGuidance leaked implementation identifier %q: %q", forbidden, validation.CorrectionGuidance) + } + } + correction, err := contracts.NewSemanticCorrection(defective.ModelCandidate.Response, validation.CorrectionGuidance) + if err != nil { + t.Fatal(err) + } + correctedResponse := []byte(`{"occurrences":[{"name":"Torch","kind":"acquired","quantity":null,"from":null,"to":"party","source_refs":[{"start_unit_id":2,"end_unit_id":2}]}]}`) + client := &fakeItemOccurrencesLLMClient{content: correctedResponse} + req.Correction = correction + corrected, err := newExtractor(t, client, references).Extract(context.Background(), req) + if err != nil { + t.Fatal(err) + } + accepted, err := itemoccurrenceshape.New(itemoccurrenceshape.Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.ItemOccurrenceList]{Value: corrected.Value}) + if err != nil || !accepted.Approved { + t.Fatalf("corrected holder validation = %#v, %v; want approval", accepted, err) + } + if len(client.requests) != 1 || client.requests[0].Correction == nil || string(client.requests[0].Correction.AssistantResponse) != string(defectiveResponse) || client.requests[0].Correction.UserGuidance != validation.CorrectionGuidance { + t.Fatalf("corrected request = %#v, want byte-faithful defective response and semantic guidance", client.requests) + } +} diff --git a/internal/modules/dnd/extract/itemoccurrences/prompt_assets_test.go b/internal/modules/dnd/extract/itemoccurrences/prompt_assets_test.go index a98f08fe..2d4af206 100644 --- a/internal/modules/dnd/extract/itemoccurrences/prompt_assets_test.go +++ b/internal/modules/dnd/extract/itemoccurrences/prompt_assets_test.go @@ -58,6 +58,17 @@ func TestPromptAssetsPrepareItemOccurrencePrompt(t *testing.T) { t.Fatalf("prepared prompt contains obsolete evidence field %q: %s", obsolete, content) } } + for _, holderRule := range []string{ + "| `discovered` | `null` | `null` |", + "| `acquired` | `null` | `party` or the named party member gaining possession |", + "| `lost` | `party` or the named party member losing possession | `null` |", + "| `consumed` | `party` or the named party member consuming the item | `null` |", + "| `transferred` | one named party member | a different named party member |", + } { + if !strings.Contains(content, holderRule) { + t.Fatalf("prepared prompt does not include holder rule %q", holderRule) + } + } } func TestPromptAssetsDoNotLeakIntoMetadata(t *testing.T) { diff --git a/internal/modules/dnd/extract/itemoccurrences/schema_test.go b/internal/modules/dnd/extract/itemoccurrences/schema_test.go index 1a13a110..bff87624 100644 --- a/internal/modules/dnd/extract/itemoccurrences/schema_test.go +++ b/internal/modules/dnd/extract/itemoccurrences/schema_test.go @@ -19,7 +19,7 @@ func TestResponseSchemaIsStrictlyStructuralAndPrivate(t *testing.T) { } valid := map[string]any{"occurrences": []any{ map[string]any{ - "name": "", "kind": "unsupported", "quantity": 0, "from": "party", "to": "Party", + "name": "", "kind": "transferred", "quantity": 0, "from": "party", "to": "Party", "source_refs": []any{map[string]any{"start_unit_id": 0, "end_unit_id": -1}}, }, map[string]any{ @@ -41,6 +41,7 @@ func TestResponseSchemaIsStrictlyStructuralAndPrivate(t *testing.T) { {"missing occurrences", map[string]any{}}, {"missing occurrence name", map[string]any{"occurrences": []any{withoutField(responseOccurrence(), "name")}}}, {"missing nullable field", map[string]any{"occurrences": []any{withoutField(responseOccurrence(), "quantity")}}}, + {"unsupported kind", map[string]any{"occurrences": []any{withField(responseOccurrence(), "kind", "unsupported")}}}, {"opaque item identifier", map[string]any{"occurrences": []any{withField(responseOccurrence(), "item_id", "item:sha256:opaque")}}}, {"unknown occurrence field", map[string]any{"occurrences": []any{withField(responseOccurrence(), "extra", true)}}}, {"segment-named range", map[string]any{"occurrences": []any{withField(responseOccurrence(), "source_refs", []any{map[string]any{"start_segment": 1, "end_segment": 1}})}}}, diff --git a/internal/modules/dnd/validate/itemoccurrences/shape/validator.go b/internal/modules/dnd/validate/itemoccurrences/shape/validator.go index 02684217..44ee6256 100644 --- a/internal/modules/dnd/validate/itemoccurrences/shape/validator.go +++ b/internal/modules/dnd/validate/itemoccurrences/shape/validator.go @@ -16,7 +16,7 @@ import ( const ( Key = "extract/dnd/item-occurrences/shape" ReasonCode = "invalid_item_occurrence_shape" - policy = "dnd.item_occurrences.shape.v1" + policy = "dnd.item_occurrences.shape.v2" ) type Options struct{} @@ -35,47 +35,181 @@ func (v *Validator) CheckpointFingerprints() []pipeline.CheckpointFingerprint { } func (v *Validator) Validate(_ context.Context, req contracts.TypedValidationRequest[dnd.ItemOccurrenceList]) (contracts.ValidationResult, error) { - if err := Validate(req.Value); err != nil { - return contracts.ValidationResult{Approved: false, ReasonCode: ReasonCode, Message: err.Error(), CorrectionGuidance: "Return a complete item-occurrence list with every required field present, valid contextual item names, supported event kinds and holder combinations, positive quantities, and valid source references."}, nil + assessment := assess(req.Value) + if len(assessment.operatorIssues) != 0 { + return contracts.ValidationResult{ + Approved: false, + ReasonCode: ReasonCode, + Message: assessment.operatorMessage(), + CorrectionGuidance: assessment.correctionGuidance(), + }, nil } return contracts.ValidationResult{Approved: true}, nil } // Validate returns one bounded error for every owned item-occurrence shape issue. func Validate(value dnd.ItemOccurrenceList) error { - issues := issuesFor(value) - if len(issues) == 0 { + assessment := assess(value) + if len(assessment.operatorIssues) == 0 { return nil } - return fmt.Errorf("%s", diagnostics.Aggregate("invalid item occurrence shape", issues)) + return fmt.Errorf("%s", assessment.operatorMessage()) } -func issuesFor(value dnd.ItemOccurrenceList) []string { +type validationAssessment struct { + operatorIssues []string + correctionGroups []correctionGroup + groupIndexes map[string]int +} + +type correctionGroup struct { + label string + rule string + records []string + recordsSeen map[string]struct{} +} + +func assess(value dnd.ItemOccurrenceList) validationAssessment { + assessment := validationAssessment{groupIndexes: make(map[string]int)} if value.Occurrences == nil { - return []string{"occurrences must be present"} + assessment.operatorIssues = append(assessment.operatorIssues, "occurrences must be present") + assessment.addCorrection("occurrences", "item-occurrence list", "Return an `occurrences` array; use an empty array when the transcript establishes no occurrences.", "") + return assessment } - issues := make([]string, 0) for index, occurrence := range value.Occurrences { prefix := fmt.Sprintf("occurrences[%d]", index) + context := occurrenceContext(occurrence) if strings.TrimSpace(occurrence.ItemID) == "" { - issues = append(issues, prefix+".item_id must not be empty: "+diagnostics.Quote(occurrence.ItemID)) + assessment.operatorIssues = append(assessment.operatorIssues, prefix+".item_id must not be empty: "+diagnostics.Quote(occurrence.ItemID)) + assessment.addCorrection("item-name", "item name", "Select a contextual item name from the supplied item registry for every occurrence.", context) } if strings.TrimSpace(occurrence.Name) == "" { - issues = append(issues, prefix+".name must not be empty: "+diagnostics.Quote(occurrence.Name)) + assessment.operatorIssues = append(assessment.operatorIssues, prefix+".name must not be empty: "+diagnostics.Quote(occurrence.Name)) + assessment.addCorrection("item-name", "item name", "Select a contextual item name from the supplied item registry for every occurrence.", context) } if !itemoccurrences.SupportedKind(occurrence.Kind) { - issues = append(issues, prefix+".kind is unsupported: "+diagnostics.Quote(string(occurrence.Kind))) + assessment.operatorIssues = append(assessment.operatorIssues, prefix+".kind is unsupported: "+diagnostics.Quote(string(occurrence.Kind))) + assessment.addCorrection("kind", "occurrence kind", "Set `kind` to exactly one of `discovered`, `acquired`, `lost`, `consumed`, or `transferred`.", context) } else if !itemoccurrences.ValidHolderCombination(occurrence.Kind, occurrence.From, occurrence.To) { - issues = append(issues, prefix+".from and .to are incompatible with "+diagnostics.Quote(string(occurrence.Kind))) + assessment.operatorIssues = append(assessment.operatorIssues, prefix+holderOperatorIssue(occurrence)) + assessment.addCorrection("holders:"+string(occurrence.Kind), string(occurrence.Kind)+" holders", holderCorrection(occurrence.Kind), context) } if occurrence.Quantity != nil && *occurrence.Quantity < 1 { - issues = append(issues, prefix+".quantity must be positive when present") + assessment.operatorIssues = append(assessment.operatorIssues, prefix+".quantity must be positive when present") + assessment.addCorrection("quantity", "quantity", "Set `quantity` to a positive integer when the transcript states one, or to JSON null when it does not.", context) } if len(occurrence.SourceRefs) == 0 { - issues = append(issues, prefix+".source_refs must contain at least one reference") + assessment.operatorIssues = append(assessment.operatorIssues, prefix+".source_refs must contain at least one reference") + assessment.addCorrection("source-refs", "source references", "Provide at least one transcript source range that directly supports every occurrence.", context) } } - return issues + return assessment +} + +func (assessment *validationAssessment) addCorrection(key, label, rule, record string) { + index, ok := assessment.groupIndexes[key] + if !ok { + index = len(assessment.correctionGroups) + assessment.groupIndexes[key] = index + assessment.correctionGroups = append(assessment.correctionGroups, correctionGroup{label: label, rule: rule, recordsSeen: make(map[string]struct{})}) + } + if record != "" { + group := &assessment.correctionGroups[index] + if _, seen := group.recordsSeen[record]; seen { + return + } + group.recordsSeen[record] = struct{}{} + group.records = append(group.records, record) + } +} + +func (assessment validationAssessment) operatorMessage() string { + return diagnostics.Aggregate("invalid item occurrence shape", assessment.operatorIssues) +} + +func (assessment validationAssessment) correctionGuidance() string { + issues := make([]string, 0, len(assessment.correctionGroups)+len(assessment.operatorIssues)) + for _, group := range assessment.correctionGroups { + issues = append(issues, group.rule) + } + for _, group := range assessment.correctionGroups { + for _, record := range group.records { + issues = append(issues, "Affected "+group.label+" record: "+record) + } + } + return diagnostics.Aggregate("Correct every rejected item occurrence and return the complete replacement list", issues) +} + +func holderOperatorIssue(occurrence dnd.ItemOccurrence) string { + return fmt.Sprintf( + ".from and .to are incompatible with %s: expected %s; got from %s and to %s", + diagnostics.Quote(string(occurrence.Kind)), holderExpectation(occurrence.Kind), + holderDisplay(occurrence.From), holderDisplay(occurrence.To), + ) +} + +func holderExpectation(kind dnd.ItemOccurrenceKind) string { + switch kind { + case dnd.ItemOccurrenceKindDiscovered: + return "both holders absent" + case dnd.ItemOccurrenceKindAcquired: + return "from absent and to present" + case dnd.ItemOccurrenceKindLost: + return "from present and to absent" + case dnd.ItemOccurrenceKindConsumed: + return "from present and to absent" + case dnd.ItemOccurrenceKindTransferred: + return "distinct named non-party holders" + default: + return "a supported holder combination" + } +} + +func holderCorrection(kind dnd.ItemOccurrenceKind) string { + switch kind { + case dnd.ItemOccurrenceKindDiscovered: + return "For `discovered` occurrences, set both `from` and `to` to JSON null." + case dnd.ItemOccurrenceKindAcquired: + return "For `acquired` occurrences, set `from` to JSON null and `to` to `party` or the named party member gaining possession." + case dnd.ItemOccurrenceKindLost: + return "For `lost` occurrences, set `from` to `party` or the named party member losing possession and set `to` to JSON null." + case dnd.ItemOccurrenceKindConsumed: + return "For `consumed` occurrences, set `from` to `party` or the named party member consuming the item and set `to` to JSON null." + case dnd.ItemOccurrenceKindTransferred: + return "For `transferred` occurrences, set `from` and `to` to two distinct named party members; never use `party` for either holder." + default: + return "Use the holder combination required by the selected supported occurrence kind." + } +} + +func occurrenceContext(occurrence dnd.ItemOccurrence) string { + name := itemoccurrences.DisplayValue(occurrence.Name) + context := "item " + diagnostics.Quote(name) + if name == "" { + context = "item with a blank contextual name" + } + if len(occurrence.SourceRefs) == 0 { + context += " without a cited source range" + } else { + ref := occurrence.SourceRefs[0] + if ref.StartUnitID == ref.EndUnitID { + context += fmt.Sprintf(" at source unit %d", ref.StartUnitID) + } else { + context += fmt.Sprintf(" at source units %d-%d", ref.StartUnitID, ref.EndUnitID) + } + if len(occurrence.SourceRefs) > 1 { + context += fmt.Sprintf(" (first of %d cited ranges)", len(occurrence.SourceRefs)) + } + } + return context + " with from " + holderDisplay(occurrence.From) + " and to " + holderDisplay(occurrence.To) +} + +func holderDisplay(value string) string { + value = itemoccurrences.DisplayValue(value) + if value == "" { + return "JSON null" + } + return diagnostics.Quote(value) } func Spec() pipeline.ValidatorSpec { diff --git a/internal/modules/dnd/validate/itemoccurrences/shape/validator_test.go b/internal/modules/dnd/validate/itemoccurrences/shape/validator_test.go index 21eeb883..9dd7ca41 100644 --- a/internal/modules/dnd/validate/itemoccurrences/shape/validator_test.go +++ b/internal/modules/dnd/validate/itemoccurrences/shape/validator_test.go @@ -2,6 +2,7 @@ package shape import ( "context" + "fmt" "reflect" "strings" "testing" @@ -68,16 +69,94 @@ func TestValidatorRejectsOwnedSemanticBoundaries(t *testing.T) { } } +func TestValidatorProvidesActionableContextualHolderGuidance(t *testing.T) { + tests := []struct { + name string + occurrence dnd.ItemOccurrence + wantGuidance []string + }{ + { + name: "discovered", + occurrence: dnd.ItemOccurrence{ItemID: "item:sha256:opaque", Name: "Ring", Kind: dnd.ItemOccurrenceKindDiscovered, From: "Aria", SourceRefs: refs(1, 2)}, + wantGuidance: []string{"For `discovered` occurrences", "both `from` and `to` to JSON null"}, + }, + { + name: "acquired", + occurrence: dnd.ItemOccurrence{ItemID: "item:sha256:opaque", Name: "Ring", Kind: dnd.ItemOccurrenceKindAcquired, From: "Merchant", To: "party", SourceRefs: refs(1, 2)}, + wantGuidance: []string{"For `acquired` occurrences", "`from` to JSON null", "named party member gaining possession"}, + }, + { + name: "lost", + occurrence: dnd.ItemOccurrence{ItemID: "item:sha256:opaque", Name: "Ring", Kind: dnd.ItemOccurrenceKindLost, From: "party", To: "Merchant", SourceRefs: refs(1, 2)}, + wantGuidance: []string{"For `lost` occurrences", "named party member losing possession", "`to` to JSON null"}, + }, + { + name: "consumed", + occurrence: dnd.ItemOccurrence{ItemID: "item:sha256:opaque", Name: "Ring", Kind: dnd.ItemOccurrenceKindConsumed, SourceRefs: refs(1, 2)}, + wantGuidance: []string{"For `consumed` occurrences", "named party member consuming the item", "`to` to JSON null"}, + }, + { + name: "transferred", + occurrence: dnd.ItemOccurrence{ItemID: "item:sha256:opaque", Name: "Ring", Kind: dnd.ItemOccurrenceKindTransferred, From: "party", To: "Borin", SourceRefs: refs(1, 2)}, + wantGuidance: []string{"For `transferred` occurrences", "two distinct named party members", "never use `party`"}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result, err := New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.ItemOccurrenceList]{Value: listWith(test.occurrence)}) + if err != nil || result.Approved { + t.Fatalf("Validate() = %#v, %v; want rejection", result, err) + } + for _, fragment := range append(test.wantGuidance, "item \"Ring\"", "source units 1-2") { + if !strings.Contains(result.CorrectionGuidance, fragment) { + t.Fatalf("CorrectionGuidance = %q, want %q", result.CorrectionGuidance, fragment) + } + } + for _, forbidden := range []string{test.occurrence.ItemID, "sha256", Key, ReasonCode, "occurrences[0]"} { + if strings.Contains(result.CorrectionGuidance, forbidden) { + t.Fatalf("CorrectionGuidance leaked implementation identifier %q: %q", forbidden, result.CorrectionGuidance) + } + } + if !strings.Contains(result.Message, "expected") || !strings.Contains(result.Message, "got from") { + t.Fatalf("operator message = %q, want expected and observed holders", result.Message) + } + }) + } +} + +func TestValidatorGroupsRepeatedHolderCorrectionsAndRetainsDistinctRules(t *testing.T) { + value := dnd.ItemOccurrenceList{Occurrences: []dnd.ItemOccurrence{ + {ItemID: "item-1", Name: "Silver Pieces", Kind: dnd.ItemOccurrenceKindAcquired, From: "Orc", To: "party", SourceRefs: refs(1, 1)}, + {ItemID: "item-2", Name: "Torch", Kind: dnd.ItemOccurrenceKindAcquired, From: "Chest", To: "Aria", SourceRefs: refs(2, 2)}, + {ItemID: "item-3", Name: "Potion", Kind: dnd.ItemOccurrenceKindConsumed, SourceRefs: refs(3, 3)}, + }} + result, err := New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.ItemOccurrenceList]{Value: value}) + if err != nil || result.Approved { + t.Fatalf("Validate() = %#v, %v; want rejection", result, err) + } + if strings.Count(result.CorrectionGuidance, "For `acquired` occurrences") != 1 || strings.Count(result.CorrectionGuidance, "For `consumed` occurrences") != 1 { + t.Fatalf("CorrectionGuidance = %q, want one rule per affected kind", result.CorrectionGuidance) + } + for _, fragment := range []string{"item \"Silver Pieces\"", "source unit 1", "item \"Torch\"", "source unit 2", "item \"Potion\"", "source unit 3"} { + if !strings.Contains(result.CorrectionGuidance, fragment) { + t.Fatalf("CorrectionGuidance = %q, want contextual fragment %q", result.CorrectionGuidance, fragment) + } + } +} + func TestValidatorAggregatesBoundedIndexedDiagnosticsAndRegistration(t *testing.T) { value := dnd.ItemOccurrenceList{Occurrences: make([]dnd.ItemOccurrence, 24)} for index := range value.Occurrences { - value.Occurrences[index] = dnd.ItemOccurrence{ItemID: "item", Name: " \n", Kind: "unsupported"} + value.Occurrences[index] = dnd.ItemOccurrence{ItemID: "item", Name: fmt.Sprintf("Item %d", index), Kind: "unsupported"} } result, err := New(Options{}).Validate(context.Background(), contracts.TypedValidationRequest[dnd.ItemOccurrenceList]{Value: value}) if err != nil || result.Approved || result.ReasonCode != ReasonCode || len([]byte(result.Message)) > 4096 || !utf8.ValidString(result.Message) || !strings.Contains(result.Message, "occurrences[0]") || !strings.Contains(result.Message, "additional issue(s) omitted") { t.Fatalf("Validate() = %#v, %v", result, err) } - if value.Occurrences[0].Name != " \n" { + if len([]byte(result.CorrectionGuidance)) > 4096 || !utf8.ValidString(result.CorrectionGuidance) || !strings.Contains(result.CorrectionGuidance, "additional issue(s) omitted") { + t.Fatalf("CorrectionGuidance = %q, want bounded aggregate", result.CorrectionGuidance) + } + if value.Occurrences[0].Name != "Item 0" { t.Fatal("Validate() mutated input") } if got := New(Options{}).CheckpointFingerprints(); !reflect.DeepEqual(got, []pipeline.CheckpointFingerprint{{Name: "policy", Value: policy}}) {