feat(server): tunable per-watcher execution_config for device-worker runs#1058
Merged
Conversation
…runs
Adds watchers.execution_config jsonb { timeout_seconds, max_budget_usd, model,
permission_mode, effort } so device-worker watcher runs (local claude/codex CLI)
are no longer capped at a hardcoded 60s. NULL = defaults.
- Migration: ADD COLUMN execution_config jsonb (idempotent).
- manage_watchers: TypeBox field + create/update/create_from_version + list
projection; sandbox SDK WatcherCreateInput/WatcherUpdateInput types.
- worker-api: emit payload.watcher.execution_config in the poll envelope.
- Bumps packages/owletto pointer to the paired dispatcher + UI change.
Round-trip + migration validated (watchers-crud test, full integration suite,
embedded PG18).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a nullable JSONB ChangesWatcher Execution Configuration Support
Dependency Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
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 `@packages/server/src/tools/admin/manage_watchers.ts`:
- Around line 409-452: The schema for execution_config currently only accepts an
object so clients cannot clear it; update the Type.Optional wrapper around
Type.Object for execution_config to allow explicit null as a valid value (e.g.,
make it a Type.Optional(Type.Union([Type.Null(), Type.Object(...)])) so callers
can send null to clear saved config), keep the same inner object shape,
descriptions and additionalProperties settings, and ensure any downstream
validators that check execution_config handle null correctly.
🪄 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: bf1e8a9b-0891-46be-a92f-8f7fba7aa38e
📒 Files selected for processing (6)
db/migrations/20260525120000_watcher_execution_config.sqlpackages/owlettopackages/server/src/__tests__/integration/watchers/watchers-crud.test.tspackages/server/src/sandbox/namespaces/watchers.tspackages/server/src/tools/admin/manage_watchers.tspackages/server/src/worker-api.ts
Addresses CodeRabbit/pi review: the nullable column was write-once because the schema only accepted an object. Widen the TypeBox + SDK update type to a null-union so update can reset execution_config to NULL/defaults (omitted = unchanged, null = clear, object = replace). The UPDATE SQL already mapped null → SQL NULL via toJsonParam. Test extended to cover the null-clear path.
Addresses multi-aspect review of the watcher execution_config PR: - Runtime validation (watcher-execution-config.ts): manage_watchers args are not schema-checked at the boundary, so a type-wrong value (e.g. timeout_seconds as a string) would persist and then fail the device-worker's strict payload decode — silently disabling every run of that watcher. Compile the TypeBox schema and reject invalid types/enums/range on create + update. - Security: gate elevated permission modes (bypassPermissions, dontAsk) behind owner/admin. device_worker_id is not ownership-checked, so a member-write actor could otherwise pin a watcher to another user's device and run an agent there with permissions bypassed. System/internal callers are exempt. - Bound timeout_seconds with a 24h maximum so an unbounded value can't wedge a device's serialized watcher queue. - query_sql parity: add execution_config to QUERYABLE_SCHEMA (watchers), fixing the table-schema drift test (DATABASE_URL mode) and enabling admin SELECTs. - Tests: unit coverage for validation + the owner gate; integration coverage for the /api/workers/poll payload carrying execution_config end-to-end.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/tools/admin/manage_watchers.ts (1)
1187-1189:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRe-validate inherited
execution_configbefore cloning.
create_from_versioncopiesw.execution_configstraight into the new watcher without going throughassertValidExecutionConfig. That bypasses the owner/admin gate forbypassPermissions/dontAsk, and it can also replicate any legacy invalid config already stored in the source row.Suggested fix
if (!version.agent_id) { // Source watcher has no agent — cloning would silently inherit null and // produce active zombies the scheduler skips. Same invariant as handleCreate. throw new ToolUserError( `Source watcher version ${args.version_id} has no agent_id; assign an agent on the source before cloning.` ); } + const inheritedExecutionConfig = version.execution_config ?? null; + assertValidExecutionConfig(inheritedExecutionConfig, ctx); // Fetch entity names for name pattern substitution const entityRows = await sql` @@ ${`{${entityId}}`}::bigint[], ${version.schedule ?? null}, ${version.schedule ? nextRunAt(version.schedule as string) : null}, ${version.agent_id ?? null}, ${version.scheduler_client_id ?? null}, - ${toJsonParam(sql, version.model_config)}, ${toJsonParam(sql, version.execution_config)}, ${toJsonParam(sql, sources)}, + ${toJsonParam(sql, version.model_config)}, ${toJsonParam(sql, inheritedExecutionConfig)}, ${toJsonParam(sql, sources)}, ${(version.version as number) ?? 1}, ${sharedVersionId}, ${toTextArrayParam((version.tags as string[]) || [])}::text[], 'active', ${createdBy}, NOW(), NOW(),Also applies to: 1243-1255
🤖 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_watchers.ts` around lines 1187 - 1189, The code path that clones watchers in create_from_version currently copies w.execution_config directly and bypasses validation; update create_from_version to run the source execution_config through assertValidExecutionConfig (the same validation used for new watchers) before assigning it to the cloned watcher, ensuring owner/admin-only flags like bypassPermissions/dontAsk are enforced and legacy invalid configs are rejected or normalized; apply the same change to the related cloning code around the other block mentioned (lines ~1243-1255) so both cloning locations call assertValidExecutionConfig and use its validated/normalized result instead of w.execution_config raw.
🧹 Nitpick comments (1)
packages/server/src/tools/admin/manage_watchers.ts (1)
731-732: ⚡ Quick winRemove the unused
envparameter fromhandleUpdate.
handleUpdatedoes not useenv, so this should be deleted rather than kept as_env.As per coding guidelines, "Fix unused parameters by deleting them, not by prefixing with `_`".Suggested fix
- update: () => handleUpdate(args, env, ctx), + update: () => handleUpdate(args, ctx), @@ async function handleUpdate( args: ManageWatchersArgs, - _env: Env, ctx: ToolContext ): Promise<{ action: 'update'; watcher_id: string; updated_fields: string[] }> {Also applies to: 1278-1281
🤖 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_watchers.ts` around lines 731 - 732, handleUpdate currently declares an unused env parameter; remove that parameter from the handleUpdate function signature and update every call site to stop passing env. Specifically, change the handler registration that reads update: () => handleUpdate(args, env, ctx) (and any similar references near the other mapping around handleCreateVersion) to call handleUpdate with only the required arguments (e.g., handleUpdate(args, ctx) or handleUpdate(args, ctx, ...) as appropriate), and remove the unused env parameter from the function definition of handleUpdate itself so no callers or signatures include env.
🤖 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.
Outside diff comments:
In `@packages/server/src/tools/admin/manage_watchers.ts`:
- Around line 1187-1189: The code path that clones watchers in
create_from_version currently copies w.execution_config directly and bypasses
validation; update create_from_version to run the source execution_config
through assertValidExecutionConfig (the same validation used for new watchers)
before assigning it to the cloned watcher, ensuring owner/admin-only flags like
bypassPermissions/dontAsk are enforced and legacy invalid configs are rejected
or normalized; apply the same change to the related cloning code around the
other block mentioned (lines ~1243-1255) so both cloning locations call
assertValidExecutionConfig and use its validated/normalized result instead of
w.execution_config raw.
---
Nitpick comments:
In `@packages/server/src/tools/admin/manage_watchers.ts`:
- Around line 731-732: handleUpdate currently declares an unused env parameter;
remove that parameter from the handleUpdate function signature and update every
call site to stop passing env. Specifically, change the handler registration
that reads update: () => handleUpdate(args, env, ctx) (and any similar
references near the other mapping around handleCreateVersion) to call
handleUpdate with only the required arguments (e.g., handleUpdate(args, ctx) or
handleUpdate(args, ctx, ...) as appropriate), and remove the unused env
parameter from the function definition of handleUpdate itself so no callers or
signatures include env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 601dff65-757e-4532-85c5-2e79b4f2781e
📒 Files selected for processing (6)
packages/server/src/__tests__/integration/watchers/manual-trigger.test.tspackages/server/src/__tests__/integration/watchers/watchers-crud.test.tspackages/server/src/__tests__/unit/watcher-execution-config.test.tspackages/server/src/tools/admin/manage_watchers.tspackages/server/src/tools/admin/watcher-execution-config.tspackages/server/src/utils/table-schema.ts
This was referenced May 26, 2026
Member
Author
|
bug_free 88, simplicity 86, slop 5, bugs 0, 0 blockers Read local diff and suite logs: typecheck/unit/integration all passed. git diff --check clean. Inspected owletto submodule diff and local claude CLI flag support. Skipped server boot because DATABASE_URL was not exported in this review shell. Full verdict JSON{
"bug_free_confidence": 88,
"bugs": 0,
"slop": 5,
"simplicity": 86,
"blockers": [],
"change_type": "feat",
"behavior_change_risk": "medium",
"tests_adequate": true,
"suggested_fixes": [],
"notes": "Read local diff and suite logs: typecheck/unit/integration all passed. git diff --check clean. Inspected owletto submodule diff and local claude CLI flag support. Skipped server boot because DATABASE_URL was not exported in this review shell.",
"categories": {
"src": 165,
"tests": 247,
"docs": 0,
"config": 0,
"deps": 2,
"migrations": 18,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
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.
Why
Device-worker watcher runs (Owletto Mac app's `WatcherDispatcher`, which spawns the local `claude`/`codex` CLI) were capped at a hardcoded 60s timeout, so every real agent loop got SIGTERM-killed — the "claude exited via SIGTERM after 60s timeout" failures on the menubar Recent Activity. Make per-run execution tunable per watcher.
What
New nullable `watchers.execution_config` jsonb (mirrors `model_config`; one column, no migration for future knobs):
```jsonc
{ "timeout_seconds": 1800, "max_budget_usd": 2, "model": "opus",
"permission_mode": "acceptEdits", "effort": "high" }
```
Pairs with lobu-ai/owletto#222 (dispatcher: default 60s→600s, config-driven claude flags, UI). This PR bumps `packages/owletto` to that commit.
Validation (red→green, run locally)
Notes
🤖 Generated with Claude Code
Summary by CodeRabbit