Re: [PR] refactor(storage): verify replaceable execution persistence [maka]

["likun666661 (via GitHub)" <gi...@apache.org>]

likun666661 commented on PR #5090:
URL: https://github.com/apache/maka/pull/5090#issuecomment-5613846464

   ## Follow-up architecture review at `e3ae678b`
   
   The provider injection, independent Map implementation, shared conformance tests, and real Host substitution test materially address the previous review. **I withdraw the earlier objection that a complete Memory backend cannot enter through supported composition.** Keeping Graph and Goal in the same consistency domain is also the right direction.
   
   There are still two concrete behavioral mismatches in the reference implementation:
   
   ### [P2] Unpublished conversation copies become visible in Memory catalogs
   
   [`selectCatalog()`](https://github.com/apache/maka/blob/e3ae678b/packages/storage/src/test-only/memory-execution-session.ts#L1205) does not exclude `conversationCopy.state === 'preparing'`. Both `list()` and `listCatalogPage()` use this helper, so a target whose copy is still being prepared can appear in ordinary catalog results.
   
   Local explicitly excludes these records in [list()](https://github.com/apache/maka/blob/e3ae678b/packages/storage/src/session-store.ts#L572) and the [paginated SQL query](https://github.com/apache/maka/blob/e3ae678b/packages/storage/src/sqlite-session-catalog-query.ts#L39).
   
   Please preserve the same publication visibility rule in Memory and add shared tests for a preparing copy being absent, then becoming visible only after publication. This is an observable difference for unchanged consumers, not just an internal representation difference.
   
   ### [P2] Memory pagination uses incompatible sort and cursor comparisons
   
   Memory [sorts equal-activity entries with `localeCompare()`](https://github.com/apache/maka/blob/e3ae678b/packages/storage/src/test-only/memory-execution-session.ts#L1215), but [advances the cursor with `id > cursor.sessionId`](https://github.com/apache/maka/blob/e3ae678b/packages/storage/src/test-only/memory-execution-session.ts#L507).
   
   Those are different orderings. With equal activity timestamps, a simple Node reproduction sorts IDs as `a, A, b, B`; after returning `a`, the cursor predicate excludes `A` and advances to `b`. This can skip records even when the catalog is unchanged. Local's SQL ordering and cursor predicate use consistent comparisons.
   
   Please use one deterministic ordering compatible with the Local contract for both sorting and cursor advancement. Add a shared pagination test with equal timestamps, mixed-case IDs, and page size 1, asserting that traversing every page returns each expected Session exactly once.
   
   ### Updated assessment
   
   - **Supported Memory/Local composition:** addressed.
   - **Behavioral equivalence:** substantially improved, but the catalog cases above are missing from the shared suite.
   - **Representation independence:** still intentionally bounded by legacy/runtime transcript-index compatibility operations.
   - **S3-backed live state:** now has an injection point, not a qualified implementation. Transaction publication and remote ownership/fencing remain separate work.
   - **Whole-application storage substitution:** still out of scope; Artifact payloads, runtime policy and other independently composed domains remain local, as the updated description correctly states.
   
   These findings concern the new Memory reference, not evidence of a regression in default Local behavior. The architectural progress is real; the remaining requirement is to make the reference and conformance suite accurately enforce the consumer-visible contract.
   
   Verification: static review plus a small Node reproduction of the comparator mismatch; I did not rerun the project suites. This comment was prepared and posted with OpenAI Codex assistance.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org