feat(schema): goals primitive — top-level handle for watcher hierarchy#813
Merged
Conversation
Adds a `goals` table (org-scoped, slug + name + status + optional template key + jsonb metadata) and a nullable `watchers.goal_id` FK (ON DELETE SET NULL). Goals are the surface the canvas (#801) hangs off — watchers stay the executable unit, goals are the user-facing intent that groups them. - Migration: db/migrations/20260517150000_goals_primitive.sql + idempotent mirror in packages/server/src/db/embedded-schema-patches.ts so PGlite restarts replay cleanly. Boot-twice verified locally. - Tool: manage_goals (create/update/get/list/archive/delete) modeled on manage_view_templates + manage_feeds, with org-scope checks, lifecycle events, and slug validation. Wired into the REST tool surface, ClientSDK (client.goals), method-metadata, and tool-access policy. - Watchers: optional goal_id on create/update with org-scope validation (the FK alone allows cross-org goal ids). Surfaced in WatcherMetadata and selected by get_watcher. - Tests: goals-crud integration suite covers create/update/list/archive/ delete, cross-org isolation, slug validation, duplicate rejection, watcher linkage, ON DELETE SET NULL on goal delete, cross-org goal_id rejection, and org-cascade. Refs #800. Composes with #796 (product epic) and #801 (canvas surface). Goal-template loading from disk remains out of scope — owletto-side.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds an organization-scoped ChangesGoals Feature — Database, API, and SDK
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 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 Comment |
![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: b5ecea0198
ℹ️ 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".
Comment on lines
+360
to
+364
| goal_id: Type.Optional( | ||
| Type.Union([Type.Number(), Type.Null()], { | ||
| description: | ||
| '[create/update] Optional goal_id linking this watcher to a goal (manage_goals). Pass null to unlink.', | ||
| }) |
There was a problem hiding this comment.
When callers set goal_id on create/update, handleList still does not select i.goal_id, so client.watchers.list() / list_watchers cannot tell which returned watchers belong to which goal. In the canvas/hierarchy flow this forces an N+1 watchers.get() lookup per watcher (or raw SQL) just to reconstruct the grouping, and bulk consumers will render goal-linked watchers as ungrouped. Include i.goal_id in the list SELECT/result whenever this field is accepted here.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/server/src/__tests__/integration/watchers/goals-crud.test.ts (2)
108-134: ⚡ Quick winConsider adding test coverage for invalid status values.
The test creates goals with valid statuses (
'active','paused'), but there's no test verifying that invalid status values are rejected. The PR summary states thatmanage_goalsvalidates status values.Suggested additional test case
it('rejects an invalid status', async () => { await expect( owner.goals.create({ slug: 'test-invalid-status', name: 'Test', status: 'invalid_status' }) ).rejects.toThrow(/invalid.*status/i); });🤖 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/server/src/__tests__/integration/watchers/goals-crud.test.ts` around lines 108 - 134, Add a test that asserts invalid status values are rejected by the goals API: call owner.goals.create with a bad status string (e.g. 'invalid_status') and assert the promise rejects with an error mentioning invalid status (use expect(...).rejects.toThrow(/invalid.*status/i) or similar). Place this alongside the existing tests in packages/server/src/__tests__/integration/watchers/goals-crud.test.ts and reference owner.goals.create for creation and owner.goals.delete if you need cleanup; ensure the assertion checks that manage_goals validation prevents invalid statuses.
167-220: ⚡ Quick winAdd verification for watcher counts in goal results.
The PR summary states that goal operations "enrich results with watcher counts," but the tests don't verify that
watcher_countappears in the response. Consider adding assertions to verify this field after creating linked watchers.Example verification
After creating a linked watcher (around line 180), verify the count:
const goalWithCount = (await owner.goals.get({ goal_id: goal.goal.id })) as { goal: { watcher_count: number }; }; expect(goalWithCount.goal.watcher_count).toBe(1);🤖 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/server/src/__tests__/integration/watchers/goals-crud.test.ts` around lines 167 - 220, The test does not assert that goal responses include the new watcher_count field; update the test around the watcher creation and subsequent changes to call owner.goals.get({ goal_id }) and assert goal.watcher_count equals expected values (e.g., 1 after creating the linked watcher, 0 after unlink/goal delete). Specifically add checks using owner.goals.get for goal.goal.id after owner.watchers.create, after owner.watchers.update(... goal_id: null), and after owner.goals.delete to confirm watcher_count is updated as expected.
🤖 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/server/src/tools/admin/manage_goals.ts`:
- Around line 352-386: The handleList function currently returns total:
goals.length (capped by limit) which breaks pagination; add a separate COUNT
query using the same filters to compute the true total number of matching goals
for ctx.organizationId (and args.status when provided). Specifically, after
building/performing the main SELECT (or before), run a sql`SELECT COUNT(*)::int
FROM goals WHERE organization_id = ${ctx.organizationId}` and include the status
filter when args.status is set, store that count in a variable (e.g.,
totalCount) and return total: totalCount instead of goals.length; keep the
existing watcher_count query and limit/offset logic unchanged.
---
Nitpick comments:
In `@packages/server/src/__tests__/integration/watchers/goals-crud.test.ts`:
- Around line 108-134: Add a test that asserts invalid status values are
rejected by the goals API: call owner.goals.create with a bad status string
(e.g. 'invalid_status') and assert the promise rejects with an error mentioning
invalid status (use expect(...).rejects.toThrow(/invalid.*status/i) or similar).
Place this alongside the existing tests in
packages/server/src/__tests__/integration/watchers/goals-crud.test.ts and
reference owner.goals.create for creation and owner.goals.delete if you need
cleanup; ensure the assertion checks that manage_goals validation prevents
invalid statuses.
- Around line 167-220: The test does not assert that goal responses include the
new watcher_count field; update the test around the watcher creation and
subsequent changes to call owner.goals.get({ goal_id }) and assert
goal.watcher_count equals expected values (e.g., 1 after creating the linked
watcher, 0 after unlink/goal delete). Specifically add checks using
owner.goals.get for goal.goal.id after owner.watchers.create, after
owner.watchers.update(... goal_id: null), and after owner.goals.delete to
confirm watcher_count is updated as expected.
🪄 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: 10a2928c-9251-4654-9f0f-f0d7ae5f86f7
📒 Files selected for processing (16)
db/migrations/20260517150000_goals_primitive.sqldb/schema.sqlpackages/server/src/__tests__/integration/watchers/goals-crud.test.tspackages/server/src/__tests__/setup/test-mcp-client.tspackages/server/src/auth/tool-access.tspackages/server/src/db/embedded-schema-patches.tspackages/server/src/sandbox/client-sdk.tspackages/server/src/sandbox/method-metadata.tspackages/server/src/sandbox/namespaces/goals.tspackages/server/src/sandbox/namespaces/index.tspackages/server/src/sandbox/namespaces/watchers.tspackages/server/src/tools/admin/index.tspackages/server/src/tools/admin/manage_goals.tspackages/server/src/tools/admin/manage_watchers.tspackages/server/src/tools/get_watchers.tspackages/server/src/types/watchers.ts
Comment on lines
+352
to
+386
| async function handleList( | ||
| args: Extract<ManageGoalsArgs, { action: 'list' }>, | ||
| ctx: ToolContext | ||
| ): Promise<ManageGoalsResult> { | ||
| const sql = getDb(); | ||
| await requireOrgReadAccess(sql, ctx); | ||
| if (!ctx.organizationId) { | ||
| throw new Error('Organization context is required'); | ||
| } | ||
| if (args.status !== undefined) { | ||
| assertValidStatus(args.status); | ||
| } | ||
|
|
||
| const limit = Math.min(args.limit ?? 100, 500); | ||
| const offset = args.offset ?? 0; | ||
|
|
||
| const rows = args.status | ||
| ? await sql` | ||
| SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count | ||
| FROM goals g | ||
| WHERE g.organization_id = ${ctx.organizationId} AND g.status = ${args.status} | ||
| ORDER BY g.created_at DESC | ||
| LIMIT ${limit} OFFSET ${offset} | ||
| ` | ||
| : await sql` | ||
| SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count | ||
| FROM goals g | ||
| WHERE g.organization_id = ${ctx.organizationId} | ||
| ORDER BY g.created_at DESC | ||
| LIMIT ${limit} OFFSET ${offset} | ||
| `; | ||
|
|
||
| const goals = (rows as Record<string, unknown>[]).map(mapGoalRow); | ||
| return { action: 'list', goals, total: goals.length, limit, offset }; | ||
| } |
There was a problem hiding this comment.
Fix the total field to return actual database count, not fetched rows.
Line 385 returns total: goals.length, which is the count of rows fetched (capped by limit), not the total count of matching goals in the database. This breaks pagination—clients cannot determine whether more results exist.
For example, if there are 1,000 goals and limit=100, the response will incorrectly show total: 100 instead of total: 1000.
🔧 Proposed fix: add a separate COUNT query
const limit = Math.min(args.limit ?? 100, 500);
const offset = args.offset ?? 0;
+ // Get total count of matching goals
+ const countRows = args.status
+ ? await sql`
+ SELECT COUNT(*)::int AS total
+ FROM goals g
+ WHERE g.organization_id = ${ctx.organizationId} AND g.status = ${args.status}
+ `
+ : await sql`
+ SELECT COUNT(*)::int AS total
+ FROM goals g
+ WHERE g.organization_id = ${ctx.organizationId}
+ `;
+ const total = Number(countRows[0]?.total ?? 0);
+
const rows = args.status
? await sql`
SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count
FROM goals g
WHERE g.organization_id = ${ctx.organizationId} AND g.status = ${args.status}
ORDER BY g.created_at DESC
LIMIT ${limit} OFFSET ${offset}
`
: await sql`
SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count
FROM goals g
WHERE g.organization_id = ${ctx.organizationId}
ORDER BY g.created_at DESC
LIMIT ${limit} OFFSET ${offset}
`;
const goals = (rows as Record<string, unknown>[]).map(mapGoalRow);
- return { action: 'list', goals, total: goals.length, limit, offset };
+ return { action: 'list', goals, total, limit, offset };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function handleList( | |
| args: Extract<ManageGoalsArgs, { action: 'list' }>, | |
| ctx: ToolContext | |
| ): Promise<ManageGoalsResult> { | |
| const sql = getDb(); | |
| await requireOrgReadAccess(sql, ctx); | |
| if (!ctx.organizationId) { | |
| throw new Error('Organization context is required'); | |
| } | |
| if (args.status !== undefined) { | |
| assertValidStatus(args.status); | |
| } | |
| const limit = Math.min(args.limit ?? 100, 500); | |
| const offset = args.offset ?? 0; | |
| const rows = args.status | |
| ? await sql` | |
| SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count | |
| FROM goals g | |
| WHERE g.organization_id = ${ctx.organizationId} AND g.status = ${args.status} | |
| ORDER BY g.created_at DESC | |
| LIMIT ${limit} OFFSET ${offset} | |
| ` | |
| : await sql` | |
| SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count | |
| FROM goals g | |
| WHERE g.organization_id = ${ctx.organizationId} | |
| ORDER BY g.created_at DESC | |
| LIMIT ${limit} OFFSET ${offset} | |
| `; | |
| const goals = (rows as Record<string, unknown>[]).map(mapGoalRow); | |
| return { action: 'list', goals, total: goals.length, limit, offset }; | |
| } | |
| async function handleList( | |
| args: Extract<ManageGoalsArgs, { action: 'list' }>, | |
| ctx: ToolContext | |
| ): Promise<ManageGoalsResult> { | |
| const sql = getDb(); | |
| await requireOrgReadAccess(sql, ctx); | |
| if (!ctx.organizationId) { | |
| throw new Error('Organization context is required'); | |
| } | |
| if (args.status !== undefined) { | |
| assertValidStatus(args.status); | |
| } | |
| const limit = Math.min(args.limit ?? 100, 500); | |
| const offset = args.offset ?? 0; | |
| // Get total count of matching goals | |
| const countRows = args.status | |
| ? await sql` | |
| SELECT COUNT(*)::int AS total | |
| FROM goals g | |
| WHERE g.organization_id = ${ctx.organizationId} AND g.status = ${args.status} | |
| ` | |
| : await sql` | |
| SELECT COUNT(*)::int AS total | |
| FROM goals g | |
| WHERE g.organization_id = ${ctx.organizationId} | |
| `; | |
| const total = Number(countRows[0]?.total ?? 0); | |
| const rows = args.status | |
| ? await sql` | |
| SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count | |
| FROM goals g | |
| WHERE g.organization_id = ${ctx.organizationId} AND g.status = ${args.status} | |
| ORDER BY g.created_at DESC | |
| LIMIT ${limit} OFFSET ${offset} | |
| ` | |
| : await sql` | |
| SELECT g.*, (SELECT COUNT(*)::int FROM watchers w WHERE w.goal_id = g.id) AS watcher_count | |
| FROM goals g | |
| WHERE g.organization_id = ${ctx.organizationId} | |
| ORDER BY g.created_at DESC | |
| LIMIT ${limit} OFFSET ${offset} | |
| `; | |
| const goals = (rows as Record<string, unknown>[]).map(mapGoalRow); | |
| return { action: 'list', goals, total, limit, offset }; | |
| } |
🤖 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/server/src/tools/admin/manage_goals.ts` around lines 352 - 386, The
handleList function currently returns total: goals.length (capped by limit)
which breaks pagination; add a separate COUNT query using the same filters to
compute the true total number of matching goals for ctx.organizationId (and
args.status when provided). Specifically, after building/performing the main
SELECT (or before), run a sql`SELECT COUNT(*)::int FROM goals WHERE
organization_id = ${ctx.organizationId}` and include the status filter when
args.status is set, store that count in a variable (e.g., totalCount) and return
total: totalCount instead of goals.length; keep the existing watcher_count query
and limit/offset logic unchanged.
# Conflicts: # db/schema.sql # packages/server/src/db/embedded-schema-patches.ts
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
6 tasks
buremba
added a commit
that referenced
this pull request
May 17, 2026
#823) Goals (added in #813) had no behavior of their own — just a nullable FK from watchers.goal_id into a parallel `goals` table plus a parallel CRUD surface. Agents already encapsulate the watcher-grouping use case via watchers.agent_id; goals were the redundant layer. The Mac app stopped using the primitive in lobu-ai/owletto#151, and the primitive never shipped in a release (v7.0.0 doesn't include it; v7.1.0 is still open). Removed: - db/migrations/20260517160000_drop_goals_primitive.sql (drops the watchers.goal_id column and the goals table; reversible) - packages/server/src/db/embedded-schema-patches.ts: drop the `goals-primitive` patch entry - packages/server/src/tools/admin/manage_goals.ts and its registration - packages/server/src/sandbox/namespaces/goals.ts (client.goals SDK) and its method-metadata entries - goal_id from manage_watchers.ts (create/update/list), get_watchers.ts, and WatcherMetadata - manage_goals from auth/tool-access scope tables - goals-crud integration test Untouched: the dispatcher (#814), the watcher schema additions from #811, and packages/owletto (separate submodule).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
goalstable (org-scoped: slug, name, status, optionaltemplate_key, jsonbmetadata) plus a nullablewatchers.goal_idFK withON DELETE SET NULL. Goals are the user-facing intent that groups watchers; watchers stay the executable unit.manage_goalsadmin tool withcreate / update / get / list / archive / deleteactions, wired into the REST tool surface, ClientSDK (client.goals), method-metadata, and tool-access policy. Modeled onmanage_view_templates+manage_feeds.manage_watchersaccepts an optionalgoal_idoncreate/update(with cross-org validation — the FK alone would accept any goal id).WatcherMetadata+get_watchernow surfacegoal_id.goals-primitive) mirrors the dbmate migration so PGlite restarts replay cleanly. Sequenced afteragents-per-org-pk-phase-c; composes with the pending watcher-schema additions (Watcher schema: adddevice_worker_id,agent_kind, notification opt-in, cooldown columns #799) since the only watcher-side change here is the new column.Test plan
make typecheckclean.make build-packagesclean (server + cli bundles).bun test src/__tests__/unit/sandbox/method-metadata.test.ts— 8/8 passing (covers the newgoals.*metadata + SDK enumeration).LOBU_DATA_DIR— first boot runs migrations, second boot runs the embedded patch (goals-primitive) without errors and emits the expected "already exists, skipping" notices for the idempotent paths.goals-crud.test.ts) covering create/update/list/archive/delete, cross-org isolation, slug validation, duplicate rejection, watcher linkage, ON DELETE SET NULL on goal delete, cross-orggoal_idrejection, and org-cascade.Scope
Closes #800. Composes with #796 (product epic) and is the schema dependency for #801 (canvas surface). Goal-template loading from on-disk YAML is out of scope — that's a separate owletto-side concern.
Notes
20260517150000was picked to land after whatever timestamp Watcher schema: adddevice_worker_id,agent_kind, notification opt-in, cooldown columns #799 (watcher-schema-additions) lands on; the embedded patch comment notes the ordering relationship explicitly.organization_id integer; the repo convention istext(everywhere else org-scoped). Used the existing convention.Summary by CodeRabbit
New Features
Tests