refactor(core): add shared runtime orchestration#1002
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb65be817b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
phernandez
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f338a6d382
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 703206a432
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
phernandez
force-pushed
the
codex/repository-explicit-sessions
branch
from
703206a to
07a8882
Compare
|
@codex review |
1 similar comment
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e93ca7c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 similar comments
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
phernandez
force-pushed
the
codex/repository-explicit-sessions
branch
from
c3c5a7b to
bd4b25c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd4b25ca3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Addressed the five latest Codex P2 findings in 07c25b2:
Regression coverage was added for each bug. Verification:
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
A local note write returns 202 before its markdown file is written; a SIGTERM in that window stopped the watcher and closed the engine while the materialization pool and its follow-up background tasks were still queued, silently losing accepted writes. Drain both queues before shutdown_database(), mirroring the one-shot CLI cleanup order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
A transient stat/checksum error during project observation dropped the path from the observed snapshot, and delete reconciliation (all_db_paths - storage_paths) then destroyed the entity and search rows for a file that still exists. Only a positively confirmed disappearance now falls out of the snapshot; anything else is carried through with an unknown (None) checksum, which classifies as modified so the batch planner re-checks it (missing -> skip, readable -> index). Unknown checksums are excluded from move-candidate matching since they carry no content evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46d0011b4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _materialization_pool.submit( | ||
| self._materialize_write_now(accepted), | ||
| workers=self.materialization_workers, | ||
| ) |
There was a problem hiding this comment.
When local production uses materialization_workers > 1 (the default is 4), two quick writes to the same note can be processed by different workers at the same time. Since _materialize_write_now() builds the preflight/publisher with the default no-op materialization lock, the second worker can snapshot the old file_checksum before the first worker publishes its stale write; after the first worker writes version N, the second worker treats that in-process write as an external checksum conflict and can mark the latest accepted note external_change_detected. Key the queue/lock by entity or path so materializations for one note run in order while other notes remain parallel.
Useful? React with 👍 / 👎.
plan_previous_note_file_delete decided old-path cleanup after a move with an exact string compare plus a checksum guard. That is correct for object storage (case-different keys are distinct objects) but wrong on a case-insensitive filesystem (APFS/NTFS), where Notes/Foo.md and notes/foo.md are the same inode. After a case-only rename the checksum guard reads the just-written new file through the old path, matches, and deletes the note's only copy; scan reconciliation then removes the entity. Carry the move destination as live_file_path on the pending delete/request and have the local inline delete skip when old and new resolve to the same physical file (FileService.paths_share_storage_target -> samefile). The storage-neutral plan and the cloud/S3 delete path are unchanged. The v1 EntityService path already guarded this; v2 accepted-move had no equivalent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…nations
normalize_note_move_destination_path is the only containment check before the
destination is joined onto the project root, but it only rejected empty strings
and a leading '/'. It accepted '..' traversal ('../../evil.md',
'folder/../../evil.md') and Windows drive/UNC roots ('C:/evil.md',
'\\host\share\x'), which are absolute on Windows so base_path is discarded.
Reject absolute destinations under either OS convention and '..' segments under
either separator convention.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
The accepted note_content write path had no version guard: accept_write overwrote the row unconditionally (UPDATE ... WHERE entity_id), so two concurrent accepted writes that both read db_version N and planned N+1 would last-write-wins, silently losing one edit. db_version was tracked and incremented but never enforced. Make accept_write a compare-and-set: the update is conditional on WHERE db_version = expected_prior (a rowcount check, portable across SQLite and Postgres where with_for_update is a no-op). On a lost race it raises NoteContentVersionConflict, which the create/update/edit/move mutation wrappers surface as a 409 so the loser retries against fresh state. synchronize_session=evaluate keeps the returned row's tz-aware datetimes (a DB refresh would hand back naive datetimes on SQLite). Also flip a note out of file_write_status='writing' on ANY post-write failure, not just storage errors: the publish phase issues DB writes that can raise SQLAlchemy errors, which previously escaped and left the row stuck 'writing' (unreadable) forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Three adaptation regressions from the cloud->core runtime extraction, each with a regression test: - Project-scan/startup path no longer wraps the search writer in a markdown-only filter, so non-markdown entities (images, PDFs, other files) are search-indexed on a full scan, matching the watcher path (was a regression vs the pre-extraction scanner). - Change detection chunks its file-path and checksum IN() lookups under the bind-parameter limit, so indexing no longer errors outright on projects larger than ~999 files (SQLite) / ~32k (Postgres). - The startup observed-file scan reuses the stored checksum when a file's mtime+size still match the indexed row, restoring the dropped watermark shortcut so large projects don't re-hash every file on every boot. The LocalIndexEntityRepository protocol now declares the select() capability the stat scan uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
A single unreadable/undeletable file aborted the whole directory-delete batch: its enqueue error propagated out of the loop, so every remaining file's DB row was already deleted while its on-disk file survived, to be resurrected by the next sync. Each per-file enqueue is now contained and collected as a failed delete so the rest of the batch still cleans up. Also capture the surviving relation-source entity ids (notes outside the directory that linked into it) so a consumer can refresh their stale search-index relation rows; exposed on DirectoryDeleteAcceptedResult. Caller-side reindex wiring is a follow-up (documented in the PR). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Five watcher/shutdown robustness fixes from the runtime extraction, each with a regression test: - Per-project change handling is isolated: an exception in one project's batch (indexing or move processing) no longer aborts the gather and drops unrelated projects' events. - Move processing that raises no longer discards the whole event batch; the remaining create/update/delete events still index. - The deleted-project guard re-reads current config instead of a stale startup snapshot, so a project deleted after the watcher started is not re-indexed by background sync. - WatchCoordinator.stop() logs and swallows a previously-stored watcher crash instead of re-raising it, so the 202-materialization drain and the rest of shutdown still run. - A gitignored file in a nested child project is dropped instead of falling through and being routed to the parent project. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…ed sessions - ConfigManager now maps the legacy BASIC_MEMORY_SYNC_CHANGES / BASIC_MEMORY_SYNC_DELAY env vars onto index_changes / index_delay. The rename's before-validator only covered config.json keys, so env-var users were silently dropped. - The Postgres FTS path wraps its tsquery execute in a savepoint so a syntax error can't poison a caller-owned transaction; swallowing the error and returning [] previously left the shared session aborted, so every later query on it failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Test + typing follow-ups for the note-write safety commits landed earlier in this branch: - Assert the new live_file_path on RuntimePendingNoteFileDelete cleanup requests (the case-only-rename skip guard) across the runtime contract tests. - Add the mutation-runner PUT-rename storage-guard (409) and non-markdown existing-entity (415) regression tests. - Cast the accept_write conditional UPDATE result to CursorResult for the rowcount compare-and-set check, and drop the now-unused FileError / FileOperationError imports after broadening the post-write except. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 757313bbcb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DELETE FROM search_index | ||
| WHERE project_id = :project_id AND entity_id IN :entity_ids |
There was a problem hiding this comment.
When a note outside the deleted directory links to a note inside it, the relation table row is cascaded away but the corresponding search_index row is owned by the surviving source entity (entity_id is the outside note), so this delete leaves it behind. Although this method computes relation_cleanup_entity_ids, the delete-directory route never reindexes those ids, so searches can continue returning relations to deleted notes until a later full reindex; delete relation rows whose from_id/to_id is in the deleted set or run the returned cleanup ids before reporting success.
Useful? React with 👍 / 👎.
Two P1 write-ordering races where a read-modify-write on note_content could silently revert a newer accepted API write: - NoteContentReconciler read state, planned from it, then applied the plan via an unconditional update_state_fields (no WHERE db_version). A concurrent accepted mutation that advanced db_version between the read and write was clobbered. update_state_fields now takes an opt-in expected_db_version and becomes a compare-and-set; the reconciler passes the version it read and treats a lost race as a benign skip (a fresh reconcile converges), rather than a 409. - Under the default Noop materialization session lock, two materializations for the same entity run concurrently. The written-file publish read note_content, planned, then wrote unconditionally, so an older job finishing after a newer one reverted file_version. The publish now guards both applies on the read db_version and, on a lost race, returns a stale result and skips the entity metadata update instead of reverting. Adds repository- and reconciler-level regression tests for the guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d18b423b8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "successful_deletes": len(self.deleted_files) - skipped, | ||
| "failed_deletes": skipped, | ||
| "deleted_files": list(self.deleted_files), | ||
| "errors": list(self.skip_reasons), |
There was a problem hiding this comment.
When a local directory delete has any skipped cleanup or enqueue failure, this response now emits errors as plain strings, but the MCP/CLI client validates the API JSON as DirectoryDeleteResult, whose errors field is List[DirectoryDeleteError] objects with path and error. In that partial-failure scenario the tool raises response validation instead of reporting the failed paths; serialize structured {path, error} entries here or update the shared schema to match the payload.
Useful? React with 👍 / 👎.
The OrphanCleanup/ProjectDelete/NoteMaterialization SessionProvider Protocol+Default pairs were three copies of the same one-line behavior (`db.scoped_session(session_maker)`), and the cloud app overrides none of them — they were pure test-injection indirection, not a local/cloud seam. Inline `db.scoped_session` at the call sites and drop the Protocol, the Default impl, and the `session_provider` dataclass fields. Tests that used the seam to inject a fake/recording session now monkeypatch `db.scoped_session` directly (the pattern already used elsewhere), and the project-delete test that existed only to assert the seam was used is dropped in favor of the existing real-DB coverage. Part of the protocol-surface reduction from the local/cloud interface audit. Signed-off-by: phernandez <paul@basicmachines.co>
The four single-impl Clock protocols (NoteMaterialization, IndexedNoteContent, VectorSync, AcceptedNoteMutation) exposed a wall/monotonic clock as a constructor/Protocol seam. Both local and cloud composition roots passed the identical System*Clock — it was never a divergent local/cloud interface, just an implementation detail that leaked up into the interface. Push the detail down: each concrete runner now calls a module-level now()/perf_counter() function directly, and the Protocol + System class + the clock field/param are removed. Deterministic tests reach the module-level function via monkeypatch (a stateful counter for the vector-sync sequence, an autouse freeze for the mutation module) instead of injecting a fake clock. Removes the clock arg from the AcceptedNoteMutationDependencies bundle built by both composition roots (cloud updated in lockstep in its own PR). Part of the protocol-surface reduction from the local/cloud interface audit. Signed-off-by: phernandez <paul@basicmachines.co>
VectorSyncProgressReporter had no implementation and no caller in either core or cloud: run_vector_sync and ProjectIndexRuntime.sync_entity_vectors both defaulted progress_reporter=None and nothing ever passed it. The only consumer was a test that injected a recording reporter to exercise the dead None-guarded report_progress branch. Drop the Protocol, the progress_reporter parameter and its branch, the pass-through in ProjectIndexRuntime, and the test's recording double plus its progress assertions (the resume test still covers checkpoint resumption). Part of the protocol-surface reduction from the local/cloud interface audit. Signed-off-by: phernandez <paul@basicmachines.co>
ProjectIndexMetadataReporter had no implementation and no caller in either core or cloud: run_project_index_move_batches / run_project_index_delete_batches, StoreProjectIndexMaintenanceRunner, and ProjectIndexRuntime all defaulted metadata_reporter=None and both production callers (project_index_coordinator and local_moves) always omit it. Cloud's own ProjectIndexMaintenanceRunner fake uses a different param and never touches this reporter. Drop the Protocol, the metadata_reporter parameter threaded through the maintenance runner Protocol / StoreProjectIndexMaintenanceRunner / the two run_*_batches functions / ProjectIndexRuntime, and the two None-guarded report_progress branches. Remove the recording reporter doubles and progress assertions in the maintenance/runtime tests, and drop the now-dead metadata_reporter param from the coordinator/runner/local-index test fakes. The batch progress dataclasses and their workflow_metadata() serialization stay covered by the plan tests. Part of the protocol-surface reduction from the local/cloud interface audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
StorageEventBucketProcessor, StorageEventSourceIndexRuntime, and
run_storage_event_source_indexing had no caller in core src or cloud: the bucket
source path was only reachable from test_storage_event_orchestration.py. Cloud's
Tigris notification processor uses the bucket-context path
(run_storage_event_bucket_indexing / StorageEventBucketIndexRuntime /
StorageEventBucketContext{Resolver,Processor}) and core local_watch uses
run_storage_event_indexing / StorageEventIndexRuntime — all of which stay.
Drop the dead Protocol, its runtime dataclass, and the source-indexing function,
plus the RecordingBucketProcessor double and the three source-indexing tests.
The bucket-context and project-indexing orchestration tests still cover the live
code paths.
Part of the protocol-surface reduction from the local/cloud interface audit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
…path ForwardReferenceLinkResolver had no concrete implementation and no caller in core src or cloud: it was only satisfied by test doubles and injected into RepositoryForwardReferenceResolutionRuntime via build_default_project_index_runtime. The real per-project resolution path is the module-level resolve_project_link_texts (also used directly by cloud's entity_indexer), so the Protocol added an indirection with a single, test-only implementation. Drop the Protocol and its injection point: RepositoryForwardReferenceResolutionRuntime now takes project_id (matching its sibling RepositoryForwardReferenceRelationSource) and calls resolve_project_link_texts directly with its session_maker; the link_resolver parameter is removed from build_default_project_index_runtime. Deterministic tests reach the resolution via monkeypatching forward_reference_resolution.resolve_project_link_texts. Remove the test doubles (NoopForwardReferenceLinkResolver, RecordingForwardReferenceLinkResolver) and retarget the runtime tests to project_id / the module-level function. Part of the protocol-surface reduction from the local/cloud interface audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Three custom structural logger Protocols in the indexing package were single-implementation seams the cloud app never implemented, so collapse each to the concrete loguru logger and keep implementation-detail logging out of the public interface. - ChangeDetectionLogger: the optional bound_logger param was dead (no production or test caller ever passed it). Delete the Protocol and the param; detect_project_file_changes now logs through the module-level loguru logger directly. - OrphanCleanupLogger: cleanup_orphan_entities has no production caller and only tests injected a RecordingLogger. Delete the Protocol and the param; the skip/deletion diagnostics now bind their structured fields onto the module-level loguru logger. Tests assert the messages via a loguru sink. - VectorSyncLogger: run_vector_sync (threaded from project_index_runtime) keeps its required logger param, now typed as loguru.Logger. The completion log binds project_id as structured context rather than passing it as a format arg (which loguru would silently drop). Tests capture the records through a loguru sink instead of a fake logger. Coverage stays 100% for change_detector and orphan_cleanup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
`AcceptedNoteDeleteSession` was a one-method Protocol view of AsyncSession (`async def delete(instance)`), used only by `delete_accepted_note_entity`. The sibling `delete_accepted_note` already takes a concrete `AsyncSession` and calls `session.delete(entity)` directly, and the cloud app supplies no second implementation — this was pure test-shaping indirection, not a local/cloud seam. Type the parameter as `AsyncSession` and drop the Protocol. The one test that exercised it now casts its fake session to `AsyncSession`, matching the sibling delete tests in the same file. Part of the protocol-surface reduction from the local/cloud interface audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…actory `LocalProjectIndexRuntimeProvider` was a factory-shaped Protocol (`runtime_for_project(project) -> LocalProjectIndexRuntime`) used only as the parameter type of `run_local_project_index_for_project`. Every caller — the CLI reindex commands, service initialization, and all tests — passes the sole concrete `LocalProjectIndexRuntimeFactory`; neither the cloud app nor any test supplies a second implementation. It was pure structural indirection, not a local/cloud injection seam. Type the `runtime_factory` parameter as the concrete `LocalProjectIndexRuntimeFactory` and delete the Protocol. Part of the protocol-surface reduction from the local/cloud interface audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
ProjectIndexMoveBatchStore and ProjectIndexDeleteBatchStore each had exactly one implementation — RepositoryProjectIndexMaintenanceStore — which cloud uses too (apps/cloud/services/index/project_runtime.py passes the same concrete as both move_store and delete_store). Neither was a local/cloud seam; they were pure test-injection indirection for the batch-orchestration functions. Retype StoreProjectIndexMaintenanceRunner, run_project_index_move_batches, run_project_index_delete_batches, and ProjectIndexRuntime's move_store/ delete_store fields to the concrete store, and delete both Protocols. The orchestration tests that used recording batch-store doubles now inject the real concrete and monkeypatch its single SQL batch method to return canned results, preserving accumulation/progress coverage without a database. Part of the protocol-surface reduction from the local/cloud interface audit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…ccepted content Batch note_content reconciliation reconciled each entity using `indexed.markdown_content`, a scan-time snapshot. Between scan and reconcile a note materialization can rewrite the file to a newer accepted db_version; the batch then promoted the stale snapshot and reverted the newer write. The db_version compare-and-set guard does not help: the stale snapshot promotes cleanly to a fresh version. Add an optional `file_reader` (NoteContentReconcileFileReader) threaded through `reconcile_indexed_note_content_batch`, `IndexedNoteContentReconciliationTask`, `IndexBatchRuntime`, and `build_default_index_batch_runtime`. When set, the reconcile task re-reads the current file: missing content (file gone) skips reconciliation, otherwise the fresh content and its mtime drive reconciliation instead of the scan snapshot. Default None preserves existing behavior so cloud compiles unchanged. Local indexing wires a filesystem-backed reader (FileServiceNoteContentReconcileFileReader) built on FileService. Cloud passes None for now to avoid doubling S3 reads and can opt in later via its S3 reader. Adds regression coverage: a stale scan snapshot no longer reverts a newer accepted note_content version once the reader re-reads the current file, plus a companion test documenting the revert that occurs without the reader. 🐛 Fixes a concurrency defect the version guard cannot catch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
An accepted local note write returns before its markdown file is written: accept_write marks note_content "pending", the materialization preflight flips it to "writing", and the publisher records "synced" only after the file lands. A process crash between those points leaves the row stuck forever and the source-of-truth file never (re)materialized. Add a startup recovery sweep that re-drives these rows: - NoteContentRepository.find_stuck_materializations() returns project-scoped rows whose file_write_status is writing/pending. - recover_stuck_materializations() rebuilds a RuntimeNoteMaterializationJobRequest from each stuck row's own db_version/db_checksum and runs it through the standard preflight/writer/publisher/status-publisher via run_recovery_materialization(). The db_version compare-and-set guard makes this safe: an older recovery attempt can never revert a newer accepted write or clobber an unexpected file. Non-fatal per row so one poisoned note cannot block startup for the rest of the project. - initialize_file_indexing runs recover_project_materializations() once per locally-syncable project before the watch service serves, so files are re-written ahead of the initial project index scan. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…flagging a conflict The startup recovery sweep (:448) re-drives note_content rows stuck in 'writing'/'pending'. A crash after the file write but before the DB publish leaves the row 'writing' with the correct accepted content already on disk and file_checksum still None (new note) or the pre-write checksum (update). On re-drive the write guard saw a present file whose checksum did not match the expected previous checksum and raised RuntimeFileConflictError, so recovery marked the row 'external_change_detected' and never advanced file_version — recovery only actually worked when the file had never been written, arguably the rarer crash location. Add a same-content short-circuit to write_prepared_note_to_content_store: read the on-disk checksum once, and if it equals the checksum of the accepted content we would write, skip the redundant write and return the existing file's state so the publisher advances file_version and marks the row 'synced'. Only raise a conflict when the present file matches neither the accepted content nor the expected previous checksum (a genuine external edit). Extract the shared runtime_file_conflict helper so assert_runtime_file_matches_expected and the write path apply identical conflict logic from a single checksum read. Adds a regression test seeding a stuck 'writing' row whose accepted content is already on disk and asserting recovery reaches 'synced' with file_version advanced. Found by adversarial review of the recovery sweep. Signed-off-by: phernandez <paul@basicmachines.co>
…coupling seam Commit 9015127 collapsed LocalProjectIndexRuntimeProvider to the concrete LocalProjectIndexRuntimeFactory on the premise it was the sole impl / pure indirection. That premise was wrong: services/initialization.py declares its own matching InitialProjectIndexRuntimeFactory Protocol and passes a value of that type into run_local_project_index_for_project. The structural Protocol was the boundary that let the services layer supply a factory without importing the concrete class from the index layer, so narrowing the parameter to the concrete broke the assignment (ty: Expected LocalProjectIndexRuntimeFactory, found InitialProjectIndexRuntimeFactory). Restore the Protocol and re-type the parameter. Caught by the full-tree ty check (the collapse's per-chunk typecheck did not cover services/). Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5073c83a4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if error.filename is not None and Path(error.filename) == project_root: | ||
| raise error | ||
| logger.warning("Skipping unreadable directory during project scan", path=error.filename) |
There was a problem hiding this comment.
When a previously indexed subdirectory becomes unreadable, this onerror path only logs and lets os.walk omit the entire subtree from observed_files. The metadata-error guard only covers files that were already yielded; this earlier walk error still produces a partial snapshot, and the coordinator compares it to all indexed paths and runs delete batches for every DB row under the skipped subtree. Treat any traversal error as non-destructive (or abort the scan) so a transient permission/mount issue does not remove valid entities and search rows.
Useful? React with 👍 / 👎.
1 participant
Why
This PR started as the explicit-session repository refactor and grew into the core half of the local/cloud runtime parity work. The goal is for
basic-memoryto own portable orchestration while cloud and local runtimes provide their own dependency adapters.Paired cloud PR: basicmachines-co/basic-memory-cloud#1345.
Cloud parity document: basic-memory-cloud/docs/local-cloud-runtime-parity.md.
Closes #750.
What Changed
basic_memory.runtimefor storage events, note-content payloads, job identity/status, workflow metadata, materialization, and project/file indexing values.basic_memory.cloudfacades for note-content reads/writes, directory deletes, project deletes, and materialization so cloud/local APIs can share route-facing orchestration.basic_memory.indexas the event-based indexing orchestration layer for storage events, local filesystem watching, project fanout, and inline local processing.basic_memory.indexingwith provider-neutral runners for accepted-note mutation/write flows, file indexing, project indexing, directory deletes, relation/vector follow-ups, and progress/result accounting.basic_memory.synccode and tests out of the active package/test tree so it is reference material only while the new index path becomes the clear file-indexing path.Public Surface
Verification
Not rerun during PR publication. The branch has been validated incrementally during the refactor with focused core tests,
just fast-check,just package-check, and paired cloud checks. Final broad core and cloud checks should run before merge.Review Notes
This PR should be reviewed with the paired cloud PR because the ownership split is intentional: core owns portable orchestration; cloud owns hosted adapters and tenant/runtime wiring.