feat(core): order accepted project changes - #1382
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
d723b19 to
5b9501a
Compare
![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: d723b197d7
ℹ️ 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".
| entity_id=materialization.entity_id, | ||
| db_version=materialization.db_version, | ||
| db_checksum=materialization.db_checksum, | ||
| project_change=materialization.project_change, |
There was a problem hiding this comment.
When a hosted enqueuer converts this request through RuntimeNoteMaterializationJobPayload.from_runtime_request, the payload model and both conversion methods omit project_change, so the worker reconstructs the request with project_change=None; RuntimeNoteFileDeleteJobPayload has the same omission for deletes. The transaction has already advanced the durable partition head, so queued materializations and deletes permanently lose the corresponding ordered change and downstream consumers observe a gap that retries cannot repair. Add this evidence to both Pydantic worker payload round trips.
AGENTS.md reference: AGENTS.md:L128-L129
Useful? React with 👍 / 👎.
Signed-off-by: phernandez <paul@basicmachines.co>
5b9501a to
3897cc3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3897cc3918
ℹ️ 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".
| change = await delete_accepted_note( | ||
| session, | ||
| project_id=project.id, | ||
| entity=entity, | ||
| note_content=note_content, |
There was a problem hiding this comment.
When an update or move commits after entity/note_content are loaded but before delete_accepted_note acquires its existing NoteContent lock, these ORM objects retain the old title, path, version, and checksum; the delete then removes the newer row but permanently records the deletion using this stale snapshot. In particular, a concurrent move can leave the new directory's wiki projection unchanged because the durable delete event names only the old path, and no later pass can correct the journal entry. Acquire the existing lock before loading the evidence, or explicitly refresh the locked entity and NoteContent before constructing both the delete plan and project_change.
AGENTS.md reference: AGENTS.md:L184-L188
Useful? React with 👍 / 👎.
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: c5d35ec7fa
ℹ️ 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".
| accepted_note_changes = relationship( | ||
| "AcceptedProjectNoteChange", | ||
| back_populates="project", | ||
| cascade="all, delete-orphan", |
There was a problem hiding this comment.
When a long-lived project with a large mutation history is deleted, cascade="all, delete-orphan" with the default passive_deletes=False makes SQLAlchemy load every journal row and delete it through the ORM, even though the foreign key already specifies ON DELETE CASCADE. Because this table grows once per accepted mutation rather than once per note, project deletion can consume excessive memory or time out; configure the relationship with passive_deletes=True or delete these rows in bulk.
Useful? React with 👍 / 👎.
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: 7bbafd3ce4
ℹ️ 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".
| AcceptedProjectNoteChange.project_id == project_id, | ||
| AcceptedProjectNoteChange.partition_position == partition_position, | ||
| AcceptedProjectNoteChange.materialized_at.is_(None), |
There was a problem hiding this comment.
When two writes to the same note are accepted before the first materialization runs, the first job exits as stale after the second advances db_version, while this update API can mark only the exact newer partition position. The older journal row therefore remains materialized_at = NULL permanently; plan_wiki_projection() treats every such position above its watermark as pending, so one normal rapid-save race blocks that project's projector and all subsequent changes indefinitely. Mark superseded positions as satisfied when a newer generation materializes, or represent a terminal superseded state rather than leaving them indistinguishable from pending.
AGENTS.md reference: AGENTS.md:L184-L186
Useful? React with 👍 / 👎.
Signed-off-by: phernandez <paul@basicmachines.co>
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: 1985fb1b21
ℹ️ 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".
| graph_silent_markdown = prepared.entity_markdown.model_copy( | ||
| update={"observations": [], "relations": []} | ||
| ) |
There was a problem hiding this comment.
When publish_graph_facts=False is used for a derived note containing observation or relation syntax, this only clears the parsed model used by the accept transaction; the canonical Markdown remains unchanged, and the local materialization flow immediately calls file_indexer.index_file() (note_content_materialization.py:722-727), which reparses those bytes and publishes the facts again. The generated Wiki note therefore becomes part of the graph after materialization—and every later reindex restores that wrong state—defeating the recursion guard. Carry the graph-silent policy into indexing or otherwise suppress facts based on durable provenance.
AGENTS.md reference: AGENTS.md:L161-L166
Useful? React with 👍 / 👎.
Signed-off-by: phernandez <paul@basicmachines.co>
8dfd16a to
1879452
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
1 participant
Why
The deterministic Wiki projector needs a stable source watermark and durable, replayable accepted-change evidence. Timestamps, webhook arrival, and workspace activity cannot provide the strict project-local order required by SPEC-88 or the compatible future journal described by SPEC-83.
This is the second Core PR in the Spec 88 stack. It builds on #1381 and supports basicmachines-co/basic-memory-cloud#1863.
What Changed
Project.partition_positioncounter and Alembic migration.Implementation Details
ProjectRepository.advance_partition_position()uses one SQLUPDATE ... SET partition_position = partition_position + 1 RETURNING partition_position, keeping the claim atomic on SQLite and PostgreSQL. The position claim andAcceptedProjectNoteChangerow are written before the caller-owned transaction commits, so note state and ordering evidence succeed or roll back together.The journal deliberately retains delete evidence after an entity is removed and exposes ordered range reads plus an idempotent materialization marker.
RuntimeAcceptedProjectNoteChangeremains runtime-neutral and storage-neutral; it is attached to existing materialization and cleanup requests instead of introducing a second queue or filesystem.Testing
just fast-checkuv run pytest -q tests/repository/test_project_partition_repository.py tests/indexing/test_accepted_note_mutation_runner.py tests/api/v2/test_accepted_note_atomicity.pyuv run pytest -q tests/repository/test_project_partition_repository.py tests/indexing/test_accepted_note_mutation_runner.py tests/api/v2/test_accepted_note_atomicity.py tests/runtime/test_runtime_job_payloads.py tests/runtime/test_project_partition.pyBASIC_MEMORY_TEST_POSTGRES=1 uv run pytest -q tests/repository/test_project_partition_repository.pyuv run pytest -q tests/test_note_content_migration.pys2p3e4c5w6k7as the single head.+1to+2failed the ordering tests; removing the accepted-journal write failed the end-to-end API materialization-boundary test. Restoring each implementation returned the focused tests to green.Risks / Follow-ups