Skip to content

fix: schema.sql drift + manage_feeds feed_key narrowing + submodule bump#804

Merged
buremba merged 4 commits into
mainfrom
fix/schema-drift-and-feed-key-narrow
May 17, 2026
Merged

fix: schema.sql drift + manage_feeds feed_key narrowing + submodule bump#804
buremba merged 4 commits into
mainfrom
fix/schema-drift-and-feed-key-narrow

Conversation

@buremba
Copy link
Copy Markdown
Member

@buremba buremba commented May 17, 2026

Summary

Three follow-ups dropped from PR #786 during the squash-merge:

  • db/schema.sql — adds the missing `COMMENT ON COLUMN personal_access_tokens.worker_id` and relocates `idx_personal_access_tokens_worker_id` to its alphabetical spot, so the snapshot matches `dbmate up` output. The migrations CI job has been red on this drift since `20260517030000_pat_worker_id_binding` landed.
  • manage_feeds.list_feeds — narrows the event-counts scan by `feed_key` as well as `connection_id` (ANY-array on both axes). The final LEFT JOIN drops any over-count from the cross product. EXPLAIN ANALYZE on prod: 50-feed list on a busy connection drops from ~1.5s to ~670ms.
  • packages/web → ca12cd2 — owletto/main moved during the PR Technical Spec: Config-Driven Integration Control Plane (Providers + MCP + API + Skills) #141 squash, so the parent was pinned at the now-unreachable pre-squash SHA. Bump points it at the merged head so check-drift goes green.

Test plan

  • `make typecheck` clean (only pre-existing pglite-postgis test setup error remains).
  • EXPLAIN ANALYZE against prod confirms the manage_feeds rewrite stays around 670ms.
  • CI green on this branch.

Summary by CodeRabbit

  • Performance

    • Faster feed listing by narrowing event scans and reducing database work for paged results.
  • Chores

    • Documented optional token→worker binding and added a conditional index to improve query efficiency.
    • Updated web package reference to a newer commit.
  • Tests

    • Updated integration tests to create and attach a test agent for watcher/classifier scenarios to improve isolation and realism.

Review Change Stack

Three follow-ups dropped from PR #786 during the squash-merge:

- db/schema.sql: insert the COLUMN COMMENT on
  personal_access_tokens.worker_id and move
  idx_personal_access_tokens_worker_id into its alphabetical spot so
  the file matches `dbmate up` output. The migrations CI job has been
  failing on this drift since 20260517030000_pat_worker_id_binding was
  added.
- manage_feeds.list_feeds: narrow the event-counts scan by feed_key as
  well as connection_id (ANY-array on both axes); the LEFT JOIN at the
  end drops any over-count from the cross product. Cuts the 50-feed
  list on a busy connection from ~1.5s to ~670ms in prod.
- Bump packages/web to ca12cd2 (owletto/main after PR #141), so the
  parent points at a submodule SHA reachable from owletto/main rather
  than at the now-orphaned pre-squash 2d2f5bf.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 17, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e438bea7-b5bf-4c1c-97b8-77ff582d6611

📥 Commits

Reviewing files that changed from the base of the PR and between 7074eb9 and fa2ba64.

📒 Files selected for processing (5)
  • packages/server/src/__tests__/integration/classifiers/classifiers-crud.test.ts
  • packages/server/src/__tests__/integration/classifiers/classifiers-isolation.test.ts
  • packages/server/src/__tests__/integration/watchers/feedback-contract.test.ts
  • packages/server/src/__tests__/integration/watchers/group-edit.test.ts
  • packages/server/src/__tests__/integration/watchers/watchers-crud.test.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/server/src/tests/integration/classifiers/classifiers-crud.test.ts

📝 Walkthrough

Walkthrough

The PR documents and indexes personal_access_tokens.worker_id, narrows event aggregation in handleListFeeds by filtering on feed_key, advances the packages/web submodule pointer, and updates integration tests to create and attach test agents when seeding watchers and classifiers.

Changes

worker_id Device Binding Documentation and Indexing

Layer / File(s) Summary
worker_id column documentation and index
db/schema.sql
A COMMENT ON COLUMN describes worker_id as an optional device binding that must match request worker_id when set. A partial index idx_personal_access_tokens_worker_id on worker_id filtered to non-NULL values is created and a duplicate index definition is removed.

Event Counts Query Optimization

Layer / File(s) Summary
feed_key filtering in event_counts CTE
packages/server/src/tools/admin/manage_feeds.ts
The event_counts CTE now restricts events by the distinct non-null feed_key values from the paged page CTE in addition to connection_id, preserving GROUP BY (connection_id, feed_key) aggregation while reducing scanned rows; comments updated to reflect planner/index-scan intent.

Web Package Submodule Update

Layer / File(s) Summary
submodule pointer advance
packages/web
The packages/web submodule reference is updated to a new commit hash b01682708902e51fc33b3f68edbc0f4319836509.

Integration Tests: Agent creation and attachment

Layer / File(s) Summary
classifiers and watchers test fixtures
packages/server/src/__tests__/integration/*
Multiple integration tests add createTestAgent imports, create a test agent during setup, and pass its agentId as agent_id when creating seeded watchers and classifiers across classifiers-crud, classifiers-isolation, feedback-contract, group-edit, and watchers-crud tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • lobu-ai/lobu#786: Similar modifications to handleListFeeds event counting and aggregation logic.

Suggested reviewers

  • codex-approver

Poem

🐰 I hopped through schema, left a note for worker_id,
Counts now peek at feed_key to keep scans light and spry.
The web pointer jumped a hash ahead,
Tests now carry agents where watchers tread.
A merry nibble — the rabbit twitches its nose and says "well-fed!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes all three main changes: schema.sql drift fix, manage_feeds feed_key optimization, and submodule bump.
Description check ✅ Passed The description provides comprehensive context for all three changes, includes a test plan with checkboxes (mostly completed), and references the related PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/schema-drift-and-feed-key-narrow

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@buremba
Copy link
Copy Markdown
Member Author

buremba commented May 17, 2026

pi review

Findings on the previous head:

  • Blockerdb/schema.sql had a stray double-blank between personal_access_tokens_organization_id_idx and the next COMMENT block; not byte-equal with dbmate up. Fixed in 3d42f82.
  • Non-blockers: feed_key = ANY(ARRAY(...)) is safe (feeds.feed_key is NOT NULL; NULL events.feed_key wouldn't match the final LEFT JOIN). Submodule SHA ca12cd2 is reachable from owletto/main head.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@packages/web`:
- Line 1: The pinned submodule commit ca12cd255e48d0b945fe6033f5d6e0dec5d21541
is not reachable from branch owletto/main; update the submodule reference so it
points to a commit that is an ancestor (or the current HEAD) of owletto/main:
fetch the remote for the submodule, find the correct commit on owletto/main (or
merge/rebase the intended changes into owletto/main), then update the submodule
pointer in this PR to that reachable commit (or to the merged head), commit the
submodule change and push; verify with git fetch --recurse-submodules and git
merge-base or git branch --contains to ensure the new SHA is contained in
owletto/main before merging.
🪄 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 Plus

Run ID: 33839fd8-e7c0-4711-b890-163e1bddb4c0

📥 Commits

Reviewing files that changed from the base of the PR and between 592d497 and 7d3a0f5.

📒 Files selected for processing (3)
  • db/schema.sql
  • packages/server/src/tools/admin/manage_feeds.ts
  • packages/web

Comment thread packages/web Outdated
@@ -1 +1 @@
Subproject commit 2d2f5bf253c130ea859982b8243cbf7a1a1719af
Subproject commit ca12cd255e48d0b945fe6033f5d6e0dec5d21541
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the submodule SHA and check if it's reachable from owletto/main

cd packages/web

echo "=== Current submodule pointer ==="
git rev-parse HEAD

echo ""
echo "=== Checking if SHA is reachable from origin/main ==="
git fetch origin main 2>/dev/null || echo "Could not fetch origin/main"
git merge-base --is-ancestor ca12cd255e48d0b945fe6033f5d6e0dec5d21541 origin/main && echo "SHA is reachable from origin/main" || echo "SHA is NOT reachable from origin/main"

echo ""
echo "=== Latest commits on origin/main ==="
git log origin/main --oneline -5 2>/dev/null || echo "Could not read origin/main log"

Repository: lobu-ai/lobu

Length of output: 830


🏁 Script executed:

cd packages/web
echo "=== Testing current pointer reachability ==="
git merge-base --is-ancestor 7d3a0f527e46b286c161b2cabc5bf4da0e8f5ed5 origin/main && echo "CURRENT SHA is reachable from origin/main" || echo "CURRENT SHA is NOT reachable from origin/main"

Repository: lobu-ai/lobu

Length of output: 147


🏁 Script executed:

# Check the submodule configuration and recent changes
echo "=== .gitmodules file ==="
cat .gitmodules | grep -A 3 "packages/web" || echo "No packages/web in .gitmodules"

echo ""
echo "=== Recent git log for .gitmodules ==="
git log -p --max-count=3 -- .gitmodules | head -100

echo ""
echo "=== Current git submodule status ==="
git submodule status packages/web

Repository: lobu-ai/lobu

Length of output: 308


Submodule SHA is not reachable from owletto/main.

The pinned commit ca12cd255e48d0b945fe6033f5d6e0dec5d21541 is not an ancestor of owletto/main. The pipeline check correctly fails because the SHA does not exist in the remote repository. This contradicts the PR objective, which claims this bump should point to owletto/main's merged head.

Verify that the correct commit was used and that it has been merged to owletto/main before this PR is merged.

🧰 Tools
🪛 GitHub Actions: Submodule Drift / 0_check-drift.txt

[error] 1-1: Pinned SHA is not reachable from owletto/main. Error emitted: "Pinned SHA $PINNED is not reachable from owletto/main." (git -C packages/web merge-base --is-ancestor "$PINNED" origin/main)

🤖 Prompt for 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.

In `@packages/web` at line 1, The pinned submodule commit
ca12cd255e48d0b945fe6033f5d6e0dec5d21541 is not reachable from branch
owletto/main; update the submodule reference so it points to a commit that is an
ancestor (or the current HEAD) of owletto/main: fetch the remote for the
submodule, find the correct commit on owletto/main (or merge/rebase the intended
changes into owletto/main), then update the submodule pointer in this PR to that
reachable commit (or to the merged head), commit the submodule change and push;
verify with git fetch --recurse-submodules and git merge-base or git branch
--contains to ensure the new SHA is contained in owletto/main before merging.

@buremba buremba merged commit 3a9d8fd into main May 17, 2026
20 of 21 checks passed
@buremba buremba deleted the fix/schema-drift-and-feed-key-narrow branch May 17, 2026 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants