You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "MicroGery (via GitHub)" <gi...@apache.org> on 2026/09/09 08:44:52 UTC
[PR] refactor(storage): isolate local persistence contracts [maka]
MicroGery opened a new pull request, #5090:
URL: https://github.com/apache/maka/pull/5090
## Summary
Refs #2370. This is PR1 of the 2026-09-09 local Runtime follow-up plan, not the earlier SessionRepository PR.
- Separate Session contracts/domain errors and Tool T1/T2 DTOs from SQLite implementations. Route the remaining Host consumers through the existing execution-store seam; preserve lease/branding, lifecycle, transaction ownership and compatible exports.
- Verify real Host recovery after a WorkHub assignment commits but before the target admission is consumed. Cover `create_new` and `delegate_existing`, with/without attachments, exact message/target identity, client retries, conflicts and all-or-none rollback.
- Fix the attachment mismatch exposed by that path: the transaction previously compared admission content with text only, rejecting copied attachments while allowing omissions. Validate ordered metadata and source/target ownership, and bind source attachments into replay equality.
SQLite remains the live-state authority. No schema migration, checkpoint I/O, remote adapter or WorkHub routing/UI redesign is included. The crash test establishes the before-first-target-dispatch window, not exactly-once arbitrary external effects. The English/Chinese architecture notes document these boundaries.
## Verification
Clean-built Core, Storage, MCP, Runtime and Runtime Host on macOS with Node 24.18.0.
| Suite | Passed | Skipped | Failed |
| --- | ---: | ---: | ---: |
| Storage | 1,220 | 8 | 0 |
| Runtime | 3,349 | 13 | 0 |
| Runtime Host | 1,841 | 12 | 0 |
- New contract/dependency and Host subprocess tests: 10 passed; attachment regressions were reproduced before the fix.
- Scoped Biome lint/format, Storage/Host typecheck, changed-file ASF headers, Windows skip inventory and `git diff --check` passed.
- No Windows execution or full monorepo/UI validation was run. Small same-machine storage timings were similar to the base, but are not a comprehensive performance gate.
## AI use
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: OpenAI Codex — implementation, tests, documentation, verification and PR preparation. Human review and merge remain required. The commit carries a `Generated-by: OpenAI Codex` trailer.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
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
Re: [PR] refactor(storage): isolate local persistence contracts [maka]
Posted by "likun666661 (via GitHub)" <gi...@apache.org>.
likun666661 commented on PR #5090:
URL: https://github.com/apache/maka/pull/5090#issuecomment-5599476513
## Architecture review: moving contracts is not backend substitutability
Reviewed at `3d07d301`, specifically against this question: **can Memory/Mock, Local, and S3-backed implementations serve the same upper-layer code without backend-specific branches?**
**Not yet. This PR improves dependency hygiene, but does not establish a replaceable persistence boundary.** The description explicitly excludes a remote adapter, so the points below are gaps against that broader architecture goal, not claims of newly introduced local regressions.
### 1. The actual composition boundary still only admits the local implementation
[`execution-stores.ts`](https://github.com/apache/maka/blob/3d07d301/packages/storage/src/execution-stores.ts#L266) authenticates stores through a private registry, while its construction path hardcodes `createSessionStore(lease.canonicalPath)`, `createSqliteAgentRunStore(...)`, and local runtime persistence. [Host composition](https://github.com/apache/maka/blob/3d07d301/packages/runtime-host/src/server/execution-composition.ts#L243) has no storage-provider injection point.
A structurally conforming implementation is therefore insufficient: a separately constructed complete store fails authentication. Narrow `Pick<...>` mocks are possible; replacing the complete Host persistence backend is not.
Keep the lease/authentication protections. But let trusted composition construct an alternative backend and wrap it in the same authenticated lifecycle facade. **If even a stateful in-memory implementation cannot enter through the supported composition path, S3-readiness is premature.**
### 2. The hard part is preserving transactions, not extracting DTOs
The atomic `assignWorkHubMessage` operation is the right abstraction: coordination linkage, target admission, optional target creation/supersession must remain one commit. Tool T1/T2 likewise require inseparable event/journal commits.
Do not weaken these into CRUD to make an object-store adapter look easy. S3 provides strong consistency and conditional writes, not native multi-key transactions ([AWS](https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html)). Independent PUTs to per-Session objects cannot satisfy this contract.
An S3-backed implementation needs an explicit commit protocol, such as immutable state plus a CAS-published manifest covering the transaction's full scope, or a transactional metadata service. A separate head per Session does not solve a cross-Session commit.
### 3. The extracted interface still carries the current implementation's organization
[`SessionAuthorityStore`](https://github.com/apache/maka/blob/3d07d301/packages/storage/src/session-store-contract.ts#L360) combines domain commands with legacy/runtime transcript-index maintenance, retirement cleanup, and change subscriptions.
Some of these are legitimate capabilities, but exposing the current legacy/runtime index machinery obliges every new backend to understand that representation. Move rebuildable indexing/migration concerns behind the implementation where possible, and expose narrow consumer-facing capabilities. Also specify whether transcript subscriptions observe this instance's commits or remote writers.
Numeric revisions and event ordinals are not inherently SQLite leaks. The concern is semantic responsibility, not cosmetic type purity.
### 4. The tests establish local correctness, not substitutability
The new contract tests instantiate the real local composition and inject failures with SQLite triggers. The dependency guards inspect imports. Both are useful; neither demonstrates that an independent implementation satisfies the same contract.
The minimum convincing next step is:
- A supported provider factory, with no SQLite construction required by the in-memory backend.
- One shared behavioral contract suite run against Memory and Local.
- Atomic visibility, identity conflicts, idempotent replay, lost commit acknowledgements, snapshot semantics, and closed-owner rejection covered by that suite.
- A Host integration test using the alternative persistence provider without changing coordinators.
- An explicit remote ownership/fencing and commit design before claiming S3-backed live-state support.
**Bottom line:** this is a reasonable local dependency cleanup. It is not yet evidence of a clean, interchangeable storage architecture. Keep live transactional state, immutable object storage, and checkpoint publication as distinct contracts; do not conflate “can publish checkpoints to S3” with “can replace live SQLite with S3.”
Static architecture review only; I did not rerun the tests. This review 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
Re: [PR] refactor(storage): verify replaceable execution persistence [maka]
Posted by "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
Re: [PR] refactor(storage): verify replaceable execution persistence [maka]
Posted by "MicroGery (via GitHub)" <gi...@apache.org>.
MicroGery merged PR #5090:
URL: https://github.com/apache/maka/pull/5090
--
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
Re: [PR] refactor(storage): verify replaceable execution persistence [maka]
Posted by "likun666661 (via GitHub)" <gi...@apache.org>.
likun666661 commented on PR #5090:
URL: https://github.com/apache/maka/pull/5090#issuecomment-5615753657
## Follow-up review at `bcc0ca2e`
The two previous catalog findings are addressed: preparing copies are hidden from catalog reads while remaining available for recovery, and pagination now uses the same ASCII comparison for sorting and cursor advancement. The shared publication and mixed-case pagination tests are appropriate coverage. Those two findings can be closed.
One further consumer-visible mismatch remains:
### [P2] Memory's unfiltered catalog excludes subagent Sessions, unlike Local
In [`selectCatalog()`](https://github.com/apache/maka/blob/bcc0ca2e/packages/storage/src/test-only/memory-execution-session.ts#L1222), the missing-filter branch evaluates `!r.header.subagentParent`. Thus both `list()` and `listCatalogPage()` silently exclude every subagent Session when `subagentParentSessionId` is omitted.
Local only adds a parent predicate when the filter is explicitly supplied: see the [paginated query](https://github.com/apache/maka/blob/bcc0ca2e/packages/storage/src/sqlite-session-catalog-query.ts#L46) and [list predicate](https://github.com/apache/maka/blob/bcc0ca2e/packages/storage/src/sqlite-session-metadata-store.ts#L5824). An unfiltered Local catalog does not exclude otherwise eligible subagent Sessions.
This reaches an actual consumer: [Host catalog queries](https://github.com/apache/maka/blob/bcc0ca2e/packages/runtime-host/src/server/session-catalog-coordinator.ts#L407) pass `undefined` as the filter. Selecting Memory therefore changes the returned Session inventory without changing the request or coordinator.
Please make absence of the filter mean “no parent restriction,” matching Local. Add shared Local/Memory cases with two parent Sessions and their children, verifying:
- Unfiltered `list()` and a complete `listCatalogPage()` traversal include all otherwise eligible parent and child Sessions.
- An explicit parent filter returns only that parent's children.
- Pagination neither loses nor duplicates children.
The new catalog tests create ordinary Sessions but do not exercise parent filtering, so they do not catch this difference.
This is a mismatch in the new Memory reference, not evidence of a regression in default Local behavior. The supported substitution entry point is now established; the remaining issue is behavioral conformance.
Verification: static incremental review only; I did not rerun project tests. 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