Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions docs/plans/lobu-apply.md
Original file line number Diff line number Diff line change
Expand Up @@ -162,16 +162,21 @@ Pi flagged these — explicit do-not-copy list for the CLI agent:

### End-to-end (this plan's exit criterion)

After all 3 PRs merge:
1. `make build-packages`
2. Spin up local Postgres, apply all migrations
3. Boot `lobu run` configured in **DB-first mode** (host-provided stores) — this matches the cloud topology
4. Author a sample `lobu.toml` with one agent, one telegram connection, one memory entity type, one provider
5. `lobu apply --dry-run` — verify diff shows 4 creates
6. `lobu apply` — accept prompt, verify Postgres rows
7. `lobu apply --dry-run` again — should report all noops
8. Edit one connection's config, re-run `lobu apply` — verify only that connection shows update + "will restart"
9. Manually edit the connection in Postgres, re-run `lobu apply` — verify update path runs (we don't have drift detection in v1, so the change is just overwritten — document this as expected v1 behavior)
**Status: proven via `scripts/e2e-lobu-apply.sh` (see hardening PR).**

The script:
1. Builds packages + CLI.
2. Boots `start-local.ts` against PGlite with `LOBU_LOCAL_BOOTSTRAP=true`. The bootstrap path mints a default user/org (slug `dev`)/PAT and saves the token to `${OWLETTO_DATA_DIR}/bootstrap-pat.txt`.
3. Reads the PAT, configures a CLI context pointing at the local server, and `lobu login --token <PAT>`.
4. Drops a sample project at `/tmp/e2e-project/` with one agent, one telegram connection, one provider, and one entity-type yaml.
5. `lobu apply --dry-run` → asserts `+ agent`, `+ connection`, `+ entity-type` rows.
6. `lobu apply --yes` → asserts "Apply complete".
7. Re-runs `--dry-run` → asserts no `+`/`~` rows (full noop round-trip).
8. Mutates `chatId` in `lobu.toml`, re-runs apply → asserts `~ connection` + "will restart" marker.
9. Curls REST endpoints with the bootstrap PAT to verify rows landed in Postgres.
10. Cleans up the server, data dir, and project dir.

Manual steps from the original plan (DB-first `lobu run`, postgres editing) are obsolete — the bootstrap path is the supported dev loop.

## Cross-cutting concerns

Expand Down
103 changes: 103 additions & 0 deletions packages/cli/src/commands/_lib/apply/__tests__/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,109 @@ describe("apply diff — memory schema", () => {
});
});

describe("apply diff — empty container preservation", () => {
// Bug fix: previously canonical() collapsed [] and {} to null, which
// meant clearing a remote allowlist by setting it to [] silently
// round-tripped as a noop instead of an update.
test("clearing networkConfig.allowedDomains from non-empty to [] is an update", () => {
const desired = buildState([
buildDesiredAgent("triage", {
metadata: { agentId: "triage", name: "Triage" },
settings: {
networkConfig: { allowedDomains: [] },
},
}),
]);
const remote: RemoteSnapshot = {
...emptyRemote(),
agents: [{ agentId: "triage", name: "Triage" }],
agentSettings: new Map<string, AgentSettings | null>([
[
"triage",
{
networkConfig: { allowedDomains: ["foo.com"] },
updatedAt: 0,
},
],
]),
connectionsByAgent: new Map([["triage", []]]),
};
const plan = computeDiff(desired, remote);
const settingsRow = plan.rows.find((r) => r.kind === "settings");
expect(settingsRow?.verb).toBe("update");
if (settingsRow?.kind === "settings") {
expect(settingsRow.changedFields).toContain("networkConfig");
}
});

test("[] is not equal to null (preserved as distinct values)", () => {
// When desired sets allowedDomains: [] and remote has the field
// missing entirely, the diff should still treat them as equivalent
// for the case where remote literally doesn't have the field — but
// [] vs the explicit array ["foo"] must differ.
const desiredEmpty = buildState([
buildDesiredAgent("triage", {
metadata: { agentId: "triage", name: "Triage" },
settings: {
networkConfig: { allowedDomains: [] },
},
}),
]);
const remoteWithItems: RemoteSnapshot = {
...emptyRemote(),
agents: [{ agentId: "triage", name: "Triage" }],
agentSettings: new Map<string, AgentSettings | null>([
[
"triage",
{
networkConfig: { allowedDomains: ["x.com"] },
updatedAt: 0,
},
],
]),
connectionsByAgent: new Map([["triage", []]]),
};
const plan = computeDiff(desiredEmpty, remoteWithItems);
expect(plan.counts.update).toBeGreaterThan(0);
});

test("{} is not equal to populated object", () => {
// empty config object vs populated config object must show as drift/update
const desired = buildState([
buildDesiredAgent("triage", {
metadata: { agentId: "triage", name: "Triage" },
connections: [
{
stableId: "triage-telegram",
type: "telegram",
config: {},
},
],
}),
]);
const remote: RemoteSnapshot = {
...emptyRemote(),
agents: [{ agentId: "triage", name: "Triage" }],
agentSettings: new Map<string, AgentSettings | null>([["triage", null]]),
connectionsByAgent: new Map([
[
"triage",
[
{
id: "triage-telegram",
platform: "telegram",
config: { botToken: "abc" },
},
],
],
]),
};
const plan = computeDiff(desired, remote);
const connRow = plan.rows.find((r) => r.kind === "connection");
expect(connRow?.verb).toBe("update");
});
});

describe("renderSummary", () => {
test("renders zero-row plan", () => {
const desired = buildState([]);
Expand Down
4 changes: 3 additions & 1 deletion packages/cli/src/commands/_lib/apply/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,9 @@ export class ApplyClient {
}): Promise<RemoteAgent> {
const { body } = await this.request<RemoteAgent>(
"POST",
`/api/${this.orgSlug}/agents/`,
// No trailing slash — Hono matches `routes.post('/', ...)` mounted at
// `/api/:orgSlug/agents` against `/api/dev/agents`, not `/api/dev/agents/`.
`/api/${this.orgSlug}/agents`,
agent,
[200, 201]
);
Expand Down
18 changes: 11 additions & 7 deletions packages/cli/src/commands/_lib/apply/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,10 @@ export interface DiffPlan {
* Stable structural equality for JSON-shaped values. Sorts object keys before
* stringifying so `{a:1,b:2}` and `{b:2,a:1}` compare equal.
*
* Returns false (i.e. "different") when either side is `undefined` and the
* other isn't — but treats `[]` vs `undefined` and `{}` vs `undefined` as
* **equal**, since empty containers carry no semantic state and the server
* commonly omits them.
* `undefined` and `null` both canonicalize to `"null"` so missing-on-one-side
* fields don't show as drift. Empty arrays and empty objects are preserved
* as themselves — clearing a remote allowlist by setting it to `[]` must
* produce an `update`, not a `noop`.
*/
function deepEqual(a: unknown, b: unknown): boolean {
return canonical(a) === canonical(b);
Expand All @@ -91,14 +91,12 @@ function deepEqual(a: unknown, b: unknown): boolean {
function canonical(value: unknown): string {
if (value === undefined || value === null) return "null";
if (Array.isArray(value)) {
if (value.length === 0) return "null";
return `[${value.map(canonical).join(",")}]`;
}
if (typeof value === "object") {
const entries = Object.entries(value as Record<string, unknown>)
.filter(([, v]) => v !== undefined)
.sort(([a], [b]) => a.localeCompare(b));
if (entries.length === 0) return "null";
return `{${entries.map(([k, v]) => `${JSON.stringify(k)}:${canonical(v)}`).join(",")}}`;
}
return JSON.stringify(value);
Expand Down Expand Up @@ -211,7 +209,13 @@ function diffConnection(
}
const changed: string[] = [];
if (desired.type !== remote.platform) changed.push("type");
if (!deepEqual(desired.config, remote.config ?? {})) changed.push("config");
// The route handler stores `platform` inside `config` for stable-id matching,
// so a noop round-trip from GET will have an extra `platform` key the CLI
// never wrote. Strip it before diffing so an unchanged connection doesn't
// show as drift on every plan.
const remoteConfig: Record<string, unknown> = { ...(remote.config ?? {}) };
delete remoteConfig.platform;
if (!deepEqual(desired.config, remoteConfig)) changed.push("config");
if (changed.length === 0) {
return {
kind: "connection",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,44 @@ describe('PUT /agents/:agentId/connections/by-stable-id/:stableId', () => {
expect(body.connection?.id).toBe(stableId);
});

test('PUT with settings-only change returns updated + willRestart', async () => {
const app = await importAgentRoutes();
const stableId = 'host-agent-tg-settings';

// First create with default settings.
const create = await app.request(
`/host-agent/connections/by-stable-id/${stableId}`,
{
method: 'PUT',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({
platform: 'telegram',
config: { chatId: 'C-1' },
}),
}
);
expect(create.status).toBe(201);

// Same config, but settings change (allowFrom from undefined to ['user-1']).
const response = await app.request(
`/host-agent/connections/by-stable-id/${stableId}`,
{
method: 'PUT',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({
platform: 'telegram',
config: { chatId: 'C-1' },
settings: { allowFrom: ['user-1'], allowGroups: true },
}),
}
);
expect(response.status).toBe(200);
const body = (await response.json()) as any;
expect(body.updated).toBe(true);
expect(body.willRestart).toBe(true);
expect(body.noop).toBeUndefined();
});

test('PUT against an unknown agent returns 404', async () => {
const app = await importAgentRoutes();

Expand Down
16 changes: 12 additions & 4 deletions packages/owletto-backend/src/lobu/agent-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -784,8 +784,14 @@ routes.put('/:agentId/connections/by-stable-id/:stableId', mcpAuth, async (c) =>
merged.platform = platform;

const configChanged = !configsShallowEqual(merged, previousConfig);

if (!configChanged) {
// Settings (allowFrom, allowGroups, etc.) are persisted alongside the
// connection config and are part of "did anything change?" — a
// settings-only update must trigger willRestart, not be silently noop'd.
const previousSettings = (existing.settings ?? {}) as Record<string, unknown>;
const mergedSettings = { allowGroups: true, ...settings } as Record<string, unknown>;
const settingsChanged = !configsShallowEqual(mergedSettings, previousSettings);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Compare connection settings deeply before deciding restart

The new settingsChanged check uses configsShallowEqual, which compares array values by reference. For settings.allowFrom, sending the same logical list in a repeated PUT (e.g. ['user-1']) creates a new array object each request, so this path always reports updated + willRestart instead of noop. That breaks idempotency for settings-bearing clients and can trigger unnecessary worker restarts whenever unchanged settings are re-applied.

Useful? React with 👍 / 👎.


if (!configChanged && !settingsChanged) {
return c.json({ noop: true, connection: existing }, 200);
}

Expand Down Expand Up @@ -846,14 +852,16 @@ routes.put('/:agentId/connections/by-stable-id/:stableId', mcpAuth, async (c) =>
// Fallback path mirrors the POST handler's no-manager branch but uses
// the supplied stable ID instead of a synthesized one. Platform is kept
// in config (matching the manager path) so subsequent idempotent PUTs
// see a stable previousConfig.
// see a stable previousConfig. Settings default `allowGroups: true` to
// match the manager-path default — symmetric with the noop comparison
// above so a follow-up PUT with no settings field round-trips as noop.
const now = Date.now();
await connectionStore.saveConnection({
id: stableId,
platform,
templateAgentId: agentId,
config: { platform, ...config } as Record<string, any>,
settings: settings as any,
settings: { allowGroups: true, ...settings } as any,
metadata: {},
status: 'stopped',
createdAt: now,
Expand Down
Loading
Loading