diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 8bf8f05..926c92f 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,7 +1,7 @@ # Sequence Implementation Review Remediation -Status: remediation in progress. Completed findings are marked below; this -record introduces no new release gate. +Status: all eleven findings complete. Each finding has a separate remediation +commit and completion note below. This record introduces no new release gate. Reviewed commit: `0be7cdb00eff576fda6b30c0600b2134d4e2e086`. Implementation range: `bc6b977..0be7cdb`. @@ -25,6 +25,10 @@ Validation on the reviewed commit: - Finding 10 follows directly from the maintained text; finding 11 follows from the prefix-scan loop, not a timing comparison or performance threshold. +Final validation after remediation: `./scripts/check.sh` and +`go test -race ./...` passed with Go 1.26.10 on Linux. The original public-API +probes for findings 1–9 now demonstrate the required corrected behavior. + P1 denotes the highest-priority correctness issue here; P2 denotes normal-priority work needed to complete the selected behavior. Preserve the agreed scope while fixing these issues. In particular, do not resolve failures by restricting valid @@ -376,7 +380,16 @@ Keep YAML-only enforcement distinct from registered final validation. ## 11. Remove The Quadratic Prefix Scan In Item Policy Projection — P2 -Status: [ ] Open. +Status: [x] Complete. + +Completion: assigned sequence ranges now supply item offsets directly. Handoffs +without a range lazily accumulate prefix lengths, visiting each preceding +contribution once rather than rescanning every prefix. Regression coverage keeps +inline and policy-less contributions in the correct positions with complete, +partial, or absent ranges. Existing ownership/placement tests remain intact. +Verified focused item-policy tests, `./scripts/check.sh`, `go test -race ./...`, +and inspection of the remaining error-only prefix-scan callers. No new benchmark +fixture or performance threshold was needed for this bounded change. Owner: [`projectNestedCollectionPolicies`](../../internal/engine/imports.go), especially line 968; the scanned helper is `priorSequenceLength` in the same file. diff --git a/internal/engine/imports.go b/internal/engine/imports.go index a9eb140..f44c294 100644 --- a/internal/engine/imports.go +++ b/internal/engine/imports.go @@ -961,17 +961,28 @@ func projectNestedCollectionPolicies(composed *composedDocument, scope acquisiti return } baseOffsets := make(map[string]int) + var prefixLengths map[string]int for sourceIndex, contribution := range scope.contributions { + if prefixLengths != nil && sourceIndex > 0 { + addSequencePrefixLengths(scope.contributions[sourceIndex-1].document, Path{}, prefixLengths) + } if contribution.nested == nil || len(contribution.nested.collectionPolicies.collections) == 0 { continue } - itemIndex := priorSequenceLength([]acquisitionScope{{contributions: scope.contributions[:sourceIndex]}}, contribution.destination) - if assigned, found := ranges[sequenceRangeKey{sourceIndex: sourceIndex, path: contribution.destination.String()}]; found { - if projected, ok := assigned.outputForInput(0); ok { - itemIndex = projected - } - } key := contribution.destination.String() + assigned := ranges[sequenceRangeKey{sourceIndex: sourceIndex, path: key}] + itemIndex, found := assigned.outputForInput(0) + if !found { + // Normal composition supplies the assignment directly. For a + // handoff without ranges, scan each preceding tree only once. + if prefixLengths == nil { + prefixLengths = make(map[string]int) + for _, previous := range scope.contributions[:sourceIndex] { + addSequencePrefixLengths(previous.document, Path{}, prefixLengths) + } + } + itemIndex = prefixLengths[key] + } base, initialized := baseOffsets[key] policy, _ := composed.collectionPolicies.at(contribution.destination) if !initialized { @@ -986,6 +997,20 @@ func projectNestedCollectionPolicies(composed *composedDocument, scope acquisiti } } +func addSequencePrefixLengths(node *document.Node, path Path, lengths map[string]int) { + if node == nil { + return + } + if node.Kind == document.Sequence { + lengths[path.String()] += len(node.Sequence) + return + } + // Contribution destinations select mapping paths, not sequence members. + for _, entry := range node.Mapping { + addSequencePrefixLengths(entry.Value, path.Key(entry.Key), lengths) + } +} + func projectNestedOrigins(target map[string]Origin, root *document.Node, origins map[string]Origin, prefix Path) { var visit func(*document.Node, Path) visit = func(node *document.Node, path Path) { diff --git a/internal/engine/item_policy_test.go b/internal/engine/item_policy_test.go index 14b5302..e4d098c 100644 --- a/internal/engine/item_policy_test.go +++ b/internal/engine/item_policy_test.go @@ -195,6 +195,40 @@ func TestNestedItemPolicyProjectionUsesStableOutputIndexes(t *testing.T) { } } +func TestItemPolicyProjectionCountsContributionsWithoutLocalPolicy(t *testing.T) { + t.Parallel() + items := Keys("items") + for _, assignments := range []map[int]int{nil, {1: 2, 3: 4}, {1: 2}, {3: 4}} { + t.Run(fmt.Sprint(assignments), func(t *testing.T) { + local := collectionPolicyScope{} + local.set(Keys("steps"), collectionPolicy{identityFields: []string{"name"}, identityPresent: true}) + scope := acquisitionScope{contributions: []acquiredContribution{ + {document: parseItemPolicyDocument(t, "inline", "items: [{id: inline-a}, {id: inline-b}]\n")}, + {document: parseItemPolicyDocument(t, "first", "items: [{id: first}]\n"), destination: items, nested: &composedDocument{collectionPolicies: local}}, + {document: parseItemPolicyDocument(t, "gap", "items: [{id: gap}]\n"), destination: items}, + {document: parseItemPolicyDocument(t, "second", "items: [{id: second}]\n"), destination: items, nested: &composedDocument{collectionPolicies: local}}, + }} + ranges := make(map[sequenceRangeKey]sequenceRange) + for sourceIndex, output := range assignments { + ranges[sequenceRangeKey{sourceIndex: sourceIndex, path: items.String()}] = sequenceRange{start: output, end: output + 1} + } + composed := &composedDocument{} + projectNestedCollectionPolicies(composed, scope, ranges) + policy, _ := composed.collectionPolicies.at(items) + if len(policy.recordOrder) != 5 { + t.Fatalf("record policies = %d, want 5 positions", len(policy.recordOrder)) + } + for index := range 5 { + record, _ := policy.recordPolicyAt(index) + _, found := record.scope.at(Keys("steps")) + if found != (index == 2 || index == 4) { + t.Errorf("local policy at item %d = %t", index, found) + } + } + }) + } +} + func TestOverlaySequenceTransfersIncomingItemPolicyOwnership(t *testing.T) { path := Keys("records") registered := mustCompileSequenceIdentityPolicy(t, []sequenceIdentityRegistration{{selector: sequenceIdentitySelector{literalSequenceSelector("records")}, identityFields: []string{"id"}}})