revert(server): drop goals primitive — agents are the grouping concept#823
Merged
Conversation
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).
📝 WalkthroughWalkthroughThis PR removes the goals feature entirely from the codebase, including the ChangesGoals Feature Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
🤖 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 `@db/migrations/20260517160000_drop_goals_primitive.sql`:
- Around line 8-45: The down path in this migration is misleadingly reversible
because DROP TABLE public.goals and DROP COLUMN watchers.goal_id in the up
section permanently delete data; update the migration to either (A) perform a
safe archive/backfill step before dropping (e.g., copy public.goals and
watchers.goal_id into an archive table or JSON blob and restore it in the down
path) by adding an archival step before DROP TABLE public.goals and a
corresponding restore in CREATE TABLE and ALTER TABLE in the down section, or
(B) mark the migration as irreversible by removing the CREATE/ALTER statements
from the down section and documenting/raising an irreversible migration error;
locate the relevant statements (DROP TABLE public.goals, DROP INDEX
idx_watchers_goal_id, DROP COLUMN goal_id on public.watchers, and the CREATE
TABLE/ALTER TABLE in the down path) and implement one of these two options so
the migration does not claim to be data-reversible while actually destroying
data.
🪄 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: 4861a471-f09e-4bda-8203-42544661a673
📒 Files selected for processing (16)
db/migrations/20260517160000_drop_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
💤 Files with no reviewable changes (13)
- packages/server/src/tools/admin/manage_goals.ts
- packages/server/src/sandbox/namespaces/index.ts
- packages/server/src/sandbox/method-metadata.ts
- packages/server/src/sandbox/namespaces/goals.ts
- packages/server/src/tests/integration/watchers/goals-crud.test.ts
- packages/server/src/sandbox/namespaces/watchers.ts
- packages/server/src/db/embedded-schema-patches.ts
- packages/server/src/auth/tool-access.ts
- packages/server/src/sandbox/client-sdk.ts
- packages/server/src/tools/admin/index.ts
- packages/server/src/types/watchers.ts
- packages/server/src/tools/get_watchers.ts
- packages/server/src/tests/setup/test-mcp-client.ts
Comment on lines
+8
to
+45
| DROP INDEX IF EXISTS idx_watchers_goal_id; | ||
|
|
||
| ALTER TABLE public.watchers | ||
| DROP COLUMN IF EXISTS goal_id; | ||
|
|
||
| DROP INDEX IF EXISTS idx_goals_organization_id; | ||
|
|
||
| DROP TABLE IF EXISTS public.goals; | ||
|
|
||
| -- migrate:down | ||
|
|
||
| CREATE TABLE public.goals ( | ||
| id bigserial PRIMARY KEY, | ||
| organization_id text NOT NULL REFERENCES public.organization(id) ON DELETE CASCADE, | ||
| slug text NOT NULL, | ||
| name text NOT NULL, | ||
| description text, | ||
| status text NOT NULL DEFAULT 'active', | ||
| template_key text, | ||
| metadata jsonb NOT NULL DEFAULT '{}'::jsonb, | ||
| created_at timestamp with time zone NOT NULL DEFAULT now(), | ||
| updated_at timestamp with time zone NOT NULL DEFAULT now(), | ||
|
|
||
| CONSTRAINT goals_status_check | ||
| CHECK (status IN ('active', 'paused', 'archived')), | ||
| CONSTRAINT goals_org_slug_unique | ||
| UNIQUE (organization_id, slug) | ||
| ); | ||
|
|
||
| CREATE INDEX idx_goals_organization_id | ||
| ON public.goals (organization_id); | ||
|
|
||
| ALTER TABLE public.watchers | ||
| ADD COLUMN goal_id bigint REFERENCES public.goals(id) ON DELETE SET NULL; | ||
|
|
||
| CREATE INDEX idx_watchers_goal_id | ||
| ON public.watchers (goal_id) | ||
| WHERE goal_id IS NOT NULL; |
There was a problem hiding this comment.
This rollback path is schema-reversible, not data-reversible.
migrate:up permanently deletes public.goals rows and every watchers.goal_id value, but migrate:down only recreates empty structures. Any environment that applied 20260517150000 and created goal data will lose it on rollback or on an up/down/up cycle. Please either archive/backfill that data before dropping it, or explicitly treat this as an irreversible migration instead of shipping a misleading down path.
🤖 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 `@db/migrations/20260517160000_drop_goals_primitive.sql` around lines 8 - 45,
The down path in this migration is misleadingly reversible because DROP TABLE
public.goals and DROP COLUMN watchers.goal_id in the up section permanently
delete data; update the migration to either (A) perform a safe archive/backfill
step before dropping (e.g., copy public.goals and watchers.goal_id into an
archive table or JSON blob and restore it in the down path) by adding an
archival step before DROP TABLE public.goals and a corresponding restore in
CREATE TABLE and ALTER TABLE in the down section, or (B) mark the migration as
irreversible by removing the CREATE/ALTER statements from the down section and
documenting/raising an irreversible migration error; locate the relevant
statements (DROP TABLE public.goals, DROP INDEX idx_watchers_goal_id, DROP
COLUMN goal_id on public.watchers, and the CREATE TABLE/ALTER TABLE in the down
path) and implement one of these two options so the migration does not claim to
be data-reversible while actually destroying data.
This was referenced May 17, 2026
buremba
added a commit
that referenced
this pull request
May 17, 2026
Pulls in #823 (revert goals primitive), #824 (auto-provision default agent + manual trigger), #825 (owletto submodule bump to #154). Submodule merged origin/main into our feat/chrome-debugger-executor branch separately; bumps the parent's submodule pointer to the merge SHA so both the executor work and main's recent owletto changes (Mac app LobuClient + connection-settings UI rework + drop default-form-layout) ship together.
This was referenced May 17, 2026
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
Goals (added in #813) had no behavior of their own — just a nullable
watchers.goal_idFK pointing at a parallelgoalstable plus a parallel CRUD surface. Agents already encapsulate the watcher-grouping use case viawatchers.agent_id, so the goals layer was redundant.agent_id→ grouped under an agent → that IS the grouping concept.What's gone
db/migrations/20260517160000_drop_goals_primitive.sql— dropswatchers.goal_id+ thegoalstable. Reversible (verified withdbmate up→down→up).goals-primitive(inpackages/server/src/db/embedded-schema-patches.ts) — removed entirely; dev embedded DBs that previously ran it should re-init or drop the orphan table manually.packages/server/src/tools/admin/manage_goals.tsand its registration intools/admin/index.ts+auth/tool-access.ts.client.goalsSDK namespace (sandbox/namespaces/goals.ts) and thegoals.*entries insandbox/method-metadata.ts.goal_idfrommanage_watchers.ts(create/update/list projection),get_watchers.ts(SELECT +WatcherQueryRow+ return shape), andWatcherMetadataintypes/watchers.ts.goals-crudintegration test.Untouched
device_worker_id,agent_kind,notification_*,min_cooldown_seconds,last_fired_at) — also real plumbing.packages/owlettosubmodule SHA. If the web admin still has goal-management views, that's a separate cleanup in the submodule repo.Diff size
16 files changed, 49 insertions(+), 1072 deletions(-).
Test plan
make typecheckcleanmake build-packagescleandbmate up(against fresh local Postgres) applies through the new revert migration cleanlydbmate downthendbmate upre-applies — reversibledb/schema.sqlregenerated viadbmate dump+scripts/normalize-schema.sh, with theschema_migrations.versionvarchar(128)patch reappliedgoals-crud.test.tsremoved; nothing else exercises goalsSummary by CodeRabbit
goal_idfield is no longer available.