Repository navigation
Preserve partial-import bridge relationships and clear authority - #18
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. |
![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: b865b569f2
ℹ️ 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 existing is None: | ||
| ent = {**public_ent, "visibility": public_ent.get("visibility", "public")} | ||
| entities.append(ent) |
There was a problem hiding this comment.
When a public entity has a non-shared% project, this code imports it on peers as public, but a later privacy revocation is never transported: _export_public_knowledge omits entities after they become private, while export_entity_files only exports shared% projects. The peer therefore keeps and may continue republishing the entity as public indefinitely. Public-only imports need a revocation/tombstone mechanism or a transport path for the newer private visibility.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 141cfae. Removed the public_knowledge-only import union; explicit shared/index visibility remains authoritative.
The new regression uses the actual source exporter and peer importer, then revokes visibility at the source and reexports from the peer. Before the fix, the peer still exported the non-shared entity as public; after the fix, public-only entities are not imported, and the shared-entity variant correctly transports public-to-private revocation. Final entity privacy suite: 8 passed; required bridge smoke: 115 passed. Updated full CI is running.
This prevents new unsafe imports. It does not migrate, demote or delete entities already imported by earlier code, and does not claim full public-projection replication.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It rewrites core cross-machine sync/merge/export logic governing foreign-key integrity, LWW parent authority, and privacy, which warrants human verification even though no objective defect was found.
Review effort: Balanced
Findings: None
What changed in this PR
This PR hardens the SQLite-memory bridge sync against data loss during partial-peer imports. When a peer holds a task's parent_id field clock but the referenced parent hasn't arrived locally (local FK is NULL), the previous export could drop the unresolved relationship and its clock, and replaying the causal event could fail the foreign-key check. The change preserves the transport parent value/clock, materializes only locally present parents, and records explicit parent-CLEAR intent so a stale snapshot cannot resurrect a cleared edge. It also carries related bootstrap-preservation fixes (orphan attachment bytes, unresolved entity links, entity visibility) and adds an opt-in BRIDGE_PRIVATE_ONLY worker mode that suppresses peer delivery, public promotion, releases, and Cloudflare deployment while keeping the public MCP signature compatible.
Changes:
- Add a read-transaction preflight (
prepare_task_export_overrides/resolve_export_parent) that resolves parent authority once and reuses it across task files, index, shared payload, and Kanban, failing closed viaTaskExportConflicton malformed/conflicting authority. - Thread
explicit_clear_fieldsthroughapply_task_mutation/upsert_field_versions(wired fromupdate_task(parent_id="CLEAR")) to record a distinctexplicit_clearevent; preserve unresolved links, orphan attachments (BRIDGE_PRESERVE_ORPHAN_ATTACHMENTS), and entity visibility. - Add
BRIDGE_PRIVATE_ONLYgating andMACHINE_IDprovenance override in the worker, plus operator docs and four new regression test suites.
| File | Description |
|---|---|
| db_utils.py | Core preservation logic: export overrides, explicit-clear authority, unresolved-link/attachment/visibility preservation, parent materialization in import. |
| bridge_sync_worker.py | Private-only mode gating, MACHINE_ID override, export-override wiring, and export-conflict blocked path. |
| task_server.py | Passes explicit_clear_fields=("parent_id",) when parent_id="CLEAR". |
| docs/ops/BRIDGE_OPERATIONS.md | Documents partial-import preservation, private-only mode, and the regression gate. |
| tests/test_bridge_parent_preservation.py | Regression suite for parent preservation/clear authority and worker behavior. |
| tests/test_bridge_bootstrap_preservation.py | Canonical fresh-peer bootstrap attachment/machine-id preservation. |
| tests/test_bridge_unresolved_links.py | Unresolved transport-link preservation and tombstone precedence. |
| tests/test_entity_public_bridge.py | Entity visibility round-trip and stale-public-vs-local-private precedence. |
| tests/test_bridge_export.py | Adds bootstrap attachment-preservation parametrized test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 141cfae5a5
ℹ️ 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".
| # Imported/seeded NULL→NULL events also exist. Their matching clock | ||
| # proves event identity, not an explicit request to erase this edge. | ||
| raise TaskExportConflict(f"{task_id}: equal-clock NULL event lacks explicit-clear intent") | ||
| return existing["parent_id"] |
There was a problem hiding this comment.
When a parent is hard-deleted through TaskDAO.delete, the schema's ON DELETE SET NULL clears each child's parent_id without advancing its field version. If the child was previously exported, this equal-clock branch returns the old transport parent, so the next export removes the parent task file but writes the child with a dangling reference and can resurrect that relationship if the parent later reappears. The hard-delete path needs to record clear authority for affected children, or preservation must distinguish FK-driven clears from partial-import projections.
Useful? React with 👍 / 👎.
A partial bridge import can retain a parent field clock while the local foreign key is NULL. Export then loses the unresolved relationship, and replaying its causal event can fail the foreign-key check. Preserve the transport relationship and clock, materialize only locally available parents, and record explicit parent CLEAR intent so an older snapshot cannot resurrect the edge.
This also carries the related bootstrap preservation fixes for attachment bytes, unresolved entity links and authoritative shared entity visibility. A public-only projection is not imported as publishable local entities because it cannot transport later revocations. Existing imported entities are not migrated or deleted. An opt-in private-only worker mode suppresses optional peer delivery, public promotion, releases and deployment. Defaults and the public MCP signature remain compatible.
Validation: final PR-head CI and post-merge main CI passed: 1,881 tests passed, 30 skipped; manifest checks, Ruff and registry MCP smoke passed. Locally, 117 relevant regression tests passed, the final eight entity privacy tests passed, and required bridge smoke passed 115 tests. Regressions cover repeated canonical sync with a causal ledger, parent arrival, newer reparenting, explicit clear, attachment preservation and actual source export → peer import → revocation → reexport for shared and public-only entities.
Only source, synthetic fixtures and operator documentation are included. A running MCP process needs a reconnect to load the changed callable. Cross-machine import receipts remain a separate operational check.