From 9c32b8d3b53154f50f47b83b5a5f1015a083b1b6 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 24 Oct 2026 01:46:55 +0000 Subject: [PATCH] Record sequence algorithm audit findings --- docs/roadmap/remediation.md | 201 +++++++++++++++++++++++++++++++++++- 1 file changed, 198 insertions(+), 3 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5558f0b..3203ba1 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,8 +1,8 @@ # Sequence Implementation Review Remediation -Status: all findings 1–17 are complete. The subsequent integration audit at -`8d40983271de5239312eb0a2896737be88c5abdc` identified findings 12–17, now -remediated below. This record introduces no new release gate. +Status: findings 1–17 are complete. The algorithm audit below identifies six +additional open findings, numbered 18–23. This record introduces no new release +gate. Reviewed commit: `0be7cdb00eff576fda6b30c0600b2134d4e2e086`. Implementation range: `bc6b977..0be7cdb`. @@ -708,3 +708,198 @@ link to the canonical operation tables. Preserve the existing atomic sequence snapshot/diff behavior and whole-record upsert semantics; correcting the prose does not authorize recursive record-field merging, a new query API, or broader history guarantees. + +## Algorithm Audit Baseline And Scope + +Reviewed commit: `c73daf891d0f29c96f7026bdabcccb571f44a751`. +Feature baseline: `4f941f8ba4c9a9ed0afa87b07fe6ba29f131d1c6`. + +This audit examines the sequence identity/operation, scoped-policy, inheritance, +inspection, and post-decode CLI algorithms, including the completed corrections +above. The intervening discovery roadmap commits add no implementation to audit. +The accepted scheduler, deterministic ordering, exact numeric normalization, +selector trie, and deliberately pairwise registration-overlap validation remain +selected design choices; they are not findings here. + +Evidence and validation: + +- Code inspection establishes the complexity of findings 19–23 on bounded-depth + inputs with linear-sized results. Required result/history copies and genuinely + growing inheritance output are excluded from the alleged excess work. Bounds + describe internal operations, not new public performance guarantees. +- An isolated public-API probe reproduces finding 18 using an in-memory + filesystem, without external services or real credentials. +- Temporary tests supplied through a Go overlay, outside the repository, checked + all 768 combinations of append/upsert/replace over the 16 unique-ID lists drawn + from three IDs. Stable ordering, duplicate rejection, and whole-record payload + replacement agreed with an independent reference calculation. +- A structural overlay test confirmed that 4, 8, and 16 one-item contributions + retain 10, 36, and 136 reverse-index cells respectively in `indexSequenceRanges`. + These are representation counts, not timing or real-world benchmark results. +- `go test ./internal/engine -run 'Sequence|Collection|Inheritance|Override|SourceVersion|Metadata|History' -count=1` + passed with Go 1.26.10 on Linux. Passing existing tests does not cover the + incorrect selector scope or establish efficient growth. + +The following are focused corrections within the selected model. This review +changes only the remediation record. + +## 18. Match Application Registrations In Effective Item Coordinates — P2 + +Status: [ ] Open. + +Owner: `composePendingItems` in +[`imports.go`](../../internal/engine/imports.go), especially its local +`composeDocumentsWithCollectionPolicy` call; the current +`TestPendingItemCompositionAppliesApplicationIdentityConstraints` in +[`item_policy_test.go`](../../internal/engine/item_policy_test.go) also needs review. + +Pending item/helper documents are composed in item-local coordinates while the +unchanged global application selector policy is supplied to their composer. +A registration for root `[prompts]` therefore also constrains an unrelated +`[items, 0, prompts]` collection before placement. This contradicts complete, +effective-destination matching and makes an otherwise unused registration reject +valid configuration. Application selectors do not traverse sequence indexes. + +Reproduction: main imports `item.yml` with destination `[items]` and placement +`sequence-item`. The item contains `id: outer` and +`prompts: [{text: payload}]`. Loading succeeds without a registration; adding +only `SequenceIdentityAt(Keys("prompts"), "id")` fails with `compose.invalid` +at `/items/0/prompts/0/id`, although no root `prompts` collection exists. + +Required remediation: separate item-local YAML policy from application matching +in effective coordinates, including nested helper scopes. Preserve applicable +outer collection constraints without reinterpreting global selectors as local +ones. Add ordinary/layered regressions for unused root and dynamic selectors +alongside imported item-local collections. Correct tests that pin the accidental +scope; do not change selector meaning or require consumers to remove valid rules. + +## 19. Avoid Dense Global-Position Storage For Sparse Item Projections — P2 + +Status: [ ] Open. + +Owners: `newSequenceItemProjectionIndex` in +[`sequence_projection.go`](../../internal/engine/sequence_projection.go), +`indexSequenceRange` / `projectedCompositionOrigins` in +[`compose.go`](../../internal/engine/compose.go), and contribution indexing in +[`history.go`](../../internal/engine/history.go). + +The reverse projection slice is sized to `maxOutput + 1`, even for one incoming +item assigned to a late assembled position. `indexSequenceRange` materializes +these indexes for ordinary contiguous append ranges too. With N one-item sources +contributing to one sequence, reverse slices retain `1 + 2 + ... + N` cells. +Initialization time and simultaneously retained auxiliary storage are therefore +quadratic despite only N actual mappings. Origin/history projection constructs +the same representation through additional callers. Finding 16's policy-copy +correction does not remove this independent cost. + +Required remediation: retain arithmetic start/end lookup for contiguous ranges +and use storage proportional to actual mappings for scattered positions, such as +a sparse reverse map. Audit shared callers and avoid rebuilding equivalent indexes +inside item queries. Preserve bidirectional lookup, absent positions, upsert +replacement evidence, ordering, and detached results. Verify the representation +on many one-item sources and noncontiguous upserts; add no timing threshold or +collection-size limit. + +## 20. Remove Full Metadata Scans Per CLI Replacement Leaf — P2 + +Status: [ ] Open. + +Owners: `updateOverridePresence`, `deletePresencePathSubtree`, and +`updateOverrideOrigins` in +[`override_decode.go`](../../internal/engine/override_decode.go). + +Every replaced scalar or subtree scans all presence keys, then all origin keys, +to find matching prefixes. A single partial-mapping override updating N scalar +children in an N-field configuration performs quadratic map-entry visits. This +does not require N CLI sources or growing historical output: the same effective +maps remain approximately N entries throughout the update. + +Required remediation: delete affected entries by walking the already known +existing subtree, or use one batch-local prefix index where needed. Retain +necessary copies at ownership boundaries without rescanning unrelated paths for +each leaf. Preserve retained explicit zero/false values, removal of replaced +descendants, null/kind changes, origin attribution, and detached loads. Use the +existing nested CLI and ownership regressions plus focused broad-mapping coverage; +do not change override semantics or impose batch limits. + +## 21. Localize Inheritance Policy Work To The Relevant Scope — P2 + +Status: [ ] Open. + +Owners: `inheritancePolicyScopeAtPath` in +[`inheritance.go`](../../internal/engine/inheritance.go), graph preparation in +[`inheritance_schedule.go`](../../internal/engine/inheritance_schedule.go), +and policy handoff/retirement in +[`inheritance_walk.go`](../../internal/engine/inheritance_walk.go). + +Scope localization scans every entry in the supplied policy table for each +definition. Mapping graph preparation also clones the full supplied table, and +the walker passes its global table to each independent scope and scans that table +again during replacement. For S scopes or siblings and P unrelated collection +policies, these paths perform repeated S-by-P work and copies. + +For example, N independent mapping groups can each contain a constant-sized +`base`/`child extends base` pair and one YAML collection declaration beneath its +base. Input, expanded values, and relationship evidence remain linear in N, but +each group's graph/localization visits or clones the other groups' policies. +This is separate from necessary expansion along a genuinely growing parent chain +and from the selected dependency scheduler. + +Required remediation: partition or index policies by semantic scope once and +carry the relevant state through traversal, including efficient descendant +retirement as inheritance introduces or removes destinations. Clone owned +in-scope state rather than unrelated global state. Preserve record owners, +pending declarations, mode lifetime, destination-local registration matching, +and independent templates. Verify wide independent groups and nested item scopes; +do not replace the agreed scheduling algorithm or add global caches. + +## 22. Index Fallback Location Lookup During Effective Projection — P2 + +Status: [ ] Open. + +Owner: `documentWithMetadataLocations` in +[`override_context.go`](../../internal/engine/override_context.go), using +`nearestDocumentLocation` in +[`validation.go`](../../internal/engine/validation.go). + +The new effective-projection walk calls the unindexed nearest-location helper +for each field lacking a real origin location, often once for its mapping entry +and again for its value. Each call starts at the fallback root and linearly scans +mapping entries. With N authored root fields and N different default-only root +fields, each default's failed lookup scans all N authored keys: quadratic work +for linear-sized input and output. The shared lifecycle calls this path even +without CLI assignments or a resolver. + +Required remediation: share an indexed fallback lookup or traverse corresponding +containers together while carrying the nearest available location. Existing +`documentLookup` / mapping-index machinery can support bounded-depth lookups +without repeated wide scans. Preserve nearest-ancestor fallback, source IDs, +synthetic locations, secret-reference diagnostics, and publication ownership. +Exercise mixed authored/default-only broad mappings; do not weaken attribution +or skip needed location attachment to avoid the work. + +## 23. Index Repeated Collection-Policy Document Lookups — P2 + +Status: [ ] Open. + +Owners: `mergeOverlayMappingsWithCollectionPolicy` in +[`overlay.go`](../../internal/engine/overlay.go) and +`mergeInheritanceCollectionPolicies` in +[`inheritance_collection.go`](../../internal/engine/inheritance_collection.go). + +These routines loop over collection policies/declarations and call plain +`documentAtPath` for each destination. That helper linearly searches every +mapping on the path. N independent collections below N sibling mapping keys +therefore cause quadratic mapping-entry searches when checking which lower +policies the incoming tier touches. The inheritance parent/child lookups and +retained-value declaration checks have the same pattern. YAML declarations alone +can establish these policies; the deliberately pairwise application-registration +overlap check is not involved. + +Required remediation: reuse a document lookup built once for each stable input +tree, or use the already collected claims where their scope is sufficient. +Index a mutated result only after its value-changing phase, rather than retaining +a stale lookup. Preserve absent destinations, non-sequence pending declarations, +literal key/index distinctions, retirement, and error locations. Verify broad +independent collections across precedence and inheritance; no new public cache, +shape requirement, or performance gate is needed.