feat(oplog-connector): split into message/collections deployments; rename subjects to chat.migration.oplog.*

[general-lex]

Summary

Two independent changes to the migration CDC pump, bundled because both alter the connector's deployment contract:

1. Deployment split — message role vs collections role

The connector previously tailed all 15 collections in one process, and any single watcher's fatal error (e.g. lost resume token on ufsTokens) tore down every watcher — killing message CDC. This PR lets the same binary run as two deployments with disjoint WATCH_COLLECTIONS:

Deployment	Watches	Federation $match
oplog-connector-messages	rocketchat_message only	active
oplog-connector-collections	the other 14 collections	inactive (nothing to filter)

• Config guard relaxed: MESSAGE_COLLECTION must be non-empty but no longer must be watched ("watched ⟹ filtered" holds by construction — the $match applies to whichever watcher's name equals it).
• New watchesMessages() drives a role-truthful startup log: Info "filter active" (message role) vs Warn "filter inactive (collections role)" — Warn so a mis-configured message pod (MESSAGE_COLLECTION typo → unfiltered message watcher → double-deliver) is conspicuous. The reopened typo case + mitigations are documented in the design spec §3.2.
• Combined single-deployment mode still works — the split is opt-in via config (backward compatible).
• Local docker-compose now runs the real two-pod topology (disjoint sets verified: 1 + 14 = original 15).
• Ops invariant (not code-enforced): watch sets disjoint, exactly one pod watches rocketchat_message. Checkpoints are per-collection docs, so disjoint sets cannot collide.

2. Subject rename: chat.oplog.* → chat.migration.oplog.*

Brings the CDC event lane under the chat.migration.* namespace alongside the existing chat.migration.internal.* request lane. Only the two pkg/subject builders change — all four migration services (connector, both transformers, direct-transfer) follow automatically; test literals and living docs updated.

⚠️ Wire-level change / rollout: an already-provisioned MIGRATION_OPLOG_{siteID} stream must have its subjects updated by ops before the renamed services deploy (dev is unaffected — BOOTSTRAP_STREAMS=true uses idempotent CreateOrUpdateStream). The prod verify path checks stream existence by name only, so a stale-subject stream would pass verification and publishes would stall. Known follow-ups from the branch review (not in this PR): verify-subjects fail-fast in bootstrapStreams, filter_active gauge, de-racing the disjoint-set test's negative assertion.

Testing

• TDD throughout: guard relaxation red→green (TestParseConfig_MessageCollectionNotWatchedPasses, TestParseConfig_WatchesMessagesWhenPresent); empty-MESSAGE_COLLECTION rejection retained.
• New integration test TestConnector_CollectionsRole_DisjointSet: a collections-role connector publishes only its own collections' subjects (compiles + lints locally; Docker unavailable in the sandbox — CI runs -tags=integration).
• Full repo unit suite green with -race; lint 0 issues; gosec clean. govulncheck/semgrep were proxy-blocked locally and rely on the CI gate.
• parseConfig 91.7% / watchesMessages 100% unit coverage; package floor is enforced in CI with integration tags.

🤖 Generated with Claude Code

https://claude.ai/code/session_012X9qhQT4NwmCHjdndwNtFD

---

Generated by Claude Code

Summary by CodeRabbit

• New Features

  • Split CDC processing into separate message and collection deployments for clearer isolation.
  • Updated migration event subjects to a new chat.migration.oplog namespace.

• Bug Fixes

  • Allowed message-related configuration to be set without requiring it to be watched in every deployment.
  • Improved startup logging to reflect whether message filtering is actually active.

• Documentation

  • Updated deployment notes, design docs, and traffic estimates to match the new subject naming and split deployment setup.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/dreamy-clarke-prpl3

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/superpowers/plans/2026-07-09-oplog-connector-deployment-split.md`:
- Around line 233-249: The example still uses the old oplog namespace, so update
the consumer filter and assertion in the snippet to the renamed migration
subjects. In the test setup around CreateOrUpdateConsumer, replace the
chat.oplog.sitecr.> filter and the chat.oplog.sitecr.rocketchat_room.insert
check with the new chat.migration.oplog.* subject pattern so the plan matches
the current migration namespace.
- Around line 262-265: The verification step currently uses a raw GOFLAGS-tagged
go vet command, which should be routed through the repo’s make-based workflow
instead. Update the plan to use an existing make target such as make lint, or
add a dedicated Make target for integration-tagged vet and reference that from
the verification instructions. Keep the guidance tied to the affected
verification section in the deployment split plan so the workflow stays
consistent with the project’s make-only convention.

In
`@docs/superpowers/specs/2026-07-09-oplog-connector-deployment-split-design.md`:
- Around line 17-20: The collections-role inventory is incomplete in the
deployment table and should be updated to match the split described elsewhere.
In the spec section containing the deployment table, expand the
`oplog-connector-collections` / `WATCH_COLLECTIONS` summary to include the
missing collections such as `tsmc_room_members` and `tsmc_hr_acct_org`, or
clearly label the table as a partial summary; keep the wording consistent with
the surrounding deployment split description so the full collections-role
inventory is explicit.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 979bc56d-c234-4f17-b9b0-1095d2c2c2d5

📥 Commits

Reviewing files that changed from the base of the PR and between f41e0e6 and a0282bb.

📒 Files selected for processing (22)

• data-migration/CDC_COVERAGE.md
• data-migration/README.md
• data-migration/oplog-connector/bootstrap_test.go
• data-migration/oplog-connector/config.go
• data-migration/oplog-connector/config_test.go
• data-migration/oplog-connector/deploy/docker-compose.yml
• data-migration/oplog-connector/envelope_test.go
• data-migration/oplog-connector/handler_test.go
• data-migration/oplog-connector/integration_test.go
• data-migration/oplog-connector/main.go
• data-migration/oplog-direct-transfer/processone_test.go
• data-migration/oplog-transformer/processone_test.go
• docs/nats-traffic-estimation.md
• docs/superpowers/plans/2026-07-09-oplog-connector-deployment-split.md
• docs/superpowers/specs/2026-06-08-oplog-connector-design.md
• docs/superpowers/specs/2026-06-15-oplog-transformer-design.md
• docs/superpowers/specs/2026-06-16-oplog-transformer-collections-design.md
• docs/superpowers/specs/2026-07-01-oplog-direct-transfer-design.md
• docs/superpowers/specs/2026-07-09-oplog-connector-deployment-split-design.md
• pkg/stream/migration_oplog_test.go
• pkg/subject/oplog_test.go
• pkg/subject/subject.go

[julianshen]

PR #482 Review: feat(oplog-connector): split into message/collections deployments; rename subjects

Author: @general-lex | Stats: +704 / −47 across 23 files

---

tl;dr — ✅ APPROVE

Two clean, backward-compatible changes: (1) lets the oplog-connector binary run as two deployments with disjoint WATCH_COLLECTIONS so a collection-side fault can't stall message CDC, and (2) renames CDC subjects from chat.oplog.* to chat.migration.oplog.*. Well-tested, well-documented, no blocking issues.

---

What changed

1. Deployment split — message role vs collections role

The connector previously tailed all 15 collections in one process, so any single watcher's fatal error killed message CDC. Now the same binary can run as two deployments:

Deployment	Watches	Federation $match
oplog-connector-messages	rocketchat_message only	active
oplog-connector-collections	the other 14 collections	inactive

Code changes:

• config.go — Drops the hard-fail slices.Contains(WATCH_COLLECTIONS, MESSAGE_COLLECTION) guard. Retains the non-empty MESSAGE_COLLECTION check. Adds watchesMessages() helper.
• main.go — Conditional startup log: slog.Info for message role, slog.Warn for collections role (Warn ensures a misconfigured message pod is conspicuous).
• integration_test.go — New TestConnector_CollectionsRole_DisjointSet: starts a collections-role connector, inserts to both a watched and an unwatched collection, asserts only the watched collection's subjects appear on the stream.
• deploy/docker-compose.yml — Single oplog-connector service split into oplog-connector-messages + oplog-connector-collections with disjoint watch sets.

Safety analysis: The "watched ⟹ filtered" invariant holds by construction — openMongoChangeSource applies the federation $match when coll == cfg.MessageCollection, so a deployment that doesn't watch the message collection has no watcher to filter. The residual risk (MESSAGE_COLLECTION typo reopening double-deliver) is mitigated by: correct default value, Warn-level startup log, and ops verification. Explicitly analyzed in design spec §3.2.

2. Subject rename: chat.oplog.* → chat.migration.oplog.*

Only the two builder functions in pkg/subject/subject.go change. All four migration services follow automatically. Test literals, docs, and traffic estimates updated consistently.

⚠️ Rollout note: pre-provisioned MIGRATION_OPLOG_{siteID} streams need subject updates before deploying the renamed services (dev is unaffected — BOOTSTRAP_STREAMS=true uses idempotent CreateOrUpdateStream).

---

Review by dimension

✅ Correctness

• Subject rename is mechanical and complete — all 23 files consistently updated.
• Guard relaxation is sound: the $match is wired per-watcher by identity, not by config validation.
• watchesMessages() is a trivial, correct predicate. 100% tested.
• Startup log correctly reflects deployment role.

✅ Security

• No secrets, tokens, or credentials exposed.
• No new attack surface.
• Accepted residual risk (MESSAGE_COLLECTION typo) is documented, mitigated, and gated behind an explicit env-var override of a correct default.

✅ Code Quality

• Idiomatic Go. Clear comments explaining rationale.
• TDD approach: tests rewritten before implementation.
• handler.go improvement: op field added to marshal-error log for better debuggability.
• docker-compose split is clean with good comments.

✅ Test Coverage

• parseConfig: 91.7% unit coverage. Both branches (watches/doesn't watch) tested.
• watchesMessages(): 100% coverage.
• Integration: new disjoint-set test validates both positive and negative assertions.
• All downstream fake subjects and test literals updated.

✅ Performance

• No hot-path changes. watchesMessages() called once at startup.
• Blast-radius reduction may improve reliability, not a code-level perf change.

✅ Documentation

• New design spec (81 lines) covering problem, goal, safety analysis, rollout.
• Parent spec updated with §7.5 deployment topology.
• All downstream design specs, CDC_COVERAGE, and traffic estimates updated.
• Minor observation: implementation plan file (448 lines) has unchecked task checkboxes.

---

Observations (non-blocking)

1. Implementation plan checkboxes unchecked — docs/superpowers/plans/2026-07-09-oplog-connector-deployment-split.md has - [ ] for all tasks despite representing completed work. Consider checking them off or removing the file.

2. No code-enforced disjoint-set invariant — The cross-deployment invariant (disjoint watch sets, exactly one watches messages) is ops/IaC responsibility. No runtime detection possible across separate processes. Well-documented.

3. Known follow-ups (from PR description, not in this PR): verify-subjects fail-fast in bootstrapStreams, filter_active Prometheus gauge, de-racing the disjoint-set test's negative assertion.

---

Verdict: ✅ APPROVE

The changes are correct, well-tested, backward-compatible, and thoroughly documented. The accepted residual risk is explicitly analyzed and mitigated. No blocking issues.

[julianshen]

Verdict: APPROVE — Well-designed deployment split and subject rename, with thorough tests and excellent documentation. Minor non-blocking comments below.

Summary: Splits the oplog-connector into message/collections deployments with disjoint WATCH_COLLECTIONS so collection-side faults can't stall message CDC, and renames CDC subjects from chat.oplog.* to chat.migration.oplog.* for namespace consistency.

Findings

• data-migration/oplog-connector/config.go:87-93 — Guard relaxation is correct: "watched ⇒ filtered" holds by construction since the federation $match is wired by collection identity. The reopened typo risk (MESSAGE_COLLECTION misconfigured → unfiltered double-deliver) has reasonable mitigations (default value, Warn-level log). Consider adding a filter_active gauge metric for production observability.

• data-migration/oplog-connector/main.go:78-85 — Conditional startup logging with Warn level for collections role is a good operational practice. The Warn makes misconfiguration conspicuous.

• data-migration/oplog-connector/handler.go:186-189 — Adding op field to marshal-error log is a practical debugging improvement for dropped events.

• data-migration/oplog-connector/integration_test.go:308-370 (TestConnector_CollectionsRole_DisjointSet) — Good test covering the disjoint-set invariant. Minor concern: assert.NoError inside Eventually won't abort on Ack failure (use require for fatal conditions). The negative assertion against message subjects only covers events received before the room insert is observed (known timing limitation, acknowledged in design spec).

• pkg/subject/subject.go:1194-1202 — Subject rename is mechanical and exhaustive. All callers use the library functions, so downstream services follow automatically. This IS a wire-level breaking change — ops must update stream subjects before deploying. Dev environments self-heal via BOOTSTRAP_STREAMS=true (CreateOrUpdateStream is idempotent).

• data-migration/oplog-connector/deploy/docker-compose.yml:30-66 — Split into oplog-connector-messages + oplog-connector-collections with disjoint watch sets (verified: 1 + 14 = original 15). Same image, different env. Both use default :9090 metrics port inside their own containers — correct since no host ports are published.

• docs/superpowers/specs/2026-07-09-oplog-connector-deployment-split-design.md (NEW) — Excellent design spec. Section 3.2's safety analysis explicitly enumerates reopened failure cases and mitigations — this level of honesty about tradeoffs is commendable.

• Cross-cutting: No code-level disjoint-set enforcement — The invariant "watch sets disjoint, exactly one pod watches rocketchat_message" is ops/IaC-owned, not code-enforced. If both deployments accidentally watch the same collection, double-publishing occurs. Acknowledged in design; startup logs make roles visible. Reasonable for an internal infra component.

• docs/superpowers/plans/2026-07-09-oplog-connector-deployment-split.md (NEW, 448 lines) — Implementation plan contains Claude Code task-tracking artifacts (sandbox paths, "Co-Authored-By" commit messages). Harmless but slightly messy for a merged PR.

Non-blocking follow-ups

1. Add filter_active Prometheus gauge for production observability of the reopened typo case.
2. Add fail-fast subject verification in bootstrapStreams for prod (stream exists but subjects are stale → silent stall).
3. Consider de-racing the negative assertion in TestConnector_CollectionsRole_DisjointSet.

[julianshen]

Code Review: PR #482 — APPROVE ✅

Summary

Two clean, backward-compatible changes to the oplog-connector deployment contract:

1. Deployment split — Relax the config guard so the same binary can run as two deployments with disjoint WATCH_COLLECTIONS (message role vs. collections role), preventing a fault on low-value collections from killing message CDC. New watchesMessages() drives truthful startup logging (Info vs. Warn).
2. Subject rename — chat.oplog.* → chat.migration.oplog.* across pkg/subject/subject.go builders; all test literals and docs updated consistently.

What's Good

• Motivation is sound. A single fatal watcher error today tears down all watchers, so message CDC should not share a failure domain with operational collections.
• Implementation is minimal. One guard removed, one method added, one log branch, docker-compose split. No new binary. Single-deployment mode still works.
• Config guard relaxation is correctly analyzed. The design doc honestly documents the one residual risk (MESSAGE_COLLECTION typo → unfiltered message watcher → double-deliver) and its mitigations: correct default, Warn-level log for the collections role.
• Test coverage is thorough. Two new unit tests for both roles, one new integration test (TestConnector_CollectionsRole_DisjointSet) verifying disjoint-set behavior. All 15+ subject literals updated. watchesMessages() at 100% coverage.
• Documentation is excellent. New 81-line design spec, 448-line implementation plan, parent spec updated (§7.5), CDC_COVERAGE.md noted, traffic estimation doc updated.
• Go-idiomatic. Proper defer, error handling, slog level selection (Info vs. Warn), no concurrency issues.

Minor Suggestions (non-blocking)

1. READ_PREFERENCE=primaryPreferred is set for the new messages service in docker-compose, whereas the original single deployment used the default secondary. This behavioral change should be explicitly noted in the PR description or commit message.

2. Handler.go error-log improvement (adding "op", ev.Op to the marshal-failure log) is unrelated to the deployment split or subject rename. Small enough to be harmless, but ideally would be a separate commit.

3. Follow-up: filter_active Prometheus gauge. The design doc mentions this as a known follow-up. A gauge based on watchesMessages() would give ops real-time visibility into each pod's role. Worth tracking as a separate issue.

4. Wire-level rollout note. The subject rename requires updating already-provisioned stream subjects before deploying renamed services in prod. The PR description acknowledges this and flags a separate follow-up for verify-subjects fail-fast. Ensure this is tracked.

Verdict

APPROVE — no blocking issues. The changes are correct, well-tested, well-documented, and address a real operational isolation concern. The acknowledged residual risk is adequately mitigated.

[julianshen]

Code Review: PR #482 — APPROVE

Great work on this PR. The deployment split and subject rename are well-designed, well-tested, and thoroughly documented.

Summary

Two bundled changes:

1. Deployment split — Connector can run as two pods (messages vs collections) with disjoint WATCH_COLLECTIONS, isolating message CDC from collection-side faults. Config guard relaxed from hard-fail to Warn-level log + non-empty check. Backward compatible.
2. Subject rename — chat.oplog.* → chat.migration.oplog.*, centralized in pkg/subject, consistent across all 23 files.

What's Good

• Minimal code change: ~3 lines removed from parseConfig(), one helper added. The federation $match was already wired per-watcher by identity — removing the guard is safe.
• Strong test coverage: New config tests for both roles (watched/not-watched), new integration test verifying disjoint-set invariant, all existing tests updated for the rename.
• Honest safety analysis: The design spec §3.2 explicitly documents the reopened "typo'd MESSAGE_COLLECTION" risk and its layered mitigations (correct default, Warn log, ops verification). This is a responsible trade-off.
• Docs are excellent: New design spec, updated parent spec, CDC_COVERAGE deployment note, all downstream spec docs updated — nothing stale.
• Docker compose split: Disjoint sets verified (1 + 14 = 15 original), both containers use default :9090 internally.

Minor Suggestions (non-blocking)

1. Warn log wording (main.go:83): "no message collection watched — federation-origin filter inactive (collections role)" says "(collections role)" even when a misconfigured messages pod hits this path. Consider dropping the parenthetical or rephrasing to avoid misleading operators.

2. Bootstrap verify-path (bootstrap.go:37): The disabled (prod) path checks stream existence by name only, not subjects. Post-rename, a stale-subject stream passes silently. This is acknowledged in the PR body as a known follow-up — worth tracking.

Verdict: APPROVE

No blocking issues. The code is correct, the tests pass, the docs are complete, and the residual risk from the relaxed guard is well-understood and mitigated.

[julianshen]

APPROVE

Summary

This PR makes two clean, well-scoped changes to the oplog-connector:

1. Deployment split — the same binary can now run as two deployments with disjoint WATCH_COLLECTIONS (message role vs collections role), isolating high-volume message CDC from collection-side faults.
2. Subject rename — chat.oplog.* → chat.migration.oplog.* to group CDC events under the chat.migration.* namespace.

What's Good

• Correctness: The federation $match is wired per-watcher by identity (coll == cfg.MessageCollection), so the invariant "watched ⟹ filtered" holds by construction. The guard relaxation is safe — it only removes a check that blocked a legitimate configuration, and the reopened risk (MESSAGE_COLLECTION typo) has layered mitigations: default env value, Warn-level startup log, and an ops/IaC invariant.
• Code quality: Minimal, focused diff. The deployment split is config-only — no code branching. watchesMessages() is a clean, testable helper. Comments explain the why, not just the what. Startup logging is now truthful per-role instead of always claiming "filter active".
• Testing: TDD approach throughout — TestParseConfig_MessageCollectionNotWatchedFails → TestParseConfig_MessageCollectionNotWatchedPasses + new TestParseConfig_WatchesMessagesWhenPresent. New integration test TestConnector_CollectionsRole_DisjointSet validates the disjoint-set invariant end-to-end. All existing tests updated for the subject rename. Coverage is maintained (91.7% on parseConfig, 100% on watchesMessages).
• Documentation: New design spec (81 lines) with full safety analysis. Parent spec updated (§7.5). CDC_COVERAGE.md, README, traffic estimation, and 4 downstream specs updated. docker-compose.yml comments explain split rationale and invariants.
• Backward compatibility: Combined single-deployment mode still works — the split is opt-in via config.

Minor Notes (non-blocking)

• The new integration test wasn't run locally (Docker unavailable in sandbox) — relies on CI. Follows existing patterns closely, so risk is low.
• Known follow-ups are listed in the PR body but not tracked as GitHub issues: verify-subjects fail-fast in bootstrapStreams, filter_active gauge, de-racing the disjoint-set test's negative assertion. Consider filing issues for these.
• The plan file (docs/superpowers/plans/2026-07-09-oplog-connector-deployment-split.md, 448 lines) is verbose — much of it is Claude Code task breakdown. Consider trimming post-merge.
• No unit test for the Warn log path in main.go — typical for startup code, covered by integration.

Verdict

APPROVE — well-designed, well-tested, well-documented. No blocking issues.

[julianshen]

Hermes Agent Code Review

This PR has been reviewed by Hermes Agent. See the review above for detailed findings.

Automated review bot — do not reply to this message.