fix(watchers,feeds): guard id-less source queries and cross-org entity_ids#1146
Conversation
…y_ids Two silent correctness bugs found while building an HN engagement watcher in the prod lobu-crm org. BUG A — a watcher source query that omits `id` (e.g. `SELECT origin_id, payload_text FROM events`) silently skips the reaction. Watcher-mode aggregation keys every row by `row.id` and the signed window_token only carries those ids, so an id-less source produces content_count=0 → complete_window reports `content_linked: 0` and skips the reaction even though the agent received the rows. No error surfaced anywhere. Fix: validate at watcher create/create_version time that each source query projects an `id` column (queryProjectsIdColumn, AST-based, fails open on parse errors) and reject with a clear message otherwise. BUG B — feeds/watchers accepted entity_ids belonging to another org, so synced events linked to a non-existent in-org entity. Fix: assertEntityIdsInOrg validates every entity_id is in the requesting org on create_feed/update_feed and watcher create_from_version; the create_from_version name lookup is now org-scoped too. Tests: unit coverage for the id-projection predicate plus red→green integration reproducers for both bugs (embedded Postgres).
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR enforces two correctness constraints for feeds and watchers: watcher SQL sources must explicitly project an ChangesCross-org validation and watcher source hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Review gate: |
Fixes two watcher/feed correctness bugs found while building the HN engagement watcher in the
lobu-crmorg.Bug A — id-less source query silently skips the reaction
A watcher whose source SQL omits
id(e.g.SELECT origin_id, payload_text FROM events) produces an emptycontent_idsin the signed window token, socomplete_windowreportscontent_linked: 0and silently skips the reaction — no error, even though the agent received the rows and extracted a result. This cost hours to diagnose in prod.Fix: new
queryProjectsIdColumn()(AST inspection, fails open on parse errors) wired intovalidateWatcherConfig, socreate/create_versionreject an id-less source at save time with a clear message.SELECT *(the default source) still passes.Bug B — feeds/watchers accept cross-org entity_ids
lobu-crmfeeds were created withentity_ids: {54}, but entity 54 belongs to a different org — so synced events never link to a valid in-org entity. The create/update path never validated entity ownership.Fix: new
assertEntityIdsInOrg()(org-scoped existence check, binds viapgBigintArrayto dodge the prodfetch_types:falsearray gotcha) wired intomanage_feedscreate/update andmanage_watcherscreate_from_version.Tests
Red→green reproducers for both bugs (unit + integration), plus regression coverage. All deterministic gates green: typecheck/unit/integration = 0.
Prod remediation (operator, post-deploy)
Repoint lobu-crm's cross-org feeds:
Follow-up (out of scope)
handleCreate's scalarentity_idpath adopts the looked-up entity's org rather than validating against request context — same class, deferred.Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests