Repository navigation
v1.228 spoke-memory Stage N2: entry-point convergence (cutover) - #469
Merged
Merged
Conversation
Add `memory_provider/legacy.rs`, the single module that projects the legacy memory surfaces as provider-native rows so one `MemoryPort` read path serves both the provider's own content and unmigrated legacy content (iteration v1.228-spoke-memory-n2, plan v228-n2-nxm5-coreservice-reroute Task 1). Read-only by construction: every statement is a SELECT on the caller's read transaction, no legacy table/file/schema is written. Projections: - records: `memory_fragments`/`character_memory_fragments` (fact), `*_soul_narratives` (nexus.soul_narrative), `memory/long-term/*.md` (authored kind via policy::namespaced_record_kind), `character_run_captures` (event, binding placement) - candidates: `*_pending_review` as pending review-queue entries - profiles: `SOUL.md` as the `personality` document External ids are `lgc:<source family>:<escaped primary key>` — derived, never random — so repeated reads and P5's migration key agree, and a migrated object's provider row always wins over its projection. `storage.rs` composes the projections into `records`, `record_for_read`, `candidates`, `candidate_for_read` and `profiles`; merged paging fetches a provider prefix that cannot omit a page row, applies the same selection/kind/provenance predicate to projected rows, and digests projected identities into the page marker. `mod.rs` opens the store with the bearer roots and exposes the two subject-kind namespaces. Merged-read coverage added to `tests/memory_services.rs` (records with provider-native + projected legacy rows, external-id stability, read-only file bytes, legacy SOUL profile, legacy pending queue, Character binding placement + ownership guard).
…ubject, projection digest (NXM-5 Task 1 F1-F3) - F1: every projection entry point (records list, record by-id, candidates list/by-id, profiles) reads `legacy_content_migration:<space_id>` from `memory_provider_meta` in the caller's read transaction and projects nothing once P5 publishes it. - F2: a bearer projects under exactly one canonical subject (smallest subject_id naming it); list, by-id and profile reads resolve the same subject. - F3: projected records carry a MEM-R026 version digest of the values they serve, folded into the page snapshot marker, so a bearer file rewritten between pages invalidates the cursor. Adds regression tests for marker cutover, canonical subject consistency, and cursor invalidation on an in-place file rewrite.
…XM-5 Task 2) Pin the LocalMemoryProvider in CoreInner for the open's lifetime (built beside the pool admission, released by the close drain) and reroute the E4 memory family onto MemoryPort: * creator + Character pending-review list/count and fragment list serve the provider's merged read; DTOs, keyset/offset pagination and error variants are preserved by the adapter (compass D5) * pending-review dismissal adopts the legacy-derived row into provider storage under the bridge's deterministic lgc id, then dismisses it through memory.candidates.dismiss - idempotent, marker-gated, legacy tables write-idle * the bridge exports the deterministic E4 subject ids and pending-review candidate ids P5's migration key shares Capture, the review drains and fragment promotion stay on their current paths: P2 (NXM-16) and P4 (NXM-8) own them, and moving them in P1 would orphan rows the still-legacy review drain consumes. Record adoption lands with P4's record mutation path.
…on, unbounded retained reads, read-only memory reads (NXM-5 Task 2 F1/F2/F4/F5/F6)
F1: the legacy review drains check the provider candidate state for each
fetched row's bridge identity and skip consumed rows before classification,
so a provider-dismissed digest can never become a memory again; the page walk
continues past consumed rows so has_more stays truthful.
F2: a cross-Creator pending_id answers the retained
ForbiddenReason { resource: "pending_review" } again, decided by the same
read-only legacy ownership lookup the pre-reroute handler used.
F4: adoption and dismissal are one guarded transaction. MemoryStore gains
dismiss_adopting (shared with the trait dismissal), which authorizes the
projection, adopts under the bridge's deterministic id and consumes in the
same BEGIN IMMEDIATE transaction, under one journalled receipt.
F5: the blanket 10,000-row scan cap and its silent truncations are gone.
Pending list/count walk the provider's own bounded pages to the end, the
Character offset window is served as asked (no false has_more), and the
keyword filter reads the whole subject set before applying its predicate.
F6: CoreAccess::ReadOnly pins a read-only provider (mode=ro, no admission,
no provisioning, no journal recovery) so memory reads keep working; write
handlers refuse with the typed Forbidden before reaching it.
Regressions: dismissed Creator/Character digests skipped by the drain,
cross-Creator ForbiddenReason, adoption+dismissal atomicity (commit fault and
CAS failure roll back both halves), listings/offset/keyword past the former
10k cap, read-only read + write refusal.
…nly bearer projection, read-only write refusal (NXM-5 Task 2 F1/F2/F3 round 2) R2-F1: a provider dismissal and the legacy review drain decide the same digest, so both take one exclusive workspace-file section per bearer. The drain's bounded fetch stays outside it; the slice is re-checked inside the section and a row a dismissal consumed in between is dropped instead of classified, and the classification/persistence run under the same section the dismissal must hold. Deterministic step-points plus a test-only section probe pin both directions in regressions for the Creator and Character bearers. R2-F2: a bearer no writer ever ensured is projected under its deterministic subject id without registering anything, so a read-only open serves its legacy pending rows, fragments and profile again; a workspace whose schema predates the provider answers the space's absence instead of a driver error. R2-F3: the store refuses a mutation on a read-only open at both mutation gates (admission and publication section) before retention, the publication lock or any connection write, answered as a typed NOT_AUTHORIZED refusal.
…safe disposition lock names, deterministic lock proofs (NXM-5 Task 2 F1-F5 round 3)
…in-lock wait proof (NXM-5 Task 2 R4-F1/F2/F3 round 4)
…Task 2 R5-F1 round 5)
…ation (NXM-5 Task 3) Four gaps left by the Task 1/2 suite, plus the D8 self-check: - the P5 completion marker is per space: a marker for one space closes every projection of that space (both bearers) and leaves another space serving the same deterministic external ids; - the wire `dismiss_candidate`/`review_candidate` entries are not adoption paths: a projected legacy id is absent to them, and once the E4 dismissal has adopted and consumed it they see a provider-native consumed row; - a record cursor minted over the merged set stays valid across that adoption (the dismissal writes only the candidate family), while a candidate cursor is refused because its authorized set really changed; - a marker published while a dismissal is parked in its publication section is re-read inside the admission transaction, so the dismissal adopts nothing, consumes nothing and returns the retained `NotFound`. `--test memory_services` is already registered in the `rust-core-domain-owners` CI leg (ci.yml:894, no job timeout), so ci.yml is unchanged.
…marker regressions (NXM-5 Task 3 fix round 1)
…ailures (NXM-5 QC fix round 1) QC1-F1: projections keep the legacy column's timestamp precision and carry the original string for E4 DTO values; merged reads order by parsed instants, and adoption still writes the provider's fixed whole-second rendering. QC1-F3: the dismissal ownership re-read propagates as the retained database_error internal instead of a false NotFound. S-1: CoreInner::memory doc matches the landed read-only provider pinning. Adds the opposite-order sub-second pagination and lookup-failure regressions.
…p parser (NXM-5 QC1-F1 fix round 2)
…eads+dismissal onto MemoryPort, legacy read-bridge, disposition locking, QC fix rounds
…or capture (NXM-6/NXM-16) E1/E2 reroute (NXM-6): - new CoreService Creator long-term memory family (list/get/create/update/ delete by authored slug, SOUL personality profile get/set, deterministic experience refresh, personality push-down) over the pinned MemoryPort, returning the upstream spoke contract objects - creator memory/soul leaves and platform stage-0 assembly are CoreService clients now; no memory_io/soul_io import remains under apps/nexus42/src - shared direct-core seam helpers (writer + read-only) in the CLI core module Creator explicit capture (NXM-16): - CoreService::create_pending_review: shared placement, world_id provenance, session anchor, task-kind -> MemoryKind kind hint, retained capture fields in content.structured.nexus_legacy, stable idempotency key - provider-native pending rows are dismissible by pending_id - napi create_pending_review (active-creator equality), POST /v1/daemon/memory/pending-review (201) and the native TS binding - creator memory capture CLI leaf - new crates/nexus-core/tests/creator_capture.rs round trip UNVERIFIED: full build/test of this state was cut short by the run budget.
… capture
First creator_capture run: Internal { category: "character_tom_wire_invalid:
missing field `extensions`" }. SourceAnchor.extensions is a non-defaulted
map in the wire schema, so the session anchor must carry an empty bag.
cargo test -p nexus-core --test creator_capture -> 3 passed.
- capture timestamp default folds into map_or_else - personality upsert uses if-let/else - soul_error is the map_err adapter (needless_pass_by_value allow) - stage0 projection assigns the body through clone_from
…all (NXM-7) E10 now consumes the upstream contract DTO: Stage0Assembly/TwoStageAssembly carry Vec<RetainedRecord> instead of the legacy LongTermMemory file type, and the shared recall seam partitions a recall result set into the nine nexus.<MemoryKind> long-term memories, the fact fragment family (summary + lexus_legacy keywords) and nothing else. The legacy memory-file parser is no longer a dependency of the crate. E6 reads through the same seam: the Character mind projection addresses the provider subject the NXM-5 read bridge projects the bearer under and serves SOUL from memory.profile.get plus fragments/promoted memories from memory.recall (shared_only, or selected_contexts+include_shared for a binding), replacing the direct fragment-table and bearer-file reads while keeping the projection's ordering, bounds and fail-closed behaviour. New acceptance target: crates/nexus-moment-context-assembly/tests/memory_recall_dto.rs
…ecord ops (NXM-8) The drain classifies with the unchanged classifier and consumes each pending candidate through memory.candidates.review, so it writes provider candidates/records only and the legacy *_pending_review tables stay write-idle. A binding-local Character PromoteToLongTerm is coerced to one fact fragment through the same call; shared Character and whole-Creator PromoteToLongTerm stage a nexus.<MemoryKind> record. Explicit fragment promotion calls memory.records.promote(target_scope: shared) with the DTO's legacy 0->1 revision sequence, the already-shared refusal and the SOUL narrative cache invalidation preserved. The review half of NXM-5's mutation-time adoption reuses adopt_in_tx through the shared consume_adopting path; the drain holds the bearer's disposition section across the batch and hands it to the provider seam instead of re-acquiring it. A started row always runs to completion: cancelling a journaled provider decision mid-transaction could leave the bearer unreconciled.
…racter (NXM-8 R4-F1..F3) The explicit fragment promotion now resolves the id the fragment list served through the bridge's deterministic external id, adopts a still-projected legacy row inside the promotion's own guarded transaction and re-authorizes the §7.21 promotion admission against the adopted row, so a legacy-projected fragment is promotable and no adopted record can be left behind by a promotion that did not commit (R4-F1). Before mutating, the promotion verifies the record's owning subject is the leasing Character's canonical subject and that the record is a `fact` fragment, both in the handler and again in the admission transaction where the CAS commits: another Character's record and a non-fragment record are refused (R4-F2). The retained SOUL-narrative invalidation is now a function of the record's state: the already-shared early-exit path runs it too and recovers the old binding from the untouched provenance, so a retry after a failed invalidation reaches one consistent state instead of returning the conflict with a stale cache (R4-F3). Regressions: list->promote of a legacy fragment, cross-Character and non-fragment refusals, and a fault-injected invalidation failure followed by a successful retry.
…unds (NXM-7 fix 1) P3-F1: a promoted-memory line is the legacy memory file again — the frontmatter block plus body, re-rendered through the legacy document type — instead of `content.text` alone. Fragment lines keep the empty-keywords case (no `— keywords:` suffix) the legacy column produced. P3-F2: the E6 recall walk filters per family while it pages. Each admitted scope keeps its own newest-100 fragment bound and promoted memories are collected independently, so unrelated kinds (events, the derived narrative cache) can occupy pages but never a family's quota. The walk stops when the quotas fill or the provider is exhausted, bounded by a documented page budget because `memory.recall` has no kind filter. P3-F3: the cloud-stage compatibility fixtures carry valid DTO timestamps. P3-F4: a provider-backed projection test asserts the populated SOUL and memory slots (exact lines, compared against the seeded memory files), shared/binding isolation, the multi-page event flood boundary, and the refusal/validation error paths.
…t (NXM-8 R2-C1) The retry after a committed promotion reconstructed the old binding from provenance.source_context_ids[0], but the provider records source contexts verbatim and independently of placement, so a valid fact fragment could have none or another context first; the retry then deleted the shared and a wrong binding cache and left the real one stale. The promotion's own guarded transaction now records the placement it leaves in memory_provider_meta (keyed by space and record), and the caller clears it once the SOUL-narrative invalidation succeeds. No DDL, no new wire op. Two fault/retry regressions cover an empty list and a list that starts with another context; both fail against the previous inference.
…seam (NXM-7 fix 2) R2-F1: the E6 projection no longer shares a ranked recall page with unrelated record kinds. Fragments (shared and binding-local) are read independently through `memory.records.list` with the `fact` kind filter at the legacy per-scope LIMIT, and the promoted-memory family is read with the nine `nexus.<MemoryKind>` kinds; both walk the provider's cursors to the family's own completion (its legacy bound or exhaustion), so no global page budget can return an empty or partial slot when the legacy read would have returned content. The `memory.recall` adapter page method is gone — it had no other caller — replaced by the additive kind-filtered `family` seam. R2-F2: the complete promoted-memory family is ranked by the bearer slug order before the 20-entry cap, matching `memory_io::list_memories` + `take`; provider recency no longer decides which memories survive. Regressions: the projection fixture floods 830 newer events (more than the former eight recall pages) behind the fragments and promoted memories, and a new case seeds 21 memories whose slug head (`alpha-memory`) is the oldest file so recency and slug order disagree.
…ject rule (NXM-7 fix 3) R3-F1: the per-scope fragment bound cut before the legacy same-`created_at` tie group. The provider orders equal instants by record id ascending while the legacy query ordered them by `fragment_id DESC`, so a 100-row prefix kept the wrong rows of one instant and dropped the fragment the legacy selection put first. `MemoryAdapter::records` now takes `keep_tie_group`, and the E6 family read fetches until the boundary tie group is complete (`tie_group_closed`); `projection_fragments` then applies the legacy per-scope order (`partition`) and caps back to the legacy LIMIT. The E4 callers keep the plain bounded read. R3-F2: `memory.records.list` is the audit listing and keeps serving an archived subject's records, which `memory.recall` excludes (storage §7.7). `MemoryAdapter::family` now re-applies that rule with `memory.subjects.get` (`archived` => no records), so a read-only core that skips `ensure_subject` projects nothing for an archived subject. No provider op, wire, grant or schema change; both ops are the pinned spoke-schemas =0.15.1 reads. Regressions: 101 same-instant shared fragments assert the legacy top of the selection (`frag-100` first, `frag-080` absent); a provider-archived Character subject read through a read-only core yields an empty record projection while a live subject still projects its fragments and promoted memory.
…T (NXM-7 fix 4) R4-F1: the per-scope 100-fragment selection ordered by parsed instant while the legacy query ordered by the RAW `created_at` TEXT (`ORDER BY created_at DESC, fragment_id DESC`). With 101 rows at `2026-02-01T00:00:00Z` and one at `2026-02-01T00:00:00.900Z`, SQLite keeps the whole-second rows and the legacy `LIMIT 100` excludes the fractional one; the instant order put it first and rendered it in the mind's 20 lines instead of a legacy-selected fragment. `partition` now keys each fragment by its original legacy `created_at` text (the bridge's `nexus_legacy.created_at`, else the record's own rendering) and sorts by that text then `fragment_id DESC`; E6 merges scopes on the same key. The E6 family read walks the kind-filtered family to exhaustion (no bounded prefix can prove it holds every row that ranks under the text key while the provider pages by instant), then caps back to the legacy LIMIT. The tie-group extension that the bounded walk needed is gone with the bound; the E4 reads are unchanged. Regression: 101 shared fragments at `2026-02-01T00:00:00Z` plus one at `2026-02-01T00:00:00.900Z`; the projection renders the legacy `Z` head (`frag-100`..`frag-081`) and never the fractional-second row. Neutralising the order back to parsed instants fails it exactly as reported.
…ipeline reroute onto provider candidate/record ops
… + E10 MCA reroute with the contract DTO
…e for fix round 1
- capture -> typed review -> promote round trip now drives the real drain (CoreService::review_memory) instead of MemoryPort::review_candidate - edit/delete/push-personality adopt a still-projected legacy memory under the bridge's deterministic id inside the guarded write; legacy files stay idle - interactive edit carries its read identity+revision into the guarded update and a stale one is the retained version_mismatch conflict - idempotency keys name the mutation (record@revision, minted memory_id, profile@revision) and a default-timestamp capture stores no generated timestamp, so true retries replay instead of colliding - creator memory capture gets its CLI smoke (default timestamp path) - CLI delete reads and confirms before writer admission, then rechecks the target under the writer - stage-0 assembly hands P3's contract records through: the merged base did not compile nexus42 (stale LongTermMemory projection)
…tion R2-F2: the legacy dual-write regression deleted a record the preceding edit had already adopted. Delete a freshly projected, never-mutated record instead and assert it disappears with the bearer file bytes unchanged.
Contributor
 [High impact] The recent changes appear safe to merge; no new actionable failure was established. SummaryMoves memory entry points onto the provider while preserving access to legacy content until migration completes.
Review was based on source inspection. Tests were not run.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
| Findings | Count |
|---|---|
| 🔴 must-fix | 0 |
| 🟠 should-fix | 3 |
| 🔵 nit | 3 |
| ❓ unverified | 10 |
📋 Review
Findings
🟠 Promotion now hard-fails any memory_id without the leading mem_ prefix
- Evidence:
- Evidence:
crates/nexus-creator-memory/src/review.rs:698-704—promote_to_long_termreplacesmemory.frontmatter.memory_id.replace("mem_", "memory-")withslug_from_memory_id(...).ok_or_else(|| MemoryError::InvalidIdFormat(...))?. The diff hunk (/workspace/pr.diff:23882-23912) shows the old derivation could never fail. - Evidence:
crates/nexus-moment-context-assembly/src/recall.rs:296-303— the promoted-record path deliberately supports ids that are notmem_*:memory_display_idfalls back to the providerrecord_id(e.g.rec_native_1intests/memory_recall_dto.rs), andMemoryDocument.memory_idis that display id. So a provider-native memory carries a non-mem_id into the document/LongTermMemoryshape the promotion consumes. - Impact: any promotion whose id lacks the leading
mem_prefix — provider-native records namedrec_*, and legacy files whosememory_idis notmem_-prefixed — used to persist under the unchanged id; it now aborts withInvalidIdFormatafter the summariser already ran (step 4 precedes step 6). The user-visible result is a promotion that can never complete for that memory, not a bad slug. - Effort: XS — decide whether the invariant holds: if only minted ids reach this function, add the assertion upstream where ids are minted; if foreign ids can arrive, fall back to the sanitised id instead of erroring.
- Risk: MED — relaxing it re-opens the collision the change set is fixing (the point of the one-to-one
mem_→memory-map); tightening it must be proven at the id-minting boundary, not here. - Confidence: MED (the refusal site and the non-
mem_id source are both read; whether a foreign id reachespromote_to_long_termin production is not traced — it is the missing link). - Merge class: should-fix
- Fix sketch: Move the prefix invariant to where ids are minted/labelled; at this call site either keep a total map (prefix rewrite, ids kept verbatim otherwise) or return a typed re…
- Evidence:
- Location:
crates/nexus-creator-memory/src/review.rs:698-709 - Category: BUG
- Merge class: should-fix
🟠 Legacy-id grammar is re-implemented as raw SQL in nexus-local-db and already diverges on escaping
- Evidence:
- Evidence:
crates/nexus-local-db/src/actor_world_binding.rs:726-790— the binding-removal gate and its two FK-release UPDATEs spell the provider identity inline in SQL:'lgc:character_memory_pending_review:' || l.pending_idand'lgc:character_memory_fragments:' || l.fragment_id(four occurrences). - Evidence:
crates/nexus-core/src/memory_provider/legacy.rs:1875-1879— the canonical builder isexternal_id(family, components)="{ID_PREFIX}:{family}"with each component percent-escaped (lgc:<family>:<component>), and the families are constants atlegacy.rs:121-127. The SQL concatenation neither escapes nor references those constants. - Impact: this is a lower-level crate (
nexus-local-db) hard-coding an id grammar owned by a higher-level crate (the dependency runs the other way), with a concrete divergence mechanism already in the code: for anypending_id/fragment_idcontaining a characterexternal_idescapes, the two spellings differ. TheNOT EXISTSthen still counts the row (binding removal refused although the provider owns it) and the pairedUPDATE ... WHERE EXISTSmisses it, leaving the FK in place so theDELETEhitsON DELETE RESTRICT. The rule must change in two crates together, and nothing makes that happen. - Effort: S — expose the two family constants + a pure
external_id-equivalent helper from the provider side and bind the ids from Rust instead of concatenating in SQL (or hoist the release intonexus-corewhere the grammar lives). - Risk: LOW — a pure refactor of string construction; the SQL keeps the same shape, only the bound ids change.
- Confidence: HIGH for the duplication and missing escaping (both sites read); the practical trigger is bounded by minted ids (
pr_*/frag_*charset), which is why this is classed a maintainability/correctness-hazard, not data loss. - Merge class: should-fix
- Fix sketch: One canonical owner for the
lgc:id grammar (provider side), consumed by `…
- Evidence:
- Location:
crates/nexus-local-db/src/actor_world_binding.rs:726-790 - Category: DEBT
- Merge class: should-fix
🔵 Binding removal leaves provider records naming a deleted binding and only repairs two legacy tables
- Evidence:
- Evidence:
crates/nexus-local-db/src/actor_world_binding.rs:720-795— the gate counts provider rows (memory_candidates.capture_context_id = ?,memory_records.scope_context_id = ?) but the release block only nullsactor_world_binding_idoncharacter_memory_pending_reviewandcharacter_memory_fragments; no provider row is touched beforeDELETE FROM actor_world_bindings. - Impact: only tombstoned provider records survive the gate (
r.tombstone_json IS NULLexcludes them from blocking), and they keepscope_context_id/capture_context_idpointing at a binding id that no longer exists — a dangling retained-provenance reference in the new surface while the legacy surface is explicitly repaired. Low impact today (tombstoned rows are not served), but any future read that resolves a scope context to a binding row will treat it as a foreign/inactive binding and fail closed. - Effort: XS — state the invariant in the comment, or null the provider scope on the same rows the gate dismissed.
- Risk: LOW — touching provider rows widens the D14 scope, so documenting the asymmetry may be the correct minimal answer.
- Confidence: MED (both the gate filter and the absence of a provider release are read; the downstream reader behaviour is inference).
- Merge class: nit
- Fix sketch: Either extend the release to the dismissed provider rows inside the same transaction, or record in the D14 comment that tombstoned provider scope ids deliberately outlive the binding.
- Evidence:
- Location:
crates/nexus-local-db/src/actor_world_binding.rs:720-795 - Category: DEBT
- Merge class: nit
🔵 Review drain lost its per-row deadline; only the between-row check remains
- Evidence:
- Evidence:
/workspace/pr.diff:5180-5240(hunk@@ -256,72 +256,83 @@, newprocess_bearer_review_batchatcrates/nexus-core/src/memory_pipeline.rs:259-330) — the old body wrapped each row intokio::time::timeout_at(deadline, process_single_review_row(...)); the new body checksInstant::now() >= deadlineonly between rows and then awaitsconsume_review_decision(...)unbounded, with a comment justifying it ("a row that has started runs to completion"). - Impact: the deadline now bounds only the queue bookkeeping. A provider call that blocks on a journal/flock wait (the new path takes the bearer's
MemoryDispositionsection and journals each decision) holds the caller past its deadline with no upper bound and no partial-progress return, where the pre-change code returned at the deadline. The stated rationale covers cancellation safety, not an unbounded single-row wait. - Effort: S — add a cancellation-safe budget: check the deadline before entering the row and give the row a bounded interrupt that cannot cut a transaction in half (or make the drain's callers own a hard outer budget).
- Risk: MED — an interrupt that lands mid-transaction is exactly what the comment warns about (unsettled journal target refuses the next read), so the bound must be taken at a safe point.
- Confidence: MED (change is read; no proof a provider call actually hangs, so this is a resilience regression rather than an observed failure).
- Merge class: nit
- Fix sketch: Keep the between-row check and add a documented safe-point bound (e.g. the deadline is re-checked inside the per-row loop's pre-transaction step, with the transaction itself unbounded), or state the unbounded worst case in the function contract.
- Evidence:
- Location:
crates/nexus-core/src/memory_pipeline.rs:290-330 - Category: BUG
- Merge class: nit
🟠 Legacy external-id prefix is re-derived as a raw SQL string in three new .sqlx snapshots, so a rename silently no-ops the binding clear
- Evidence:
- Evidence:
.sqlx/query-[REDACTED].json:3—UPDATE character_memory_fragments SET actor_world_binding_id = NULL ... WHERE r.record_id = 'lgc:character_memory_fragments:' || fragment_id; the mirror snapshot.sqlx/query-[REDACTED].json:3does the same forcharacter_memory_pending_review/memory_candidates; the count snapshot.sqlx/query-[REDACTED].json:3re-derives the same prefix a third time. The Rust side owns the rule as a derivation (crates/nexus-core/src/memory_provider/mod.rs:1609— "lgc:character_memory_fragments:<id>"), i.e.lgc:<family>:<key>built in code, never as a shared SQL literal. - Impact: the count query and the two clear-UPDATEs must agree with the Rust derivation character-for-character, and nothing structurally enforces it (only one test pins the literal,
crates/nexus-core/tests/legacy_content_migration.rs:741, and it pins it for one family). If a family name or separator changes in the Rust derivation, these UPDATEs match zero rows and report success — the cleared-legacy guarantee of the migration silently stops holding (legacy rows stay actor-bound and keep inflating the pending/fragment counts), with no error to catch it. The three new snapshots are the committed, reviewable form of that coupling. - Effort: S — one shared constant/helper for the legacy id form, and have the SQL sites take the prefix as a bound parameter.
- Risk: LOW — parameterising the prefix keeps behaviour identical while removing the silent-mismatch mode.
- Confidence: MED (read all three snapshots and the Rust doc; the drift is a future-edit failure mode, not a present defect —
actor_world_binding.rs:742/:796currently agree with the snapshots). - Fix sketch: express the legacy external id once (a
legacy_external_id(family, key)helper plus per-family SQL that binds the prefix instead of concatenating a literal), and assert the helper's output against the migration's expectations for every family, not just fragments.
- Evidence:
- Location:
.sqlx/query-[REDACTED].json:3 - Category: tech-debt
- Merge class: should-fix
🔵 CI comment points maintainers at an escape-path document that is not in the repo
- Evidence:
- Evidence:
.github/workflows/ci.yml:286-287— "Run prepare only through the documented escape path (pr-fix2-report.md F4), never as a CI gate." A repo-wide search forpr-fix2-reportreturns only these two comment lines, and nopr-fix2-report.mdexists anywhere in the worktree (glob over**/pr-fix2-report.mdand the repo root: no match). - Impact: this comment is the only pointer telling a future maintainer why
cargo sqlx prepare --checkis not a gate and where the sanctioned procedure lives. The referenced artifact is a plan-local report that was never committed, so the cited escape path is unreachable — the next person either re-litigates the decision or reintroduces the gate the comment exists to prevent. - Effort: XS — restate the reason in the comment itself, or point at a committed doc (e.g. the knowledge tree entry this PR adds).
- Risk: LOW — comment-only.
- Confidence: HIGH (comment opened; absence verified by search).
- Fix sketch: replace the
(pr-fix2-report.md F4)citation with the committed rationale (the writer-protocolnexus_writer_*UDF constraint is already spelled out two lines above), or link the committed knowledge/spec document that carries the procedure.
- Evidence:
- Location:
.github/workflows/ci.yml:286-287 - Category: docs
- Merge class: nit
Prior findings recheck — 0/25 reassessed, 66 open
Prior findings recheck
Reassessed 0/25 open prior finding(s) this round · 25 omitted · 41 over cap · 66 open · context coverage: truncated
| finding | disposition | evidence | thread |
|---|---|---|---|
creator memory pending-list panics on a non-ASCII pending_id/session_id (byte-slice truncation) |
unverifiable (invalid-output) | - | - |
SL: push-personality prints a blank memory id on a missing identity instead of refusing |
unverifiable (invalid-output) | - | - |
New nexus-core test suite creator_capture.rs is never executed by CI |
unverifiable (invalid-output) | - | - |
pending-list truncates IDs by byte index — panics on a non-char-boundary ID |
unverifiable (invalid-output) | - | - |
creator memory pending-show page walk is unbounded and contradicts its own 'bounded loop' comment |
unverifiable (invalid-output) | - | - |
service-native CI filter omits the workspace crates the native addon links |
unverifiable (invalid-output) | - | - |
pending_show pagination loop is documented as bounded but has no iteration cap |
unverifiable (invalid-output) | - | - |
| stage0_memories docstring promises 'authored kind' filtering the predicate does not enforce | unverifiable (invalid-output) | - | - |
save_memory leaks its .md.tmp file when the write itself fails |
unverifiable (invalid-output) | - | - |
Moment assembly's fragment-keyword fetch can never succeed: the route requires creator_id, the caller sends none |
unverifiable (invalid-output) | - | - |
Daemon memory family clamps limit at 1..=100 on character routes and 1..=250 on creator routes |
unverifiable (invalid-output) | - | - |
| Changeset is ~10k added lines across 42 files — past the reviewable band; split before merge | unverifiable (invalid-output) | - | - |
creator memory capture silently drops a second session's digest when --pending-id is reused, and still reports success |
unverifiable (invalid-output) | - | - |
pending_show comment claims a call cap it does not have, and each page re-walks from the start |
unverifiable (invalid-output) | - | - |
Adapter records() can return up to a whole extra page beyond its documented want bound |
unverifiable (invalid-output) | - | - |
| Tombstone policy diverges across the three rerouted memory read paths (list drops, show silently succeeds, stage0 keeps) | unverifiable (invalid-output) | - | - |
Binding-removal gate and its release UPDATE rely on a memory_records.scope_context_id column with no evidenced FK/consistency guarantee |
unverifiable (invalid-output) | - | - |
| Review drain reads the whole pending queue before applying its 50-row bound | unverifiable (invalid-output) | - | - |
| Review drain no longer bounds a single row, so REVIEW_CALL_TIMEOUT stops bounding the call while the bearer's disposition flock is held | unverifiable (invalid-output) | - | - |
| Two 2k+/6.6k-line files added or grown by this PR cross the file-size watch | unverifiable (invalid-output) | - | - |
TEST: review_drains_small_queue never asserts the queue was drained |
unverifiable (invalid-output) | - | - |
[BUG-02] soul push-personality prints an empty memory id and a fabricated kind on the fallback path |
unverifiable (invalid-output) | - | - |
Legacy bridge silently swallows malformed fragment keywords JSON instead of reporting Corrupt |
unverifiable (invalid-output) | - | - |
| Pending-review count now walks and materializes every provider page instead of one COUNT query | unverifiable (invalid-output) | - | - |
| Capture test asserts an output-shape property instead of product behaviour | unverifiable (invalid-output) | - | - |
Review 12 · commit 6836f05
…reads) PR-F1 (seat 1): the migration's already-exists check treats a provider row that still carries the legacy object's identity as the object's successor instead of conflicting. `resolve_legacy_snapshot` substitutes the stored record/profile for the projection, so a pre-migration edit or SOUL write no longer blocks the pass, the marker records the values provider-only reads serve, and `source_digest` keeps the source-drift refusal (P3-F3). A row that collides with a legacy id without the bridge provenance refuses `idempotency_conflict`. PR-F2 (seat 1): the Character pending queue sorts by the RAW legacy `created_at` TEXT with the id tiebreak, as the retained query did; mixed formats are covered on the offset page and on the bounded review batch. PR-F3 (seat 2): `--test creator_capture` joins the rust-core-domain-owners leg. Clippy: `assert_is_empty` at memory_services.rs:3011 replaced with a count assertion. Greptile threads: - Character captures adopt their own retained queue row at capture time, so a post-marker capture stays visible; the Character dismissal falls back to the native candidate id. - The binding-removal gate counts served memory: provider-placed pending candidates and live records, legacy rows only when the provider does not own their origin (narrative cache rows unchanged). - Promotion records the authored identity (`slug`, `memory_id`, `memory_kind`) the CLI addresses a memory by. - Reflection reads fragment stats and signals from provider `fact` records; the narrative cache row stays the retained table. - Long-term files are selected by request: a by-id read opens only its own file and a non-authored kind request opens none, so one malformed memory no longer breaks unrelated reads. `memory_by_slug` resolves the deterministic id first. - The keyword filter is ASCII case-insensitive, as the retained `LIKE` was.
Runs the full declared verification set at 1acedf2 and repairs what it exposed, keeping the landed contracts (P1-P5 seams, D5/D6/D13). - Creator slug read: the deterministic-id fast path served a tombstoned record to admin coverage, so a deleted memory still resolved by slug. The by-id answer is only used when it is not a tombstone; otherwise the authored-kind scan decides (it excludes tombstones and provider-owned legacy rows). - Character capture after the completion marker: `adopt_in_tx` resolved its row through a marker-gated projection, so the capture's own adoption was a no-op and the row was invisible to list/count/dismissal. Adoption now takes `include_migrated`: the capture passes true (it adopts the row it just wrote), the dismissal passes false, so "the marker re-read inside the admission transaction withdraws a raced row" still holds (`dismissal_racing_the_completion_marker_adopts_nothing`). - Binding removal gate: ownership is matched on the bridge's deterministic id (`memory_candidates.candidate_id` / `memory_records.record_id`) instead of the projected `nexus_legacy` body, which a consumed candidate and a tombstoned record no longer carry. The provider-owned legacy rows still name the binding in an `ON DELETE RESTRICT` foreign key, so removal releases that provenance the way the legacy local->shared promotion does (row kept, reference cleared). - Clippy: `record_keywords` is `pub` (pedantic `redundant_pub_crate` inside the private `memory` module); the new SQL literals drop needless raw-string hashes. - rustfmt (nightly-2026-06-26): pending diffs in soul.rs/storage.rs and the migration test.
Contributor
Author
|
@greptile-apps review |
![wood-s-inspector[bot]](https://avatars.githubusercontent.com/in/4803300?s=60&v=4){kind=small}
Contributor
Author
|
@greptile-apps review |
- F1 migration: compare a pending candidate's `created_at` at the precision the provider column stores, so an adopted capture whose legacy instant carries fractional seconds no longer refuses the whole pass. - F2 migration: a binding removal releases the legacy row's placement provenance, and the provider row is the object's successor — the record identity compares provenance without the released source contexts, an owned candidate restores its released binding before the content-digest check, and the marker's source digest is taken over that release-normalized source, so the release neither refuses the pass nor re-conflicts a re-run. - F3 memory pipeline: the promoted slug maps only the leading `mem_` prefix (one-to-one), shared with the retained-file promotion path so two ids can never address one memory. - F4 local-db: the binding-removal gate and its two release statements use `sqlx::query_scalar!`/`sqlx::query!`, with the regenerated `.sqlx` entries. - F5 legacy bridge: a malformed stored `keywords` column reports the module's typed `Corrupt` answer instead of a silently empty keyword list. - F6 legacy bridge: both fragment families project in the order of the legacy per-scope read they stand in for (`created_at DESC, fragment_id DESC`). Regressions: a fractional-instant capture and a released binding both migrate (first pass and re-run); promoted slugs stay one-to-one; malformed keywords fail the read; both fragment families keep the legacy tie-break.
Contributor
Author
|
@greptile-apps review |
- F1 legacy bridge: a by-id read selects its fragment row before parsing the stored keywords (or opens no fragment family), so one damaged fragment no longer fails show/edit/delete of another memory; list reads and the migration snapshot still project the whole family. - F2 slug: the promoted-slug product test also asserts both colliding pending ids list under their own slug (neither overwrites the other); the byte-stability test pins the literal `memory-<id>` contract instead of deriving the expectation from the helper under test. - F3 slug: `slug_from_memory_id` accepts only `mem_` ids and returns `Option`, so `mem_a` / `memory-a` cannot share a file; both call sites map the impossible `None` to their typed error. - F4 context: stage 0 admits only live records (personality_core still excluded), so a user-deleted memory never enters prompt assembly. - F5 soul CLI: `push-personality` omits the identity lines when the record carries none instead of printing an empty id and a fabricated kind. - F6 memory CLI: one kind-normalizing helper (shared with stage 0), and a record without an authored slug is skipped rather than listed blank. - F7 CI: no `cargo sqlx prepare --check` step - it cannot verify freshness here (writer-protocol UDF triggers + SQLX_OFFLINE degeneracy); the constraint is recorded in the workflow instead. Regressions: damaged-keyword fragment isolated from unrelated memory reads; promoted slugs one-to-one and both listed; stage-0 drops tombstones; the soul push identity is never fabricated; the list skips slugless rows and renders bare kinds.
Contributor
Author
|
@greptile-apps review |
1 similar comment
Contributor
Author
|
@greptile-apps review |
- F1: count pending reviews by walking the provider's pages and incrementing
(`MemoryAdapter::pending_count`), instead of materializing and sorting the
whole candidate set through `pending_rows`; the Creator and Character count
paths share it, each keeping its own scope predicate.
- F2: `MemoryAdapter::records` truncates the collected rows to `want` so the
documented "at most `want`" bound is the caller's, not the provider page's.
- F3: the dismissal answer is documented as what the code already does — only
an `Ok` receipt means the dismissal consumed the row; `CandidateConsumed`,
`NotFound` and `RevisionConflict` are the retained not-found answer. Pinned
the product contract: a dismissal after the review drain consumed the row is
`NotFound`, never success.
- F4: `stage0_memories`'s docstring now states the predicate it enforces (every
live record except `personality_core`, no authored-kind taxonomy) instead of
claiming an authored-kind filter; the unit test pins the narrowed contract.
- F5: a SOUL document that fails to parse in `context assemble-moment` is
labelled `SOUL.md: {error}` instead of `Memory error:`.
Regression: `creator_capture::creator_pending_count_matches_the_listed_length_across_pages`
(101 pending rows over two provider pages), `memory::tests::records_returns_at_most_want_rows`
(returns 2 pre-fix for `want = 1`), the round-trip's post-review dismissal
assertion, and `context::tests::stage0_drops_tombstoned_and_personality_records`.
Contributor
Author
|
@greptile-apps review |
B1: the Character capture is one authoritative provider write. It writes the candidate through MemoryPort::capture under the bridge's deterministic id for the pending_id (no legacy queue insert, so no crash window can strand a legacy row), keeps the retained nexus_legacy projection fields and placement, and replays a re-delivered digest. The legacy binding admission is preserved as a read-only validation. B2: a provider-native promoted memory resolves by its authored slug over the provider's own records only — the legacy read bridge is now optional on the records list op — so one damaged unrelated bearer file cannot break its show/edit/delete. B3: the binding-removal provenance release keeps its two narrow UPDATEs, documented at the site as the PM-ruled legacy-domain FK bookkeeping, and the migration regression now asserts that only actor_world_binding_id changes on those rows (plus the fragment table's own OCC trigger bump).
B1 (D15): the Character capture's binding provenance is validated on the provider's own capture transaction (its connection, under BEGIN IMMEDIATE), so remove_binding cannot interleave between the check and the candidate write. The failure keeps its retained character_memory 403: it travels outside the port vocabulary (CaptureError::Binding). New regression pins both lock orderings with a capture-transaction rendezvous seam. B2 (D16): a fixed-id capture whose id already names a legacy-only projection adopts that projection under the same id and writes nothing of its own, so the one-shot migration's same-candidate check sees one version. Regression seeds the stock state, re-captures with a divergent body and migrates. B3 (D14): the binding-release site comment cites D14; the migration regression asserts every content column is byte-identical and names the fragment table's own trigger as the one audit-column change (revision +1).
…rite attempt The D15 ordering proof waited on a separate zero-busy `BEGIN IMMEDIATE` probe run before `nexus_local_db::remove_binding` began its own transaction, so a sequential capture-then-removal execution still passed. The report now comes from the removal itself: under `test-hooks` `remove_binding_reporting` makes its own first write-lock attempt with the connection's busy timeout held at zero, notifies the armed waiter when that attempt is refused with SQLite's lock/busy code, then restores the timeout and begins the transaction on that same connection through the normal busy-timeout retry. Only real contention reports, and the retry is already parked on the capture's lock when the capture is released. The production `remove_binding` path and its busy-timeout behaviour are unchanged; every seam is compiled out without `test-hooks`. Both ordering assertions are untouched.
…gin_immediate The D15 ordering proof went through a separate transaction-opening implementation (remove_binding_reporting / begin_immediate_reporting), so a regression that stopped the production begin_immediate from acquiring the write lock would not fail it. Instrument begin_immediate itself under test-hooks: a task-local report armed around the removal future makes that function's own first BEGIN IMMEDIATE run without a busy wait and notify only on SQLite's lock/busy codes (5/6) before the normal busy-timeout retry. Both production and the test now exercise the one transaction-opening path; the seam is compiled out of a production build.
Contributor
Author
|
@greptile-apps review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage N2 entry-point convergence per
.mstar/specs/proposed/architecture/spoke-memory-adoption-plan.md§7.MemoryPortvia provider-seam legacy read bridge (deterministiclgc:ids, canonical subject rule, per-space completion marker, mutation-time JIT adoption, per-bearer disposition locking).memory_io/soul_ioabsent fromapps/nexus42.LongTermMemory→RetainedRecordcontract DTO.creator memory migrate-legacy— one guarded transaction, deterministic ids, byte-exact preservation acceptance, idempotent re-runs, completion-marker publication.Data-preservation acceptance (before/after equivalence incl. byte-exact bodies and legacy byte-identity) is a named test target. Knowledge: legacy-surface-cutover-pattern. Residuals: I-000140 (D13 owner adjudication), I-000149 (QC1-F2, satisfied by P5 alignment).