Persist external root bridge state

[salmonumbrella]

What changed

• persist external-root bindings, field mappings, field reconciliation baselines/conflicts, retry claims, pending outbound comments, and connector identity pins
• preserve external-root state through export/import, replay, project move/merge, lifecycle, restore, and federation projection rules
• enforce the same storage contract through SQLite and PostgreSQL conformance coverage

Persisted schema

Both backends add external_root_bindings, external_field_mappings, and external_field_states, with foreign keys, active-binding uniqueness, due-work indexes, typed-field constraints, conflict snapshots, and retry/claim state. PostgreSQL adds migration 000026_external_root_bridges and advances the schema version to 26; SQLite updates its canonical bootstrap schema to the same logical shape.

Scope and dependency

This is PR 2 of the delivery sequence tracked in #286 and depends on #287. The branch is cumulative so it builds before #287 merges; after #287 lands, the review diff narrows to storage and migration changes only.

Reconciliation behavior and user-facing daemon/API/CLI surfaces follow in subsequent PRs.

Part of #286.

[roborev-ci]

roborev: Combined Review (a357e7e)

Changes need revision: three medium-severity correctness issues could break comment synchronization, starve due bindings, or restore unusable mappings.

Medium

• Missing opaque comment revision — pkg/connector/protocol.go:132
Comment lacks the monotonically advancing revision required by the connector contract and external comment projection’s ExternalRevision. Bridges cannot reliably detect stale observations or populate the projection without deriving revisions from timestamps.
  Fix: Add a required Revision field and enforce it in connector conformance cases for listed, published, and deleted comments.

• Fresh claims can starve eligible bindings — internal/db/sqlitestore/external_roots.go:293, internal/db/pgstore/external_roots.go:391
ListDueExternalRootBindings applies LIMIT before excluding bindings with fresh claims. Those rows can repeatedly fill the batch and prevent later unclaimed bindings from being processed.
  Fix: Accept a stale-claim cutoff and filter for unclaimed or stale-claimed bindings in the due query, matching the existing issue-sync scheduling pattern.

• Replay accepts inconsistent root mappings — internal/db/sqlitestore/import_replay.go:720, internal/db/pgstore/import_replay.go:330
  Replay does not verify that the exported mapping source equals connector:<connector_instance> or that its external ID equals external_root_key. Malformed JSONL can therefore restore a binding unusable by later projection operations.
  Fix: Validate both relationships before resolving or inserting the binding, with equivalent rejection coverage for SQLite and PostgreSQL replay.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 31m42s

[roborev-ci]

roborev: Combined Review (f8200d0)

Changes need revision: two medium-severity integration gaps leave hook subscriptions incomplete and documented connector configuration unusable.

Medium

• internal/hooks/config_validate.go:15 — Bridge events are missing from hook validation. The new issue.external_* event types are absent from knownEventTypes, causing exact subscriptions to be rejected while issue.* and * hooks silently ignore bridge mutations. Add every new bridge issue event to the allowlist and test exact and wildcard matching.

• internal/config/daemon_config.go:23 — Documented connector configuration cannot be loaded or administered. ConnectorConfig is not included in DaemonConfig, so strict TOML decoding rejects [connector.env]. There is also no daemon API or CLI lifecycle path to reconfirm restored bindings, potentially leaving them permanently paused. Wire the connector configuration and administration paths into the daemon and CLI/API, or clearly document these interfaces as unavailable until implemented.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 28m32s

[roborev-ci]

roborev: Combined Review (cddac0d)

The PR has two medium-severity correctness issues involving federation transitions and PostgreSQL mapping identity collisions.

Medium

• Read-only federation transitions can strand active external-root bindings
  Location: internal/db/sqlitestore/federation.go:484, internal/db/pgstore/federation_control.go:118
  A project with an active external-root binding can become a read-only federation spoke. Subsequent hub content changes then fail materialization because the bridge owns the title/body, while bridge projections and Pause/Unbind operations are rejected by the spoke write gate. Reconciliation remains blocked until federation is disabled or left.
  Suggested fix: Reject read-only spoke transitions when active external-root bindings exist, or atomically unbind them before enabling federation.

• Millisecond timestamps can collide as PostgreSQL field-mapping identities
  Location: internal/db/pgstore/external_roots.go:1817
  PostgreSQL creates field-mapping identities with nowStoredTimestamp(), which truncates timestamps to milliseconds, while CreatedAt distinguishes mapping revisions during replay. Two revisions created within the same millisecond can receive the same portable identity, causing field state to attach to the wrong mapping or restoration to fail with a duplicate key.
  Suggested fix: Give mapping revisions durable unique UIDs and reference them from exported field state, or otherwise use a collision-free portable identity.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 38m27s

[roborev-ci]

roborev: Combined Review (8122614)

Code review found two medium-severity correctness issues; no exploitable security weakness was identified.

Medium

• External comment timestamps lose precision on PostgreSQL — internal/db/pgstore/external_roots.go:1023, 1159
  PostgreSQL uses formatStoredTime for externally supplied comment creation timestamps, truncating them to milliseconds, while SQLite preserves full precision. This loses provider data and can cause backend-dependent ordering for comments within the same millisecond. Use formatExternalObservationTime in both insertion paths and add cross-backend tests with sub-millisecond timestamps.

• Connector responses accept malformed canonical field values — internal/connector/client.go:377
validFieldValues checks only the field identifier and kind, allowing invalid dates, malformed instants, and local datetimes without valid timezones. Validate and parse each kind’s complete canonical representation, enforce timezone requirements, and add client tests for malformed responses.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 34m5s

[roborev-ci]

roborev: Combined Review (4be77fd)

High-severity federation reset data loss and three medium-severity ownership/namespace integrity issues must be fixed before merge.

High

• Federation reset destroys external-root binding state
  internal/db/sqlitestore/federation_push.go:322, internal/db/pgstore/federation_reset.go:80
  Reset deletes every project issue, cascading through external_root_bindings.issue_id and removing active bindings, field state, and inactive root-history records. Bridge audit events do not preserve this state because folding ignores them. Preserve and reattach bridge records using durable issue UIDs while fencing claims, or block resets whenever external-root binding history exists.

Medium

• Federation reconciliation bypasses external-comment ownership
  internal/db/sqlitestore/federation.go:1805, internal/db/pgstore/federation_projection.go:584
  Reconciliation updates and deletes comments without enforcing external ownership. A remote edit can overwrite a provider-owned comment; deletion can remove its mapping while the retained revision marker prevents re-import. Enforce ownership before federation-driven updates or deletions and reject conflicts atomically.

• Lifecycle and provider comment IDs share a namespace
  internal/db/sqlitestore/external_roots.go:995, internal/db/pgstore/external_roots.go:1120
  Provider-controlled IDs such as lifecycle:complete:revision-1 can collide with synthetic lifecycle mappings, suppress lifecycle state and needs-review, or overwrite lifecycle comments. Put lifecycle identities in a mapping source or namespace disjoint from provider IDs.

• Root-mapping validation permits additional target fields
  internal/db/sqlitestore/import_mappings.go:33, internal/db/pgstore/import_mappings.go:33
  Protection verifies only that issue_id is unchanged, allowing the same mapping to gain a comment_id, link_id, or label. Deleting that extra target can cascade-delete the root mapping and binding. Require root mappings to contain exactly the bound issue_id and no other target fields, including during JSONL replay.

---

Reviewers: 2 done | Synthesis: codex, 13s | Total: 46m54s

[roborev-ci]

roborev: Combined Review (678b054)

One medium-severity concurrency issue must be fixed before merge.

Medium

• internal/db/pgstore/external_roots.go:2114 — externalFieldMutationContext validates the binding claim and active field mapping using unlocked reads. Concurrent mapping reconfiguration can invalidate and fence a stale claim, but PostgreSQL may serialize the mutation’s earlier reads before that reconfiguration, allowing stale-worker state changes and event emission to commit afterward.

  Suggested fix: Lock the binding row with FOR UPDATE before claim validation, consistent with externalRootProjectionContext, and serialize mapping validation with configuration changes. Add a PostgreSQL concurrency test that races field mutation against mapping rotation and verifies the stale mutation is rejected without changing state or emitting an event.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 25m53s

[roborev-ci]

roborev: Combined Review (1272795)

Medium-severity account identity bug found; no High or Critical findings.

Medium

• Account identity is omitted from binding history and uniqueness
  • Location: internal/db/sqlitestore/external_roots.go:259, internal/db/pgstore/external_roots.go:383
  • Impact: Reconfiguring a connector instance for a different account can reject a legitimate same-key root or silently reuse the previous account’s issue and root mapping.
  • Fix: Enforce a stable account identity per connector instance, or consistently include external_account_key in root-history lookups, mapping identity, and uniqueness constraints.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 30m37s

[roborev-ci]

roborev: Combined Review (034002e)

Medium-severity issues remain: a PostgreSQL upsert race and inconsistent SQLite timestamp encoding.

Medium

• PostgreSQL active-mapping upsert race — internal/db/pgstore/external_roots.go:1863
  The active field mapping is read before acquiring the connector advisory lock. Concurrent upserts can retain stale hasExisting state; after waiting for the lock, the second transaction may attempt another active insert and receive an unretried 23505 instead of behaving idempotently.
  Fix: Acquire the lock before reading the mapping snapshot, or retry and re-read after an active-mapping unique-index conflict. Add a concurrent PostgreSQL upsert test.

• SQLite comments can be ordered incorrectly — internal/db/sqlitestore/external_roots.go:913, internal/db/sqlitestore/external_roots.go:1054
  External comment timestamps are bound as time.Time, while local comments use fixed-width sqliteTimeFormat strings. Because SQLite orders created_at lexically, mixed encodings can place same-day external and local comments out of chronological order.
  Fix: Store all comment timestamps in one fixed-width canonical UTC representation, including external projections and lifecycle comments. Add mixed local/external ordering coverage with sub-millisecond timestamps.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 31m32s

[roborev-ci]

roborev: Combined Review (2d004e3)

One medium-severity correctness issue found; no exploitable security vulnerabilities identified.

Medium

• Incorrect ordering for mixed-precision comment timestamps — internal/db/pgstore/external_roots.go:1047, internal/db/pgstore/comments.go:271, internal/db/pgstore/ui.go:681

  External comments use nanosecond timestamps while local comments use milliseconds, but PostgreSQL lexically orders these TEXT values. Within the same millisecond, a later timestamp such as .123100000Z sorts before .123Z, causing incorrect comment order in storage and UI reads.

  Suggested fix: Order by parsed timestamps or normalize both formats in the query, consistent with the SQLite implementation. Add behavioral coverage mixing millisecond- and nanosecond-precision comments.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 22m0s

[roborev-ci]

roborev: Combined Review (bd7916b)

Changes need a targeted validation fix before approval.

Medium

• internal/connector/client.go:282 — Runtime validation accepts duplicate or out-of-order comments and duplicate field descriptors, despite the connector protocol requiring unique comment and field IDs and comments ordered by created_at, then id. Malformed connector output can therefore create ambiguous or unstable projections. Update validateDecodedResult to reject duplicate IDs and incorrectly ordered comments, with tests covering duplicate and out-of-order responses.

---

Reviewers: 2 done | Synthesis: codex, 6s | Total: 31m11s

[roborev-ci]

roborev: Combined Review (f962d12)

Code review found two medium-severity cross-backend consistency issues; no high or critical findings.

Medium

• Timestamp precision mismatch — internal/db/sqlitestore/queries.go:1236-1267, internal/db/sqlitestore/federation.go:1499-1500
  SQLite stores local comment timestamps with nanosecond precision, but event payloads and federation snapshots truncate them to milliseconds. Comments created within the same millisecond may be reordered after federation materialization. Preserve the full createdAt value and add a federation round-trip test using sub-millisecond timestamps.

• Embedded NUL handling differs by backend — internal/connector/client.go:326-345,449-450, internal/db/external_root_types.go:376-400
  Connector validation permits NUL characters in canonical identities and root/comment bodies. SQLite accepts these values, while PostgreSQL text columns reject them, potentially causing repeated reconciliation failures only on PostgreSQL. Reject NUL characters at both protocol-client and storage validation boundaries, with backend-neutral behavioral coverage.

---

Reviewers: 2 done | Synthesis: codex, 16s | Total: 28m2s

[roborev-ci]

roborev: Combined Review (11faae2)

Federated soft deletion can bypass active-binding safeguards; one medium-severity issue remains.

Medium

• internal/db/sqlitestore/federation.go:1659; internal/db/pgstore/federation_projection.go:456 — Federation reconciliation checks only title/body changes before updating the entire issue row. A remote issue.soft_deleted event with unchanged content can set deleted_at on an actively bound issue, bypassing the requirement to unbind first and leaving an active bridge targeting a hidden issue.

  Fix: In both backends, load the existing deletion state and reject live-to-deleted federation transitions when an active external-root binding exists. Add cross-backend coverage for federated soft deletion of a bound issue.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 19m28s

[roborev-ci]

roborev: Combined Review (5c5cc94)

Federated connector comments will be quarantined on push-enabled spokes due to inconsistent actor attribution.

Medium

• internal/db/sqlitestore/external_roots.go:914, internal/db/pgstore/external_roots.go:1045 — Projected comments store the connector actor as the author and include it in the issue.commented payload, while event insertion rewrites only the event actor to the spoke’s bound actor. Because hub ingestion requires the payload author to match the bound actor, these comments—including lifecycle comments—are quarantined instead of synchronized.
  • Fix: Resolve the project’s effective local mutation actor before creating external comments and use it consistently for the stored author, payload, and event on both backends. Preserve provider attribution in source metadata and add a push-enabled spoke-to-hub projection test.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 47m44s

[roborev-ci]

roborev: Combined Review (e5952ca)

Code review found one medium-severity federation compatibility issue.

Medium

• Federation rejects external-root events — internal/db/sqlitestore/external_roots.go:748, internal/db/pgstore/external_roots.go:884

  External root/field projections and bridge audit mutations attribute federation-eligible events directly to the connector or caller. On push-enabled spokes, hub ingestion requires the event actor to match the configured bound actor, so these events may be rejected, quarantining or stalling federation push.

  Suggested fix: Resolve effectiveLocalMutationActorTx in every event-producing external-root transaction and use it for event attribution, while retaining connector identity in source metadata. Add cross-backend tests that push root, field, and audit events through federation ingestion.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 29m20s

[roborev-ci]

roborev: Combined Review (f84c784)

Potential ownership conflict remains in import replay.

Medium

• External-root replay bypasses issue-sync ownership guard — internal/db/sqlitestore/import_replay.go:760, internal/db/pgstore/import_replay.go:361

  Active external bindings can be replayed onto issue-sync-managed content because replay omits the guard used during live creation. This can produce competing enabled owners that normal APIs reject.

  Fix: Call rejectIssueSyncManagedExternalRootTx before inserting an active replayed binding in both backends. Add a backend-conformance replay test that verifies atomic rollback with ErrExternalRootIssueSyncConflict when the issue-sync source and import mapping match.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 43m51s

[roborev-ci]

roborev: Combined Review (e8d2905)

One medium-severity protocol validation issue found; no exploitable security regressions identified.

Medium

• internal/connector/client.go:104 — ReadRoot and CompleteRoot validate returned roots structurally but do not verify that the returned key matches the requested RootKey. A connector could return root B for root A, causing B’s data to be projected onto A’s bound issue, or have a mismatched completion accepted as successful. Compare returned and requested keys in both methods, return ErrProtocolFailure on mismatch, and add behavioral tests for mismatched responses.

---

Reviewers: 2 done | Synthesis: codex, 13s | Total: 33m13s

[roborev-ci]

roborev: Combined Review (ff5c726)

Two medium-severity issues require changes before approval.

Medium

• Contract-invalid mutation readbacks are accepted — internal/connector/client.go:120
CompleteRoot accepts state: "open", PublishComment accepts a body different from the submitted body, and field operations accept results missing requested keys. This can record a mutation or baseline as successful even when the connector did not perform it. Enforce method-specific postconditions after decoding and add behavioral tests for invalid readbacks.

• External field mappings can collapse distinct planning values — internal/db/sqlitestore/schema.sql:715, internal/db/pgstore/schema.sql:786
  Active uniqueness only covers (connector_instance, kata_field), allowing the same external field ID to map simultaneously to scheduled and deadline fields. Because connector field operations are keyed by external field ID, these values can collapse into one ambiguous write. Enforce active uniqueness on (connector_instance, external_field_id) in both backends and reject conflicting live or imported mappings.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 37m15s

[roborev-ci]

roborev: Combined Review (3bab7c2)

Two medium-severity issues remain; no high or critical findings were identified.

Medium

• internal/db/sqlitestore/external_roots.go:236 — SQLite comments preserve nanosecond timestamps, but the initial publish frontier advances by a full millisecond. A comment created shortly after binding within that millisecond can fall before the frontier and never be published.

  • Fix: Advance by SQLite’s minimum timestamp increment (one nanosecond), or use an exclusive cursor with a stable ID tie-breaker. Add coverage for a post-binding comment created within the same millisecond.

• internal/jsonl/export.go:1118 — Version-aware project exports include field mappings only when referenced by existing field state. An active mapping for a project-bound connector that has not synchronized yet is omitted, causing its configuration to be lost on restore.

  • Fix: Include active mappings whose connector instance has a project binding, matching the current-schema exporters. Add a project-export round-trip test with no field state.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 25m25s

[wesm]

looking now

[salmonumbrella]

i paused on one schema suggestion instead of blindly adding it. I want to avoid bloat.

the two Kata fields are scheduled_on and deadline_on. they're different:

• scheduled_on is a start date and keeps the issue out of ready/next until then
• deadline_on is the due date and never gates readiness

for Notion, that would normally be two separate properties, e.g. Start and Due. the edge case is that today both Kata fields can be mapped to the same Notion property, so reads/writes collapse into one value.

the suggested DB constraint would only prevent that within one connector instance: (connector_instance, external_field_id) stays unique while active. i haven't added it yet because i want to confirm the product call first: keep both mappings and require distinct external properties, or simplify to one date mapping? wdyt

[roborev-ci]

roborev: Combined Review (79651b3)

Medium-severity validation gaps remain in CAS requests and external-root checkpoints.

Medium

• Locations: pkg/connector/serve.go:168, internal/connector/client.go:168
WriteFields does not require Fields and Expected to have identical keysets, and the process client does not validate Expected. Malformed requests can omit compare-and-set preconditions for written fields, undermining the concurrency safeguard. Add shared validation for identical keysets and valid values before handler dispatch or process invocation, plus behavioral tests proving mismatched requests are rejected without invoking the connector.

• Locations: internal/db/sqlitestore/external_roots.go:632, internal/db/pgstore/external_roots.go:769
RecordExternalRootSuccess accepts empty or invalid external state/revision values, overwrites the durable checkpoint, and clears the claim as though reconciliation succeeded. The error path also accepts arbitrary state values when given a state/revision pair. Require a nonempty revision and canonical open or complete state for successful checkpoints, apply the same enum validation to error checkpoints, and test malformed updates while a valid claim is active.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 38m40s

[roborev-ci]

roborev: Combined Review (9d60648)

Compatibility verdict: one medium-severity protocol-breaking issue should be resolved before merge.

Medium

• pkg/connector/protocol.go:247 — WriteFieldsParams.expected is now mandatory, but ProtocolVersion remains kata.connector.v1. Existing v1 servers reject the unknown field, while new servers reject requests that omit it, making v1 implementations mutually incompatible. Introduce and negotiate a new protocol version or CAS-write capability, or preserve the original v1 request shape and behavior. Update the protocol documentation accordingly.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 13m55s

[roborev-ci]

roborev: Combined Review (0a09925)

Code is not ready to merge: one medium-severity data-loss issue remains in scoped legacy exports.

Medium

• internal/jsonl/export.go:1118 — The version-aware project exporter only includes mappings referenced by external_field_states. An active mapping for a connector bound to the selected project may not yet have a state row, causing scoped legacy backup/restore to retain the binding but silently lose its mapping configuration. Match the current SQLite/PostgreSQL exporters by including active mappings for connector instances with selected-project bindings, as well as historical mappings referenced by selected-project field state. Add a scoped legacy-export test for an active mapping with a binding but no state.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 32m12s