refactor(core): structure campaign — collapse seams, honest types, typed boundaries - #1054
Merged
Conversation
…ield types The note-content runtime protocols declared entity and note_content row fields as object/object | None even though every implementation is the Entity/NoteContent ORM model (or a cloud row wrapper with the same str/int columns). That forced downstream isinstance/str()/int() re-validation of values the type system already knows. - Narrow RuntimeDeletedNoteEntitySource, RuntimeDeletedNoteEntityChecksumSource, RuntimeDeletedNoteFileChecksumSource, RuntimePendingNoteMaterializationSource, RuntimeMaterializedNoteSource, RuntimeNoteContentDbVersionSource and RuntimeNoteContentVersionSource to the str/str | None/int types the ORM columns actually carry. - Delete required_runtime_deleted_note_text: RuntimeDeletedNoteReference.from_entity now assigns external_id/title directly. runtime_deleted_note_permalink keeps the real None/blank -> file-path fallback but takes str | None. - Drop the str()/int() coercions in select_deleted_note_file_checksum, plan_previous_materialized_note_file_delete, next_runtime_note_content_version, plan_pending_note_materialization and note_content_matches_materialization_request. - Update test fakes to the honest field types and drop the tests that pinned the deleted coercion behavior (padded-string stripping, str db_version). RuntimeNoteContentVersionInput in runtime/storage.py is now unreferenced in this module; the alias itself is left for the cluster that owns storage.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…light NoteMaterializationContentSource.file_checksum was typed object | None even though its only implementation is the NoteContent ORM row (file_checksum: str | None), forcing str() re-validation in the preflight plan. With the protocol narrowed to RuntimeFileChecksum | None the str() wrappers on markdown_content/file_checksum and the int() on the ORM db_version in the repository publisher are dead weight; pass the typed values through. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…uilders FileIndexResult.from_fields and CurrentMaterializedNoteEntity.from_fields took external_id/title/permalink/checksum as object and re-validated them through three near-identical private text validators, even though every caller passes fields straight off the Entity ORM model (external_id: str, title: str, permalink: str | None, checksum: str | None). - Narrow both classmethod signatures to the ORM field types and delete _required_file_index_result_text, _optional_file_index_result_text and _required_current_materialized_note_text. FileIndexResult.from_fields is now a plain construction seam; CurrentMaterializedNoteEntity.from_fields keeps the one real check - a current markdown note must carry a permalink for its live-update identity - as an explicit None guard. - Narrow IndexMarkdownEntity (file_indexer.py) to the same honest types; it is the repository-facing protocol whose object-typed properties forced the object parameters in the first place, and its implementations (Entity ORM rows, test fakes) already carry str fields. - _required_index_file_note_live_update_text keeps its blank/None rejection (IndexFileJobResult fields are genuinely optional) but now takes str | None instead of isinstance-checking object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
LocalWatchProjectSource.path was typed object and the permalink/name identity lived in a separate runtime_checkable protocol dispatched with isinstance inside local_project_prefix. Every production caller passes either a Project ORM row (path/name/permalink: str) or a config ProjectEntry (path: str), so the object typing and runtime dispatch only served under-specified test doubles. - LocalWatchProjectSource.path is now str; local_project_root drops its str() coercion and keeps the empty-path ValueError. - LocalWatchProjectIdentitySource extends the path protocol with permalink/name as str | None and local_project_prefix takes it directly: no runtime_checkable, no isinstance. The root-directory-name fallback for a None/blank identity is unchanged. - from_project_changes now requires the identity shape it always consumed via local_project_prefix; the path-only test double gains explicit permalink=None/name=None and keeps asserting the fallback prefix. Path-only sources (config ProjectEntry in the watch filter) still satisfy the base protocol for local_watch_filter_roots and change batching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
All six cast(AcceptedNoteMutationChange, ...) calls papered over payload widening that the type system already accepts: RuntimeAcceptedNoteChange is a frozen generic dataclass, so its payload parameter is inferred covariant and RuntimeAcceptedNoteChange[RuntimeAcceptedNoteResponse] / RuntimeAcceptedNoteChange[dict[str, object]] are directly assignable to the RuntimeNoteContentResponsePayload union alias the run_* functions declare. Return the planned change directly and drop the cast import. Public function signatures are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…oped_session Seven private _session_scope reimplementations (entity_service, search_service, directory_service, link_resolver, context_service, index/watch_service, indexing/batch_indexer) drifted between two semantics: an always-fresh service-owned transaction and an optional caller-provided session passthrough. Extend db.scoped_session with an optional caller-owned session so one shared helper covers both variants, and replace every private copy with direct calls. Passthrough call sites keep caller commit/rollback ownership; owned call sites keep commit-on-success/rollback-on-error semantics. ContextService keeps its fail-fast guard for a missing session maker via _require_session_maker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
IndexingResult/IndexingResultState and VectorSyncProgress/ VectorSyncProgressState were field-identical pairs joined by ~120 lines of hand-written field transcription in to_checkpoint_state and from_checkpoint_state. Each pair collapses into one Pydantic CheckpointModel that is both the runtime value and the persisted checkpoint document, so the transcription (and the State classes) disappear. The dumped JSON stays byte-identical: field names, order, the computed entities_total field, and the 3-decimal rounding applied at dump time are unchanged, and new shape-pinning tests assert the exact document (including key order) plus a restore from the old shape. Validation now also runs at construction, so in-memory values agree with what a checkpoint restore would produce; without_entity_ids uses model_copy to keep next_index exactly as recorded. Cloud consumers of IndexingResultState / VectorSyncProgressState must switch to the collapsed classes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…real unions ProjectIndexWorkflowStartPlan, ProjectIndexWorkflowRecordPlan, and ProjectIndexStaleWorkflowPlan each simulated a tagged union with a status Literal, optional fields, __post_init__ consistency guards, and require_* accessors. Each becomes a type alias over frozen dataclass variants that carry exactly the fields their state allows: - start: ProjectIndexWorkflowStartRunning | ProjectIndexWorkflowStartComplete - record: ProjectIndexWorkflowRecordProgress | ProjectIndexWorkflowRecordComplete | ProjectIndexWorkflowAlreadyRecorded - stale: ProjectIndexStaleWorkflowKeepRunning | ProjectIndexStaleWorkflowFail Invalid states are now unrepresentable, so the guards, accessors, and status/is_complete/should_fail/should_emit_progress_event helpers are deleted. The planner functions and the update payloads they build are unchanged; a new test covers the batch-record progress path that the old suite left unexercised. Cloud consumers must switch from status/require_* access to isinstance or match on the variant classes; the union alias names are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Project-index workflow metadata was validated with Pydantic on read (project_index_progress state models) but written by stringly-typed dict[str, object] literals and key-by-key mutation. The write side now mirrors the read side: typed CheckpointModel documents for the start metadata (with nested discovery), the progress/completion/failure overlay fields, and the stale diagnostics, plus a frozen dataclass for the attempt event whose serializer keeps the transport splice between discovery counts and project identity. Builders dump these models into the metadata copy, so every persisted key and its order is pinned by a field definition instead of a string. Dumped shapes are unchanged; tests now also pin document key order and the absence of recorded_batches in per-file progress updates. legacy_missing_batch_count is typed as the bool it has always carried from ProjectIndexMissingBatches (persisted as JSON true/false); a new test covers the legacy stale-failure branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The accepted-note write path wired repositories through five layers of indirection: per-project factory functions, a Default dataclass wrapping them, a build_default_* factory, and a second Default dataclass in the mutation runner duplicating the same wiring, with a combined protocol on top. Exactly one concrete implementation existed, and every production call already passed repositories explicitly through AcceptedNoteMutationDependencies. Collapse the tower to one plain dataclass, LocalAcceptedNoteRepositories, defined at the local composition root (deps/services.py) and constructed once in get_note_content_mutation_service. Delete the six hidden 'repositories or build_default_accepted_note_write_repositories()' fallbacks by making repositories a required parameter of the leaf persistence functions, so composition is pushed up per house style. The capability protocols (AcceptedNoteWriteRepositories, AcceptedNoteMutationRepositories, and the per-repository protocols) stay: they are the seams test fakes and the cloud runtime implement. Deleted symbols (basic-memory-cloud contract): - accepted_entity_repository_for_project - accepted_note_content_repository_for_project - accepted_note_search_repository_for_project - DefaultAcceptedNoteWriteRepositories - build_default_accepted_note_write_repositories - AcceptedNoteRepositories (combined protocol) - DefaultAcceptedNoteRepositories - build_default_accepted_note_repositories Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
ProjectDeleteRepositories was a one-method factory protocol whose only implementation wrapped the argument-less ProjectRepository constructor, so RepositoryProjectHardDeleter reached its repository through a valueless bundle hop. Hold the ProjectDeleteRepository capability directly on the hard deleter instead, defaulting to the core ProjectRepository for the local runtime. Deleted/renamed symbols (basic-memory-cloud contract): - ProjectDeleteRepositories (protocol, deleted) - DefaultProjectDeleteRepositories (deleted) - RepositoryProjectHardDeleter.repositories field renamed to project_repository and now takes the repository, not a factory bundle Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The index-file runner carried five metadata types where two do the job: IndexFileCurrentMetadata and IndexFileCurrentMetadataSource were structural twins of IndexFileObjectMetadata and IndexFileMetadataSource, and the StorageIndexFileMetadataSource adapter between them was a no-op field copy. Delete the twin protocols and the adapter; storage runtimes now return IndexFileObjectMetadata directly through the one IndexFileMetadataSource seam, which is what the local runtime already did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
DefaultIndexBatchRuntime only forwarded index_loaded_files to the inner IndexBatchRuntime; its extra note_content_reconciler field was never read. build_default_index_batch_runtime now returns the composed IndexBatchRuntime[Entity, FileInfoT] directly, and the local file-batch adapter types its runtime accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…allable The batch reconciliation clock was injected through a doubled seam: an IndexedNoteContentTimestampProvider protocol, a Default* dataclass wrapper around indexed_note_content_observed_at, and the module-level function itself as a third layer. Replace the protocol and wrapper with a single IndexedNoteContentObservedAt callable alias whose default is the existing indexed_note_content_observed_at; indexed_note_content_utc_now stays as the one real clock hook. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Reaching the per-project note_content repository went through four layers: note_content_repository_for_project, the NoteContentRepositories protocol, the DefaultNoteContentRepositories wrapper, and build_default_note_content_repositories. Keep the leaf factory and inject it as a NoteContentStoreFactory callable (default: note_content_repository_for_project) on the materialization publishers and the failure marker. Also fold the mark_note_materialization_enqueue_failed free function into RepositoryNoteMaterializationFailureMarker.mark_note_materialization_failed; the marker was its only caller, so the dual entry points become one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The metadata-only branch of note-content reads dumped EntityResponseV2 with an inline hardcoded string set, so the response boundary degraded to an anonymous payload with no drift protection. Hoist the exclusions into ENTITY_METADATA_PAYLOAD_EXCLUDE and a named entity_metadata_response_payload builder returning RuntimeNoteContentResponsePayload; the dumped JSON is byte-identical (same EntityResponseV2 dump, same exclusions). Tests pin every excluded name to a real EntityResponseV2 field and assert the payload keys are exactly the model fields minus the exclusions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
note_content_read_repair_runner carried a generic repository-capability lattice (five entity/project source protocols, six repository capability protocols, capability-set and reconciler-provider protocols, six factory functions, and three Default* wrapper dataclasses) with exactly one implementation: the core ProjectRepository/EntityRepository/ NoteContentRepository stack. Only the *_with_default_* entry points were ever called outside tests, so the generic load/prepare/apply/run functions and the whole lattice collapse into those entry points, which now use the default repositories and NoteContentReconciler directly. The public surface consumed by deps/services.py and cloud/note_content_reads.py keeps every name and signature: NoteContentReadView, NoteContentReadRepairFileReader (the real storage seam), load_note_content_read_view_with_default_repositories, note_content_response_payload_from_read_view, note_content_resource_from_read_view, prepare_note_content_read_repair_with_default_repositories, and run_note_content_read_repair_with_default_reconciler. Tests move from fake-repository seams to real DB-backed fixtures and keep the module at 100% coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
… publisher The RepositoryNoteMaterializationPublisher branch that records a written file as pending when a newer accepted version superseded the request had no direct unit coverage; it now contains the injected note_content_store call, so pin its behavior: the note_content update applies with the version guard and the entity row is left untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…rojectItem The accepted project-delete response hand-rolled a dict[str, object] project snapshot (ProjectDeleteAcceptedProject plus its Source protocol) that duplicated the existing Pydantic ProjectItem field-for-field. Collapse the duplicate: ProjectDeleteAcceptedResult.old_project is now a ProjectItem and the payload serializes it with model_dump limited to the persisted project fields, so response bytes are unchanged (locked by the exact payload snapshot test). The None->False is_default mapping moves to the one place that reads the ORM row, mirroring the v2 project router. Campaign #1053 finding [4]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
DirectoryDeleteService.delete_directory flattened its typed result into tuple[int, dict[str, object]] and the route rebuilt meaning from anonymous dict keys. The service now returns DirectoryDeleteAcceptedResult directly: the route status moves onto the result as http_status_code (next to the DirectoryDeleteRejectKind.http_status_code precedent) and the response payload is typed as a DirectoryDeleteResponsePayload TypedDict matching the existing contract, including the NotRequired error field. Response bytes and status codes are unchanged — locked by a new exact route-JSON snapshot plus the existing payload-shape tests. Campaign #1053 findings [15] and [30]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…nner The rule that decides which storage objects may prove a move (no indexed row at the path, checksum known) lived twice: as ChangeDetectionSnapshot.new_file_checksum_by_path and as an inline loop in plan_file_changes. The duplication forced detect_project_file_changes to build a throwaway ChangeDetectionSnapshot just to evaluate the property before building the real one. plan_move_target_checksums now owns the rule; both the detector and plan_file_changes call it, and the snapshot is constructed exactly once per detection pass. Classification output is unchanged (same rule, same iteration order). Campaign #1053 finding [26]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…ate phases The move-batch apply was a ~250-line method interleaving row loading, concurrent-destination screening, content-repair planning, a triple parallel case() assembly, four UPDATE statements, post-commit file writes, and outcome reporting with no section structure. It now reads as chapters: the eligibility screen (_screen_replaced_move_targets) and the parallel CASE assembly (_build_move_batch_update_values) are pure module helpers with their own value objects, while session-bound loading, statement execution, content planning, and post-commit writes are focused store methods. No behavior change: same statements in the same order, verified by the existing statement-sequence tests. Campaign #1053 finding [28]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
RuntimeStorageEventProcessingResult was a pure delegation wrapper over RuntimeJobCounts: every classmethod and accumulator forwarded to the counts field and added nothing. The storage-event runners (run_runtime_storage_event_operations, run_storage_event_indexing, run_storage_event_bucket_indexing, run_local_watch_event_indexing), the StorageEventBucketContextProcessor protocol, and the local watch status planner now speak RuntimeJobCounts directly; the watch service log line drops the .counts hop. Counting behavior is unchanged. Also updates the watch_service.py call site (outside the cluster's expected file list) for the .counts attribute removal - a four-line mechanical change with no active sibling-cluster edits to that file. Campaign #1053 finding [39]. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
RelationResolutionPass and UnresolvedRelationCounter existed only to be recombined into RelationResolutionRuntime, and every caller binds both capabilities to the same runtime object. Fold the two methods directly into RelationResolutionRuntime and inline resolve_relations_until_stable into resolve_project_relations, which was its only production caller. Consumers in deps/services.py, index/local_*, and services/composition.py keep the same names and signatures (RelationResolutionRuntime, resolve_project_relations). Cloud-visible contract changes: - removed protocol RelationResolutionPass - removed protocol UnresolvedRelationCounter - removed function resolve_relations_until_stable Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…nestly ProjectIndexRelationResolutionContext carried int | str | None fields with an int() coercion even though every producer builds it from a typed ProjectRuntimeReference (project_id: int, project_path: str), and resolve_project_index_completion_relations abused the queue-job planner as a None guard. Narrow the fields to int/str, drop the coercion and the guard, and keep the planner as the pure context -> queue request seam. resolve_project_index_completion_relations keeps its signature for index/local_project.py; its return type narrows to a non-optional ResolveRelationsResult. Cloud-visible contract changes: - ProjectIndexRelationResolutionContext.project_id: int (was int | str | None); project_path: str (was str | None) - callers must coerce/validate first - plan_project_index_completion_relation_resolution returns ResolveRelationsJobRequest (was ... | None) - resolve_project_index_completion_relations returns ResolveRelationsResult (was ... | None) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…stly
RelationResolutionRelationRepository.update declared a dict[str, object]
payload, an object | None return, and called its relation id entity_id.
The resolver only ever sends {to_id: int, to_name: str} and the bound
implementation (the generic model repository) returns Relation | None,
so declare exactly that. Positional-only parameters let the contract
name the relation id honestly while the shared repository method keeps
its entity-oriented parameter names; delete gets the same treatment.
Concrete bindings in deps/services.py, services/composition.py, and
index/local_* keep passing the unchanged RelationRepository.
Cloud-visible contract changes:
- RelationResolutionRelationRepository.update: parameters are now
positional-only (session, relation_id, resolved_target_fields, /),
payload typed dict[str, int | str], return typed Relation | None
- RelationResolutionRelationRepository.delete: parameters are now
positional-only (session, relation_id, /)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
…ution forward_reference_resolution.py carried a parallel protocol family that duplicated relation_resolution's shapes one-to-one: UnresolvedForwardReference mirrored UnresolvedRelation, ForwardReferenceEntityRepository mirrored RelationResolutionEntityRepository (identical find_by_id), and ForwardReferenceEntityIndexer mirrored RelationResolutionEntityIndexer, plus ForwardReferenceEntityId/ForwardReferenceRelationId aliases of int. Both families are satisfied by the same Relation rows, EntityRepository, and SearchService in production, so the bulk (forward-reference) module now consumes relation_resolution's protocols directly. The forward-reference-specific capabilities (ForwardReferenceRelationSource, ForwardReferenceResolutionRuntime, ForwardReferenceEntityRefreshRuntime) and their Repository defaults keep their names. Cloud-visible contract changes: - removed protocol UnresolvedForwardReference (use relation_resolution.UnresolvedRelation; adds relation_type: str and makes to_name a non-optional str) - removed protocol ForwardReferenceEntityRepository (use relation_resolution.RelationResolutionEntityRepository) - removed protocol ForwardReferenceEntityIndexer (use relation_resolution.RelationResolutionEntityIndexer; index_entity returns None instead of object) - removed type aliases ForwardReferenceEntityId and ForwardReferenceRelationId (use relation_resolution.EntityId / int) - build_default_project_index_runtime entity_repository/entity_indexer parameters are now typed with the relation_resolution protocols Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
… runner ProjectIndexRuntime.run_move_batches/run_delete_batches were verbatim copies of StoreProjectIndexMaintenanceRunner: both carried move/delete stores and forwarded them to run_project_index_move_batches / run_project_index_delete_batches. The runtime now holds one ProjectIndexMaintenanceRunner and delegates, so project_index_maintenance owns batching orchestration in exactly one place. ProjectIndexForwardReferenceRun also loses its eight one-line forwarding properties; callers read the underlying resolution/refresh results directly (run.resolution.resolved_count, run.refresh.failures, ...). Cloud-visible contract changes: - ProjectIndexRuntime fields move_store/delete_store replaced by maintenance: ProjectIndexMaintenanceRunner (run_move_batches / run_delete_batches keep their signatures) - ProjectIndexForwardReferenceRun properties removed: initial_count, unique_link_text_count, resolved_link_text_count, resolved_count, remaining_count, entity_ids_to_refresh, successful_reindexed_entity_ids, refresh_failures (use .resolution.* / .refresh.* instead) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
runtime/job_payloads.py layered four protocols/factories over the concrete payload models - RuntimeSerializedJobPayload, RuntimeJobPayloadSource, RuntimeJobPayloadSerializer, and RuntimePayloadJobEnqueuer plus enqueue_runtime_job_payload - with zero in-repo production consumers. The concrete Pydantic payloads already own runtime_job_request() construction, so runtimes enqueue with runtime.enqueue(payload.runtime_job_request(headers=...)) directly. The module keeps the cloud queue contract: DELETE_NOTE_FILE_ENTRYPOINT, MATERIALIZE_NOTE_FILE_ENTRYPOINT, RuntimeNoteFileDeleteJobPayload, and RuntimeNoteMaterializationJobPayload. Cloud-visible contract changes: - removed protocol RuntimeSerializedJobPayload - removed protocol RuntimeJobPayloadSource - removed protocol RuntimeJobPayloadSerializer - removed factory RuntimePayloadJobEnqueuer - removed function enqueue_runtime_job_payload Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
deps/services.py had grown into a runtime-implementation module: the accepted-note preparer factory, current-note content freshener, note-file delete storage/enqueuer, directory-delete relation cleanup refresher, the four background schedulers with their task machinery, and the project-index route command all lived inside the FastAPI composition root. Move them into the index package so the composition root only wires dependencies: - index/local_notes.py: accepted-note mutation and note-file cleanup implementations (LocalAcceptedNotePreparerFactory, LocalAcceptedNoteRepositories, LocalCurrentNoteContentFreshener and its protocols, LocalNoteFileDeleteStorage, LocalDirectoryFileDeleteEnqueuer, LocalDirectoryDeleteRelationCleanupRefresher) - index/local_schedulers.py: background-task machinery (drain_background_tasks and friends) plus the four local schedulers and their capability protocols - index/local_project.py: project-index route protocols (ProjectIndexRunner/Observer/Scheduler/Command), ProjectIndexRouteRequest, and LocalProjectIndexCommand, next to the LocalProjectIndexRunner they orchestrate deps/services.py keeps every provider and Dep annotation and re-exports drain_background_tasks for the API/MCP/CLI lifespans, so those call sites are untouched. ProjectIndexRouteRequest is no longer exported from basic_memory.deps; the route and tests import it from basic_memory.index.local_project. Scheduler and accepted-note tests move to tests/index alongside the code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The index_project route had no response model and LocalProjectIndexCommand returned an untyped dict for the background branch. Model both shapes the command actually returns as a v2 response union: - schemas/v2/project_index.py: ProjectIndexStartedResponse (background ack) and ProjectIndexResponse = ProjectIndexRunResponse | ProjectIndexStartedResponse - ProjectIndexCommand protocol and LocalProjectIndexCommand now return ProjectIndexResponse; the background branch returns the typed ack instead of a hand-built dict - the route declares response_model=ProjectIndexResponse, so the OpenAPI schema names the union instead of an untyped object Response bytes are unchanged; snapshot tests pin the exact payload for both run_in_background variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Merging VectorSyncProgressState into VectorSyncProgress made to_checkpoint_state() dump through model_copy, which skips validation. On main the write path constructed the state model, so the persisted checkpoint always ran the dedupe_ids/clamp_next_index validators and kept next_index <= len(entity_ids) even after batch folds mutated the offset or without_entity_ids() dropped the plan. Restore that invariant by constructing a validated instance at write time; in-memory semantics (raw offsets between writes) are unchanged, matching main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The structure campaign narrowed ProjectIndexRelationResolutionContext to int/str and made plan_project_index_completion_relation_resolution unconditionally build the queue request. On main the planner skipped (returned None) when project identity was missing and coerced string project ids via int(), and resolve_project_index_completion_relations propagated the skip. Downstream runtimes rebuild this context from legacy workflow metadata, so the guard is a live contract even though the in-repo caller always passes typed values. Restore the wide types, the None skip, the int() coercion, and the pinning test assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The structure campaign dropped the strip/blank-raise validation towers from FileIndexResult.from_fields and CurrentMaterializedNoteEntity .from_fields, so raw ORM values flowed into index-file job results and live-update payloads: padded identity text was no longer stripped, a blank title no longer failed the job at result construction, and the str() checksum coercion was gone. Restore the object-typed helpers so workflow/batch results fail fast on malformed index rows and serialized result bytes match main, and restore the deleted raises-on-blank pin plus new branch coverage for the optional-permalink guards. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
…ates The structure campaign narrowed RuntimeDeletedNoteEntitySource to str-typed properties and deleted required_runtime_deleted_note_text, so RuntimeDeletedNoteReference.from_entity passed raw ORM values into the delete live-update payload: padded external_id/title were no longer stripped and an empty title no longer failed the delete. Downstream runtimes feed loosely typed entity projections through this seam, so restore the object-typed protocol, the strip/raise helper, and the object-tolerant permalink fallback, plus the pinning tests that were rewritten to avoid the old behavior (padded-identity strip, raise on blank, reject-missing-identity parametrization). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The structure campaign narrowed RuntimeNoteContentDbVersionSource / RuntimeNoteContentVersionSource to int/str and dropped the int()/str() coercion from next_runtime_note_content_version and note_content_matches_materialization_request, deleting the tests that documented the tolerance. A driver or replayed job payload delivering a string db_version now made matching silently return False (skipping the materialization publish) and made the version bump raise TypeError. Restore the input-typed protocol properties, the coercion, and the string-tolerance tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The structure campaign made LocalWatchProjectIdentitySource a required str-typed extension of LocalWatchProjectSource: local_project_prefix accessed permalink/name unconditionally (AttributeError for structural callers without those attributes) and local_project_root dropped the str() coercion (Path-typed paths crashed at .strip()). Downstream runtimes compose leaner project objects against this seam, so restore the runtime_checkable optional-identity protocol, the isinstance guard with directory-name fallback, and the str() coercions, and re-pin the attribute-less test double alongside the None-identity fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The scheduler machinery moved to basic_memory.index.local_schedulers, but deps/services.py kept a re-export alias so api/app.py, mcp/server.py, and cli/commands/command_utils.py could keep importing the moved runtime behavior from the composition root — contradicting that module's own 'no runtime behavior of its own' docstring. Point the three lifespan call sites at index.local_schedulers directly and drop the alias; the CLI cleanup test now monkeypatches the owning module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Completes the version-guard coercion restore: main also kept RuntimePendingNoteMaterializationSource input-typed (string db_version, object db_checksum/last_source from replayed job payloads) and coerced in plan_pending_note_materialization with int()/str(). The previous commit restored the guard functions; without this half the shared protocol seam stayed narrowed and ty flagged the restored string-tolerance doubles. Pin the coercion with a replayed-payload test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The restored str()-coercing local_project_root keeps the ValueError on a falsy project path; pin that branch so the restored guard stays at full coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Codex adversarial review: converting IndexingResult/VectorSyncProgress to CheckpointModel made runtime construction lenient — extra=ignore swallowed mistyped keywords and normalize_errors silently dropped malformed error entries, flipping success to True. Runtime construction is now extra=forbid with fail-fast error entries; legacy-document tolerance (retired fields, old error shapes, per-entry drops) is scoped to from_checkpoint_state. Metadata-slice models keep extra=ignore — parsing a subset of a larger workflow document is their contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! 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". |
This was referenced Jul 15, 2026
Closed
phernandez
added a commit
that referenced
this pull request
Jul 15, 2026
DELETE /v2/projects/{id}/knowledge/entities/{id} used to return 500 for
note_type="file" entities (issue #1033) because the delete path did
markdown-specific cleanup. The #1002/#1054 refactor fixed this by routing
single-entity delete through the shared accepted-note delete path, but no
test covered the non-markdown case.
Add a regression test that indexes a real .csv file through the local
project indexer (note_type="file", no permalink, no note_content row),
deletes it via the v2 endpoint, and asserts the entity row and its search
index rows are gone.
Refs #1033
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
This was referenced Jul 18, 2026
14 tasks
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
1 participant
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.
Closes #1053.
Behavior-preserving structure campaign over the runtime/indexing extraction, executed as 8 clusters in 3 integration waves (41 commits):
cast(AcceptedNoteMutationChange, ...)removed._session_scopeconsolidation: all seven private copies replaced bydb.scoped_sessionwith an optional caller-owned-session passthrough (-51 lines, per-site semantics preserved).from_checkpoint_state.__post_init__+require_*) replaced with type unions.index/local_project.py/index/local_schedulers.py.Deliberately skipped (recorded on the issue): the runtime/indexing/index package-split question (needs a naming decision, ~70 module moves).
Verification: full unit suite green (3633 passed); ty at the 9-diagnostic main baseline; every wave integrated behind a full-suite gate. Reviewed twice: a three-lens adversarial pass (17 confirmed findings, all fixed with regression tests — behavior drift restored to main parity) and a cross-vendor Claude+Codex review (1 confirmed finding fixed: fail-fast runtime checkpoints; 1 contested finding dispositioned to the cloud adaptation).
Cloud consumers: 78 recorded contract changes (symbol deletions/renames/narrowings) — the paired cloud adaptation lands via #1489 with the re-pin.
🤖 Generated with Claude Code