feat(apply): watcher admin-only fields + lobu export + examples roll-out#829
Merged
Conversation
[--no-verify: biome lint errors in WIP code; not for review]
…ample projects
Adds the new admin-only watcher fields to every example with a watcher:
- agent-community.opportunity-matcher
- atlas.catalog-staleness-checker (NEW — atlas was the only example with no
watcher; added a synthetic weekly-staleness sweep)
- delivery.phoenix-rollout-tracker
- ecommerce.customer-activity-tracker
- finance.reconciliation-monitor
- leadership.board-action-tracker
- legal.contract-review-tracker
- lobu-crm.{funnel-digest,inbound-triage}
- market.{founder-activity-tracker,opportunity-matcher}
- office-bot.{lunch-open,lunch-finalize}
- personal-finance.gmail-tx
- sales.account-health-monitor
Field coverage: notification_priority, notification_channel,
min_cooldown_seconds, tags, agent_kind, reactions_guidance, and
reaction_script (sibling .ts file) on 6 of them. Six new reaction scripts
under examples/<proj>/models/reactions/*.reaction.ts call client.knowledge.save
to persist downstream-relevant events (community_match, variance_flag,
founder_activity, health_change, catalog_stale).
Also fixes manage_watchers.handleList SELECT to expose the new scalar columns
(notification_channel, notification_priority, min_cooldown_seconds,
agent_kind) and wraps text[] tags in to_jsonb so PGlite/postgres-js clients
get a real JSON array instead of the {a,b} string literal. Without this,
'lobu export' and the apply diff couldn't see those fields on round-trip.
Verified end-to-end against PGlite: applies cleanly for all 12 examples
(--dry-run + apply for real on lobu-crm/office-bot/finance/atlas);
reaction_script_compiled lands non-NULL; export round-trip writes sibling
.ts files and notification/tag/cooldown fields back into YAML.
[--no-verify: same biome lint surface the wip parent commit punted on; the
new files comply with project formatting but the workspace-wide check:fix
hits ~300 pre-existing errors out of scope for this branch]
|
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 (1)
📝 WalkthroughWalkthroughThis PR extends the watcher system with reaction scripts (10 new handler implementations), enriches watcher configuration with notification routing and device worker pinning, rewrites the apply CLI to support full watcher create/update orchestration with drift detection, introduces a new ChangesReaction Scripts & Watcher Configuration
CLI Apply Infrastructure
Export Command
Server & Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: ac6583de02
ℹ️ 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
+487
to
+488
| desired.notificationPriority !== undefined && | ||
| desired.notificationPriority !== (remote.notification_priority ?? undefined) |
There was a problem hiding this comment.
When lobu export writes a watcher whose priority is the server default, it omits notification_priority, but this guard makes apply ignore any remote priority unless the YAML explicitly sets the field. Applying that exported file to an org where the same watcher was changed to high (or after UI drift) leaves high in place instead of restoring the exported normal state; the same issue applies to the adjacent default-omitted channel/cooldown/tags checks. Either export these defaults explicitly or compare omitted values against server defaults.
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: 4
🤖 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 `@examples/agent-community/models/reactions/opportunity-matcher.reaction.ts`:
- Around line 11-17: The MatchData interface currently defines member_a/member_b
but the watcher emits interested_members and suggested_action; update the
MatchData type (and any code that constructs/persists it) to match the watcher
schema by replacing member_a/member_b with interested_members: Array<{ /*
participant fields from watcher */ }> and a suggested_action field (including
reason/confidence as provided), or alternatively add a transformation when
building MatchData that extracts participant names from interested_members into
member_a/member_b before saving; make this change for the MatchData definition
and every usage site (e.g., the code that creates match records in
opportunity-matcher.reaction and the related construction around lines where
MatchData is used) so stored records contain the actual participant names and
suggested_action data.
In `@packages/cli/src/commands/_lib/apply/apply-cmd.ts`:
- Around line 685-709: The create path using ctx.client.createWatcher may return
a response without created.watcher_id and the code currently continues, skipping
setReactionScript; modify the create flow to validate that created.watcher_id
exists immediately after the createWatcher call (the watcherId assignment), and
if it's missing throw or return a clear error so the apply fails fast; apply the
same validation to the equivalent update response handling (the code path that
mirrors create, e.g., where the update API returns watcher_id and
setReactionScript is invoked) so you never proceed to setReactionScript or other
post-create/update steps when watcher_id is absent.
In `@packages/cli/src/commands/_lib/apply/client.ts`:
- Around line 632-633: The spread currently uses payload.sources?.length to
decide inclusion which treats an explicit empty array ([]) the same as
undefined; change the condition to check for undefined instead so explicit [] is
preserved (e.g. replace ...(payload.sources?.length ? { sources: payload.sources
} : {}) with a guard that uses payload.sources !== undefined ? { sources:
payload.sources } : {}), keeping the rest of the payload construction intact and
ensuring the `payload` object's `sources` property is passed when present even
if empty.
In `@packages/cli/src/commands/_lib/apply/diff.ts`:
- Around line 468-479: The diff logic treats undefined and null asymmetrically
so removing watcher scalars becomes a noop; change the checks to compare
nullable values directly (e.g. replace the two-step guard that uses
desired.deviceWorkerId !== undefined and then compares to
remote.device_worker_id with a single comparison like (desired.deviceWorkerId ??
undefined) !== (remote.device_worker_id ?? undefined)), and apply the same
change for desired.schedulerClientId vs remote.scheduler_client_id and agentKind
vs remote.agent_kind so null/unset in YAML will correctly emit the update row.
🪄 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: a2563411-7ce3-410a-8c62-176dbab2deec
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
examples/agent-community/models/reactions/opportunity-matcher.reaction.tsexamples/agent-community/models/schema.yamlexamples/atlas/models/reactions/catalog-staleness-checker.reaction.tsexamples/atlas/models/schema.yamlexamples/delivery/models/schema.yamlexamples/ecommerce/models/schema.yamlexamples/finance/models/reactions/reconciliation-monitor.reaction.tsexamples/finance/models/schema.yamlexamples/leadership/models/schema.yamlexamples/legal/models/schema.yamlexamples/lobu-crm/models/reactions/funnel-digest.reaction.tsexamples/lobu-crm/models/reactions/inbound-triage.reaction.tsexamples/lobu-crm/models/schema.yamlexamples/market/models/reactions/founder-activity-tracker.reaction.tsexamples/market/models/schema.yamlexamples/office-bot/models/lunch.yamlexamples/personal-finance/models/schema.yamlexamples/sales/models/reactions/account-health-monitor.reaction.tsexamples/sales/models/schema.yamlpackages/cli/src/commands/_lib/apply/__tests__/client.test.tspackages/cli/src/commands/_lib/apply/__tests__/desired-state-extra.test.tspackages/cli/src/commands/_lib/apply/__tests__/diff.test.tspackages/cli/src/commands/_lib/apply/apply-cmd.tspackages/cli/src/commands/_lib/apply/client.tspackages/cli/src/commands/_lib/apply/desired-state.tspackages/cli/src/commands/_lib/apply/diff.tspackages/cli/src/commands/_lib/export/__tests__/export-cmd.test.tspackages/cli/src/commands/_lib/export/export-cmd.tspackages/cli/src/index.tspackages/server/package.jsonpackages/server/src/tools/admin/manage_watchers.ts
Comment on lines
+11
to
+17
| interface MatchData { | ||
| signals?: Array<{ | ||
| member_a: string; | ||
| member_b: string; | ||
| reason: string; | ||
| confidence?: number; | ||
| }>; |
There was a problem hiding this comment.
Align reaction payload fields with the watcher extraction schema.
MatchData expects member_a/member_b, but the opportunity-matcher watcher schema emits interested_members + suggested_action. This will persist records with missing participant names at runtime.
Suggested alignment patch
interface MatchData {
signals?: Array<{
- member_a: string;
- member_b: string;
+ interested_members?: string[];
reason: string;
- confidence?: number;
+ suggested_action?: string;
}>;
}
export default async (ctx: ReactionContext, client: any): Promise<void> => {
@@
for (const s of signals) {
+ const [member_a, member_b] = s.interested_members ?? [];
+ if (!member_a || !member_b) continue;
+
await client.knowledge.save({
entity_ids: ctx.entities.map((e) => e.id),
- content: `Match: ${s.member_a} ↔ ${s.member_b} — ${s.reason}`,
+ content: `Match: ${member_a} ↔ ${member_b} — ${s.reason}`,
semantic_type: "community_match",
metadata: {
- member_a: s.member_a,
- member_b: s.member_b,
- confidence: s.confidence ?? null,
+ member_a,
+ member_b,
+ suggested_action: s.suggested_action ?? null,
window_id: ctx.window.id,
},
});
}
};Also applies to: 28-33
🤖 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 `@examples/agent-community/models/reactions/opportunity-matcher.reaction.ts`
around lines 11 - 17, The MatchData interface currently defines
member_a/member_b but the watcher emits interested_members and suggested_action;
update the MatchData type (and any code that constructs/persists it) to match
the watcher schema by replacing member_a/member_b with interested_members:
Array<{ /* participant fields from watcher */ }> and a suggested_action field
(including reason/confidence as provided), or alternatively add a transformation
when building MatchData that extracts participant names from interested_members
into member_a/member_b before saving; make this change for the MatchData
definition and every usage site (e.g., the code that creates match records in
opportunity-matcher.reaction and the related construction around lines where
MatchData is used) so stored records contain the actual participant names and
suggested_action data.
Comment on lines
+685
to
+709
| if (row.verb === "create") { | ||
| const created = await ctx.client.createWatcher({ | ||
| slug: w.slug, | ||
| agentId: w.agent, | ||
| name: w.name, | ||
| description: w.description, | ||
| prompt: w.prompt, | ||
| extraction_schema: w.extractionSchema, | ||
| schedule: w.schedule, | ||
| sources: w.sources, | ||
| reactions_guidance: w.reactionsGuidance, | ||
| device_worker_id: w.deviceWorkerId, | ||
| scheduler_client_id: w.schedulerClientId, | ||
| notification_channel: w.notificationChannel, | ||
| notification_priority: w.notificationPriority, | ||
| min_cooldown_seconds: w.minCooldownSeconds, | ||
| tags: w.tags, | ||
| agent_kind: w.agentKind, | ||
| json_template: w.jsonTemplate, | ||
| keying_config: w.keyingConfig, | ||
| classifiers: w.classifiers, | ||
| condensation_prompt: w.condensationPrompt, | ||
| condensation_window_count: w.condensationWindowCount, | ||
| }); | ||
| watcherId = created.watcher_id; |
There was a problem hiding this comment.
Fail fast when a created watcher has no watcher_id.
This path silently skips setReactionScript() when the create response omits watcher_id. That leaves a newly created watcher without its declared reaction script while the apply still looks successful.
Suggested fix
const created = await ctx.client.createWatcher({
slug: w.slug,
agentId: w.agent,
@@
condensation_prompt: w.condensationPrompt,
condensation_window_count: w.condensationWindowCount,
});
watcherId = created.watcher_id;
+ if (w.reactionScript && !watcherId) {
+ throw new ApiError(
+ `create watcher "${w.slug}" failed: response is missing watcher_id, so reaction_script cannot be uploaded`
+ );
+ }
@@
- if (w.reactionScript && watcherId) {
- await ctx.client.setReactionScript(
- watcherId,
- w.reactionScript.sourceCode
- );
+ if (w.reactionScript) {
+ if (!watcherId) {
+ throw new ApiError(
+ `apply watcher "${w.slug}" failed: watcher_id is required to upload reaction_script`
+ );
+ }
+ await ctx.client.setReactionScript(
+ watcherId,
+ w.reactionScript.sourceCode
+ );
}Also applies to: 792-798
🤖 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/cli/src/commands/_lib/apply/apply-cmd.ts` around lines 685 - 709,
The create path using ctx.client.createWatcher may return a response without
created.watcher_id and the code currently continues, skipping setReactionScript;
modify the create flow to validate that created.watcher_id exists immediately
after the createWatcher call (the watcherId assignment), and if it's missing
throw or return a clear error so the apply fails fast; apply the same validation
to the equivalent update response handling (the code path that mirrors create,
e.g., where the update API returns watcher_id and setReactionScript is invoked)
so you never proceed to setReactionScript or other post-create/update steps when
watcher_id is absent.
Comment on lines
+632
to
+633
| ...(payload.schedule ? { schedule: payload.schedule } : {}), | ||
| ...(payload.sources?.length ? { sources: payload.sources } : {}), |
There was a problem hiding this comment.
Preserve explicit empty sources on watcher create.
Here [] is not equivalent to omission: omitting sources lets the server inject default queries, while sources: [] means “create this watcher with no sources”. This spread collapses those two states.
Suggested fix
- ...(payload.sources?.length ? { sources: payload.sources } : {}),
+ ...(payload.sources !== undefined ? { sources: payload.sources } : {}),📝 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
| ...(payload.schedule ? { schedule: payload.schedule } : {}), | |
| ...(payload.sources?.length ? { sources: payload.sources } : {}), | |
| ...(payload.schedule ? { schedule: payload.schedule } : {}), | |
| ...(payload.sources !== undefined ? { sources: payload.sources } : {}), |
🤖 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/cli/src/commands/_lib/apply/client.ts` around lines 632 - 633, The
spread currently uses payload.sources?.length to decide inclusion which treats
an explicit empty array ([]) the same as undefined; change the condition to
check for undefined instead so explicit [] is preserved (e.g. replace
...(payload.sources?.length ? { sources: payload.sources } : {}) with a guard
that uses payload.sources !== undefined ? { sources: payload.sources } : {}),
keeping the rest of the payload construction intact and ensuring the `payload`
object's `sources` property is passed when present even if empty.
Comment on lines
+468
to
+479
| if ( | ||
| desired.deviceWorkerId !== undefined && | ||
| desired.deviceWorkerId !== (remote.device_worker_id ?? undefined) | ||
| ) { | ||
| scalar.push("device_worker_id"); | ||
| } | ||
| if ( | ||
| desired.schedulerClientId !== undefined && | ||
| desired.schedulerClientId !== (remote.scheduler_client_id ?? undefined) | ||
| ) { | ||
| scalar.push("scheduler_client_id"); | ||
| } |
There was a problem hiding this comment.
Diff nullable watcher scalars symmetrically.
Removing device_worker_id, scheduler_client_id, or agent_kind from YAML currently produces noop, so apply can never unpin/unset those fields. executePlan() already clears them with null; the diff just never emits the update row.
Suggested fix
- if (
- desired.deviceWorkerId !== undefined &&
- desired.deviceWorkerId !== (remote.device_worker_id ?? undefined)
- ) {
+ if ((desired.deviceWorkerId ?? null) !== (remote.device_worker_id ?? null)) {
scalar.push("device_worker_id");
}
if (
- desired.schedulerClientId !== undefined &&
- desired.schedulerClientId !== (remote.scheduler_client_id ?? undefined)
+ (desired.schedulerClientId ?? null) !==
+ (remote.scheduler_client_id ?? null)
) {
scalar.push("scheduler_client_id");
}
@@
- if (
- desired.agentKind !== undefined &&
- desired.agentKind !== (remote.agent_kind ?? undefined)
- ) {
+ if ((desired.agentKind ?? null) !== (remote.agent_kind ?? null)) {
scalar.push("agent_kind");
}Also applies to: 504-509
🤖 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/cli/src/commands/_lib/apply/diff.ts` around lines 468 - 479, The
diff logic treats undefined and null asymmetrically so removing watcher scalars
becomes a noop; change the checks to compare nullable values directly (e.g.
replace the two-step guard that uses desired.deviceWorkerId !== undefined and
then compares to remote.device_worker_id with a single comparison like
(desired.deviceWorkerId ?? undefined) !== (remote.device_worker_id ??
undefined)), and apply the same change for desired.schedulerClientId vs
remote.scheduler_client_id and agentKind vs remote.agent_kind so null/unset in
YAML will correctly emit the update row.
Addresses pi review findings on PR #829: - reaction_script path: must be relative POSIX (./foo.ts), no `..` segments, must end in `.ts`, must resolve under the YAML's directory, ≤256 KiB. Prevents a hostile YAML from pulling in /etc/passwd or escaping via ../.. - export: when a local reactions/<slug>.reaction.ts already exists and --force isn't set, drop the YAML's reaction_script reference + print a loud warning. Previously, re-applying could upload stale local code instead of the server's actual script. - export: defensive slug sanitization before using the watcher slug as a filesystem path component (regex-gated basename). - Tests: 3 new path-validation cases (absolute, .., non-.ts) + 1 export case asserting the skipped-file → no-reference behavior. Pi findings NOT addressed here (tracked as follow-ups): - Clearing optional fields via `reaction_script: null` / `tags: null` / `device_worker_id: null` etc. — current behavior is "omitted = leave alone", changing this is additive but bigger than this fix. - Apply-executor integration test covering mixed scalar + version-bound + reaction-script drift end-to-end.
Member
Author
Pi review responsePi flagged 6 issues. Addressed 3 in commit 3b84cc1 (path traversal hardening, export skip-emit guard, slug sanitization). Three deferred to follow-ups: Addressed
Deferred (tracked for follow-up)
E2E re-run after fixes: all 12 examples still apply cleanly, reaction_script_compiled non-NULL, export round-trip clean. |
bun.lock still listed packages/web in dependencies (the pre-rename name), which made `bun install --frozen-lockfile` fail in CI. Re-running bun install removed the stale entry. Pure lockfile hygiene; no dep version changes.
Integration's table-schema drift test was failing because the existing device_worker_id / agent_kind / notification_channel / notification_priority / min_cooldown_seconds / last_fired_at columns on `watchers` weren't listed in QUERYABLE_SCHEMA — predates this branch, surfaced by CI on a fresh DB. Adds them so the polyglot SQL validator allows queries to reference them. No DB change. Note: the other 2 integration test failures on this PR (public-pages- contract.test.ts) are pre-existing on `main` per gh run list and unrelated to this branch.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts (1)
34-34: ⚡ Quick winRemove the unused
_initparameter from the fetch stub signature.It isn’t used in the callback, so keep the signature minimal instead of underscore-prefixing.
As per coding guidelines, "When fixing unused-parameter errors, delete the parameter rather than prefixing with
_".🤖 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/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts` at line 34, The fetch stub uses an unused parameter named _init in its async callback signature; remove that parameter altogether so the signature is (input: RequestInfo | URL) => Promise<Response> (i.e., change "async (input: RequestInfo | URL, _init?: RequestInit)" to "async (input: RequestInfo | URL)"), updating the fetch stub declaration used in the test so there are no unused parameters left.
🤖 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/cli/src/commands/_lib/apply/desired-state.ts`:
- Around line 878-894: Replace the lexical containment check for the watcher
`reaction_script` with a real-path based check: call realpathSync on the model
directory (derived from modelFileAbsPath/baseDir) and on the resolved script
path (abs), then use path.relative(realBase, realAbs) to ensure the script does
not escape (relative must not start with '..' and realAbs must not equal
realBase incorrectly); perform this containment check before calling
readFileSync and, if you still need to read the target, read the real path
(realAbs) so symlinks can't bypass validation; update the error messages to
reference raw.slug, rel and the real resolved path as needed.
In `@packages/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts`:
- Around line 259-305: Run the code formatter over the updated test block to
satisfy Biome linting: reformat the changed lines around the test that creates
mkTempDir(), writes the local script file, sets up buildFetch(...) responses,
calls exportCommand(...), and asserts file contents and parsed YAML; ensure
whitespace, trailing commas, and line breaks conform to project Biome settings
(or run the project's formatting script/command) so the test file
packages/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts is properly
formatted.
---
Nitpick comments:
In `@packages/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts`:
- Line 34: The fetch stub uses an unused parameter named _init in its async
callback signature; remove that parameter altogether so the signature is (input:
RequestInfo | URL) => Promise<Response> (i.e., change "async (input: RequestInfo
| URL, _init?: RequestInit)" to "async (input: RequestInfo | URL)"), updating
the fetch stub declaration used in the test so there are no unused parameters
left.
🪄 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: 06c22c1d-a098-43a5-959d-9f0b7237094e
📒 Files selected for processing (4)
packages/cli/src/commands/_lib/apply/__tests__/desired-state-extra.test.tspackages/cli/src/commands/_lib/apply/desired-state.tspackages/cli/src/commands/_lib/export/__tests__/export-cmd.test.tspackages/cli/src/commands/_lib/export/export-cmd.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/src/commands/_lib/export/export-cmd.ts
Comment on lines
+878
to
+894
| const baseDir = resolve(modelFileAbsPath, ".."); | ||
| const abs = resolve(baseDir, rel); | ||
| // Belt-and-braces — symlinks or unusual relative-path forms shouldn't | ||
| // escape the baseDir even if the above checks let one through. | ||
| if (!abs.startsWith(`${baseDir}/`) && abs !== baseDir) { | ||
| throw new ValidationError( | ||
| `watcher "${raw.slug}" \`reaction_script\` resolves outside the model directory (${abs})` | ||
| ); | ||
| } | ||
| let sourceCode: string; | ||
| try { | ||
| sourceCode = readFileSync(abs, "utf-8"); | ||
| } catch { | ||
| throw new ValidationError( | ||
| `watcher "${raw.slug}" \`reaction_script\` ${rel} does not exist (resolved to ${abs})` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and read the relevant section
git ls-files packages/cli/src/commands/_lib/apply/ | head -20Repository: lobu-ai/lobu
Length of output: 900
🏁 Script executed:
# Read the file to see the actual implementation
wc -l packages/cli/src/commands/_lib/apply/desired-state.tsRepository: lobu-ai/lobu
Length of output: 115
🏁 Script executed:
# Get the relevant section around lines 878-894
sed -n '870,910p' packages/cli/src/commands/_lib/apply/desired-state.tsRepository: lobu-ai/lobu
Length of output: 1762
🏁 Script executed:
# Also check the imports at the top of the file
head -50 packages/cli/src/commands/_lib/apply/desired-state.tsRepository: lobu-ai/lobu
Length of output: 1992
🏁 Script executed:
# Search for any tests related to reaction_script or this validation
fd -e test.ts -e spec.ts packages/cli | xargs grep -l "reaction_script" 2>/dev/null || echo "No test files found"Repository: lobu-ai/lobu
Length of output: 448
🏁 Script executed:
# Check if there are any existing symlink-related tests or handling in the codebase
rg "realpathSync|readlink" packages/cli --type ts -B 2 -A 2 | head -40Repository: lobu-ai/lobu
Length of output: 38
🏁 Script executed:
# Check the desired-state test file
wc -l packages/cli/src/commands/_lib/apply/__tests__/desired-state.test.tsRepository: lobu-ai/lobu
Length of output: 129
🏁 Script executed:
# Search for reaction_script handling in tests
rg "reaction_script" packages/cli/src/commands/_lib/apply/__tests__/ -B 2 -A 5Repository: lobu-ai/lobu
Length of output: 10681
🏁 Script executed:
# Look at the extra tests too
head -100 packages/cli/src/commands/_lib/apply/__tests__/desired-state-extra.test.tsRepository: lobu-ai/lobu
Length of output: 3105
🏁 Script executed:
# Check if there are any symlink-related tests or mocking in the broader codebase
rg "symlink|readlink|realpath" packages/ --type ts | head -20Repository: lobu-ai/lobu
Length of output: 2468
🏁 Script executed:
# Let's verify Windows path behavior by examining how path.resolve works with backslashes
python3 << 'EOF'
# Simulate Windows path.resolve behavior
import os
# Windows paths use backslashes
windows_base_dir = "C:\\Users\\test\\models"
windows_abs = "C:\\Users\\test\\models\\foo.ts"
# Check if forward slash matching works on Windows-style paths
check_with_forward_slash = windows_abs.startswith(f"{windows_base_dir}/")
check_with_backslash = windows_abs.startswith(f"{windows_base_dir}\\")
print(f"Windows path abs: {windows_abs}")
print(f"Windows path baseDir: {windows_base_dir}")
print(f"Check with forward slash (/) - would FAIL: {check_with_forward_slash}")
print(f"Check with backslash (\\) - would PASS: {check_with_backslash}")
print()
print("VULNERABILITY CONFIRMED: On Windows, resolve() returns backslashes,")
print("but the code checks for forward slashes, so valid paths would be rejected.")
EOFRepository: lobu-ai/lobu
Length of output: 382
🏁 Script executed:
# Now verify symlink behavior - check if readFileSync follows symlinks
cat > /tmp/test_symlink.js << 'EOF'
const { existsSync, readFileSync, mkdirSync, writeFileSync } = require('fs');
const { join, resolve } = require('path');
const { symlink } = require('fs').promises;
(async () => {
// Create test directories
const testDir = '/tmp/symlink_test';
const modelsDir = join(testDir, 'models');
const outsideDir = join(testDir, 'outside');
// Clean up if exists
try { require('child_process').execSync(`rm -rf ${testDir}`); } catch {}
mkdirSync(modelsDir, { recursive: true });
mkdirSync(outsideDir, { recursive: true });
// Create a file outside models directory
const outsideFile = join(outsideDir, 'secret.ts');
writeFileSync(outsideFile, 'console.log("OUTSIDE")');
// Create a symlink inside models pointing outside
const symlinkPath = join(modelsDir, 'link.ts');
await symlink(outsideFile, symlinkPath);
// Now test the vulnerable check
const modelFileAbsPath = join(modelsDir, 'model.yaml');
const baseDir = resolve(modelFileAbsPath, '..');
const rel = 'link.ts';
const abs = resolve(baseDir, rel);
console.log('Base dir:', baseDir);
console.log('Resolved path:', abs);
console.log('Outside file:', outsideFile);
// Check would pass (symlink is under models)
const checkPasses = !abs.startsWith(`${baseDir}/`) && abs !== baseDir ? false : true;
console.log('Check passes:', checkPasses);
// But readFileSync follows the symlink and reads the outside file
try {
const content = readFileSync(abs, 'utf-8');
console.log('readFileSync succeeded, content:', content);
console.log('\nVULNERABILITY CONFIRMED: readFileSync followed symlink to outside file!');
} catch (e) {
console.log('Error:', e.message);
}
})();
EOF
node /tmp/test_symlink.jsRepository: lobu-ai/lobu
Length of output: 337
🏁 Script executed:
# Check how agent-worker handles symlink containment - they seem to have fixed this already
sed -n '1,100p' packages/agent-worker/src/shared/tool-implementations.ts | grep -A 30 "Containment check"Repository: lobu-ai/lobu
Length of output: 38
🏁 Script executed:
# Get more context from the agent-worker solution
rg -A 15 "workspaceReal.*realpath" packages/agent-worker/src/shared/tool-implementations.tsRepository: lobu-ai/lobu
Length of output: 633
🏁 Script executed:
# Verify that the suggested fix uses relative() correctly on Windows
python3 << 'EOF'
import os
from pathlib import Path
# Test Windows path scenario with the suggested fix logic
windows_base_dir = r"C:\Users\test\models"
windows_abs = r"C:\Users\test\models\foo.ts"
# Simulate the fix: using relative() to check if path is outside baseDir
# relative(base, target) returns '..' if target is outside base
rel_path = os.path.relpath(windows_abs, windows_base_dir)
print(f"Base: {windows_base_dir}")
print(f"Abs: {windows_abs}")
print(f"relative(base, abs): {rel_path}")
print(f"Starts with '..': {rel_path.startswith('..')}")
print()
# Test escaping attempt
escape_abs = r"C:\Users\secret.txt"
rel_escape = os.path.relpath(escape_abs, windows_base_dir)
print(f"Escape attempt abs: {escape_abs}")
print(f"relative(base, escape): {rel_escape}")
print(f"Starts with '..': {rel_escape.startswith('..')}")
print()
print("VERIFIED: relative() + '..' check works on Windows!")
EOFRepository: lobu-ai/lobu
Length of output: 355
Use real paths for reaction_script containment.
The path check at line 881–883 is lexical only. On Windows, resolve() returns backslashes, so the forward-slash check abs.startsWith(${baseDir}/) fails silently for all valid child paths. Additionally, readFileSync(abs) follows symlinks, allowing a symlink under models/ to point outside the tree and bypass containment entirely.
Replace the lexical check with real-path resolution using realpathSync() and validate containment via relative(), which correctly handles both symlinks and Windows paths. The agent-worker codebase already uses this pattern successfully (see packages/agent-worker/src/shared/tool-implementations.ts).
Suggested fix
-import { existsSync, readdirSync, readFileSync } from "node:fs";
+import { existsSync, readdirSync, readFileSync, realpathSync } from "node:fs";
import { readdir, readFile, stat } from "node:fs/promises";
-import { join, resolve } from "node:path";
+import { isAbsolute, join, relative, resolve } from "node:path";
...
- const baseDir = resolve(modelFileAbsPath, "..");
- const abs = resolve(baseDir, rel);
- // Belt-and-braces — symlinks or unusual relative-path forms shouldn't
- // escape the baseDir even if the above checks let one through.
- if (!abs.startsWith(`${baseDir}/`) && abs !== baseDir) {
+ const baseDir = realpathSync(resolve(modelFileAbsPath, ".."));
+ const abs = resolve(baseDir, rel);
+ let realAbs: string;
+ try {
+ realAbs = realpathSync(abs);
+ } catch {
+ throw new ValidationError(
+ `watcher "${raw.slug}" \`reaction_script\` ${rel} does not exist (resolved to ${abs})`
+ );
+ }
+ const relToBase = relative(baseDir, realAbs);
+ if (isAbsolute(relToBase) || relToBase.startsWith("..")) {
throw new ValidationError(
- `watcher "${raw.slug}" \`reaction_script\` resolves outside the model directory (${abs})`
+ `watcher "${raw.slug}" \`reaction_script\` resolves outside the model directory (${realAbs})`
);
}
let sourceCode: string;
try {
- sourceCode = readFileSync(abs, "utf-8");
+ sourceCode = readFileSync(realAbs, "utf-8");
} catch {
throw new ValidationError(
- `watcher "${raw.slug}" \`reaction_script\` ${rel} does not exist (resolved to ${abs})`
+ `watcher "${raw.slug}" \`reaction_script\` ${rel} could not be read (resolved to ${realAbs})`
);
}
...
- out.reactionScript = { sourcePath: abs, sourceCode };
+ out.reactionScript = { sourcePath: realAbs, sourceCode };🤖 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/cli/src/commands/_lib/apply/desired-state.ts` around lines 878 -
894, Replace the lexical containment check for the watcher `reaction_script`
with a real-path based check: call realpathSync on the model directory (derived
from modelFileAbsPath/baseDir) and on the resolved script path (abs), then use
path.relative(realBase, realAbs) to ensure the script does not escape (relative
must not start with '..' and realAbs must not equal realBase incorrectly);
perform this containment check before calling readFileSync and, if you still
need to read the target, read the real path (realAbs) so symlinks can't bypass
validation; update the error messages to reference raw.slug, rel and the real
resolved path as needed.
Comment on lines
+259
to
+305
| const out = mkTempDir(); | ||
| await mkdir(join(out, "models", "reactions"), { recursive: true }); | ||
| const localScript = "// stale local version\nexport default async () => {};\n"; | ||
| writeFileSync( | ||
| join(out, "models", "reactions", "with-reaction.reaction.ts"), | ||
| localScript, | ||
| ); | ||
|
|
||
| const fetchImpl = buildFetch({ | ||
| manage_entity_schema: () => ({ | ||
| entity_types: [], | ||
| relationship_types: [], | ||
| }), | ||
| "watchers?watcher_id": () => ({ | ||
| watcher: { | ||
| reaction_script: "export default async () => 'NEW SERVER VERSION';\n", | ||
| description: null, | ||
| }, | ||
| }), | ||
| "watchers?include_details": () => ({ | ||
| watchers: [ | ||
| { | ||
| slug: "with-reaction", | ||
| watcher_id: "42", | ||
| agent_id: "triage", | ||
| prompt: "Work.", | ||
| extraction_schema: { type: "object" }, | ||
| }, | ||
| ], | ||
| }), | ||
| }); | ||
|
|
||
| await exportCommand({ cwd: out, out, fetchImpl, only: "models" }); | ||
|
|
||
| // Local script is untouched. | ||
| expect( | ||
| readFileSync( | ||
| join(out, "models", "reactions", "with-reaction.reaction.ts"), | ||
| "utf-8", | ||
| ), | ||
| ).toBe(localScript); | ||
|
|
||
| // YAML does NOT reference reaction_script. | ||
| const bundle = parseYaml( | ||
| readFileSync(join(out, "models", "exported.yaml"), "utf-8"), | ||
| ) as { watchers: Array<Record<string, unknown>> }; | ||
| expect(bundle.watchers[0]?.reaction_script).toBeUndefined(); |
There was a problem hiding this comment.
Run formatter for this changed block to fix CI.
This range is currently failing Biome format-lint and should be auto-formatted before merge.
🧰 Tools
🪛 GitHub Actions: CI / format-lint
[error] 259-305: Biome format check failed. Formatter would have reformatted string assignment and function call arguments (e.g., line breaks inside const localScript and readFileSync calls).
🤖 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/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts` around
lines 259 - 305, Run the code formatter over the updated test block to satisfy
Biome linting: reformat the changed lines around the test that creates
mkTempDir(), writes the local script file, sets up buildFetch(...) responses,
calls exportCommand(...), and asserts file contents and parsed YAML; ensure
whitespace, trailing commas, and line breaks conform to project Biome settings
(or run the project's formatting script/command) so the test file
packages/cli/src/commands/_lib/export/__tests__/export-cmd.test.ts is properly
formatted.
4 tasks
buremba
added a commit
that referenced
this pull request
May 18, 2026
…#828) * feat(connectors): browser.evaluate / fill_form / page_text + subdir layout Adds three Chrome-extension connectors (browser.evaluate, browser.fill_form, browser.page_text) whose executors live in the Owletto for Chrome extension. Definitions sit under `packages/connectors/src/browser/` so primitive groupings are structurally distinct from third-party service connectors. - packages/connectors/src/browser/{evaluate,fill_form,page_text}.ts (new) - packages/connectors/src/index.ts: re-export browser/* - packages/connector-worker/src/compile-connector.ts: resolve dotted keys via subdir (`browser/evaluate.ts`) in addition to the existing underscore-flat convention (`chrome_tabs.ts`). - packages/server/src/utils/connector-catalog.ts: scan one level deep so browser/* is discovered; preserve relative `source_path` so resolvers don't collide on basename. Submodule (packages/owletto) is left at main's pin (aeb3324) — the browser.evaluate executor already shipped via #825/#159, so no bump is needed by this PR. * chore(format): biome format pre-existing diff.test.ts drift Pre-existing format drift introduced in #829 was failing main's format-lint CI for several commits. Bundling the auto-fix here so PR #828's format-lint check turns green on top of an already-red main. * fix(connectors): subdir-aware resolvers + userManaged on browser primitives Pi review found four follow-ups to the browser/* subdir layout introduced in the previous commit: - packages/server/src/utils/connector-catalog.ts: server-side findBundledConnectorFile() was still flat-only — auto-install / device-reconcile / worker-poll would treat browser.evaluate as "no bundled source." Now mirrors the worker-side resolver (subdir first, underscore-flat fallback). Adds bundledConnectorSourcePath(filePath) so subdir paths round-trip through connectorSourcePathToUri. - packages/server/src/utils/ensure-connector-installed.ts + packages/server/src/worker-api/device-reconcile.ts: stop persisting basename(filePath) as source_path — collides on basename across subdirs and breaks source_uri resolution for subdir connectors. - packages/cli/src/commands/_lib/connector-loader.ts: CLI resolver was also flat-only. - packages/connectors/src/browser/{evaluate,fill_form,page_text}.ts: mark feeds userManaged so device-reconcile doesn't auto-wire them with config=NULL. These are bridge-composing primitives (script / url + fields / url are required, gateway-author-supplied), not end-user feeds. * fix(mcp,device-reconcile,owletto): inherited main failures + pi follow-up - packages/server/src/workspace/multi-tenant.ts: when a Bearer header is present but PAT verify, OAuth verify, AND session-cookie lookup all fail, return RFC 6750 `invalid_token` 401 (with WWW-Authenticate error=invalid_token) instead of falling through to anonymous and returning generic `unauthorized` later. Fixes mcp/auth.test.ts "should reject expired/invalid OAuth access token" — they assert the standards-compliant error code so MCP clients (Claude Desktop etc.) surface "bad token" rather than mistaking it for "no auth needed." - packages/server/src/worker-api/device-reconcile.ts: short-circuit ensureDeviceConnectorWired() when declaredFeedKeys is empty (every feed userManaged → nothing to auto-wire). Avoids a compile + upsert + no-auth-connection adopt per Chrome poll for browser.* primitives. Definition + version row still get installed lazily by ensureConnectorInstalled when a composing connector runs them. (Second-round pi review finding.) - packages/owletto: bump pointer to 2552ed0 — fix(build): vite target=esnext for top-level await in main.tsx (lobu-ai/owletto#161, merged). Unblocks PR Validation / build-test which had been failing on packages/owletto's vite build since the auth-pivot landed. * fix(server): repair sibling-walk SPA template/dist paths post-rename The packages/web → packages/owletto rename in #817 updated APP_ROOT-relative candidates but missed two: - The `../web/{dist,index.html}` sibling-deploy candidate was kept verbatim ("for out-of-monorepo deployments"), but after the rename the sibling dir is `../owletto/`, not `../web/`. The stale path silently misses on every lookup. - The `path.resolve(process.cwd(), '../packages/owletto/...')` candidate has always resolved to `packages/packages/owletto/...` (double `packages`) when cwd is `packages/server` — i.e. exactly the layout the integration job runs under. Rewriting as `../owletto/...` lands in the right sibling. Symptoms: every `public-pages-contract.test.ts` (and `mcp/auth.test.ts` indirectly) failure on `main` since 7a72456 — `buildPublicPageModel` returned a real model but `loadAnySpaHtmlTemplate()` returned null, so the catch-all fell through to the JSON discovery response with `Cache-Control: no-store`. Reproduces against PGlite locally; fixed and verified all 3 public-pages tests pass. Same broken pattern repaired in `utils/public-origin.ts:hasLocalFrontend` (8 candidates, 4 corrected). * fix(device-reconcile,catalog): pi review pass-3 follow-ups Third pi review flagged two issues with the prior round: - worker-api/device-reconcile.ts: the earlier early-return-when-empty short-circuited too aggressively. `local.directory` and other device connectors whose only feed is `userManaged` rely on ensureDeviceConnectorWired to install the connector_definition + version + connection — `/api/workers/me/feeds` 404s on "no connection wired" otherwise. Move the short-circuit into the fast-path check instead: when the connection + version already exist AND there's nothing to verify (declaredFeedKeys empty), fast-path returns without compiling. First poll still does the full install; subsequent polls are zero work. - utils/connector-catalog.ts: the one-level subdir scan was descending into `connectors/src/__tests__/` and trying to extract catalog metadata from test files that import `bun:test`, producing esbuild warnings on every cold scan. Skip `__tests__` and any leading-underscore dir.
This was referenced May 22, 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
Extends
lobu applyto cover every admin-only watcher field onmanage_watchers(previously web-UI only) and adds a newlobu exportsubcommand that round-trips server state back into apply-compatible YAML + sibling.tsfiles. All 12 example projects updated to demonstrate the new surface end-to-end.Watcher fields now apply-able
.tsreaction scripts (reaction_script: ./*.reaction.ts) — matches connector-definition file-reference pattern, pushed verbatim toset_reaction_script.device_worker_id(UUID),scheduler_client_id,notification_channel(canvas/notification/both),notification_priority(low/normal/high),min_cooldown_seconds,tags,agent_kind.reactions_guidance,json_template,keying_config,classifiers,condensation_prompt,condensation_window_count— routed throughcreate_versionso they don't clobber the version chain.update, version-bound drift throughcreate_version+ auto-bump, reaction-script throughset_reaction_script(push-always since write-only).device_worker_idalso wired through CLI create/update + diff.lobu exportNew subcommand pulls memory schema + connectors and writes apply-compatible files (
models/exported.yaml, siblingmodels/reactions/<slug>.reaction.ts,connectors/exported.yaml). Credentials never exported; operator warned to re-add$ENVrefs.--forceto overwrite,--only models|connectorsto scope.Examples demonstrating the new surface
Every project with a watcher (11/12) updated with at least
tags+notification_priority; six picked forreaction_scriptsiblings that callclient.knowledge.save()to persist downstream-relevant events.atlasgets a syntheticcatalog-staleness-checkerwatcher since it had none.Server fix
manage_watchers.handleListSELECT was missingnotification_channel / notification_priority / min_cooldown_seconds / agent_kindcolumns and returningtagsas Postgres array literal"{a,b}"instead of a JSON array. Without this,lobu exportand apply's drift detection couldn't see the new fields on round-trip.Test plan
make typecheck— cleanmake build-packages— cleanbun test packages/cli/src/commands/_lib/— 131 pass, including 23 new tests (9 watcher parser, 2 connection device, 5 diff variants, 4 export round-trip, 3 updated existing)/tmp/e2e-examples.sh):funnel-digest|high|both|3600|2|t|t,inbound-triage|normal|canvas|300|2|t|t(notif priority + channel + cooldown + tag-count + reaction_script + reaction_script_compiled all set)lunch-finalize|high|canvas|600|2,lunch-open|high|both|600|2reaction_script_compilednon-NULLlobu exportround-trip writes sibling .ts files; re-apply shows onlyreaction_scriptas drift (idempotent push-always; design)--dry-runcleanlyOut of scope (deferred)
verboseLogging,pluginsConfig)organization) issue — skipped from apply-for-real subset--no-verify)Summary by CodeRabbit
New Features
lobu exportCLI to export models/connectors and optionally write sibling reaction script files.Improvements