feat(cli): flatten Lobu context config#955
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates context storage from ChangesContext Configuration Schema Migration and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Comment |
|
Codecov Report❌ Patch coverage is 📢 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/dev.ts (1)
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShared-DB safety gate is bypassed when shell overrides a pinned
.envURL.Line 92 gives shell env precedence, but Line 102 only checks whether
.envhas anyDATABASE_URL. If.envhas a local URL and shell has a shared/prod URL,mergedEnv.DATABASE_URLis shared and this guard is skipped.Proposed fix
const mergedEnv = { ...userServerEnv, ...envVars, ...(process.env as Record<string, string>), }; const hasDatabaseUrl = Boolean(mergedEnv.DATABASE_URL?.trim()); + const projectDatabaseUrl = envVars.DATABASE_URL?.trim(); + const effectiveDatabaseUrl = mergedEnv.DATABASE_URL?.trim(); + const projectPinsEffectiveDatabaseUrl = + Boolean(projectDatabaseUrl) && projectDatabaseUrl === effectiveDatabaseUrl; if ( hasDatabaseUrl && - !envVars.DATABASE_URL?.trim() && - isSharedDatabaseUrl(mergedEnv.DATABASE_URL!) && + !projectPinsEffectiveDatabaseUrl && + isSharedDatabaseUrl(effectiveDatabaseUrl!) && !options.unsafeSharedDb ) {Also applies to: 100-104
🤖 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/dev.ts` around lines 89 - 93, The mergedEnv construction gives shell envs precedence so the shared-DB safety check (which reads mergedEnv.DATABASE_URL) can be bypassed when a pinned .env has a local URL but the shell overrides it; change the guard to derive the DATABASE_URL from the pinned/env-file sources instead of mergedEnv by computing a pinnedDbUrl = userServerEnv.DATABASE_URL ?? envVars.DATABASE_URL (or equivalent lookup of the .env-supplied value) and use pinnedDbUrl for the shared-DB safety gate checks (also update the other similar check at the second occurrence around lines 100-104) so shell overrides do not bypass the protection.
🧹 Nitpick comments (1)
packages/cli/src/index.ts (1)
532-573: 💤 Low valueDeprecation notice for
--api-urlshould be communicated to users.The
--api-urloption is marked as a deprecated alias in the code, but there's no runtime warning when users use it. Consider emitting a deprecation warning to stderr whenoptions.apiUrlis used instead ofoptions.url.📝 Optional: Add deprecation warning
async ( name: string, options: { url?: string; apiUrl?: string; cwd?: string; lifecycle?: "managed" | "external"; } ) => { const url = options.url ?? options.apiUrl; + if (options.apiUrl && !options.url) { + console.error(chalk.yellow(" Warning: --api-url is deprecated; use --url instead.")); + } if (!url) { throw new Error("Missing required option --url <url>"); }🤖 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/index.ts` around lines 532 - 573, Add a runtime deprecation warning when the deprecated alias is used: in the action handler for the "add <name>" command (the async function that reads options and assigns const url = options.url ?? options.apiUrl), detect when options.apiUrl is present and options.url is not, and emit a stderr deprecation message (e.g., console.warn or process.stderr.write) advising users to use --url instead; keep behavior unchanged (still prefer options.url when both provided) and include the option name (--api-url) and suggested replacement (--url) in the message.
🤖 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/owletto`:
- Line 1: The submodule pointer for packages/owletto is set to a SHA that isn't
an ancestor of the remote branch owletto/main, causing the drift guard to fail;
update the submodule ref so it's reachable from owletto/main by either (a)
merging the pinned commit into the remote branch owletto/main and then updating
the submodule pointer, or (b) changing the packages/owletto submodule SHA to a
commit that is an ancestor of owletto/main (use git fetch on the owletto remote,
list reachable commits, and set the submodule to a valid SHA), then commit the
updated submodule entry so CI's hard drift check passes.
---
Outside diff comments:
In `@packages/cli/src/commands/dev.ts`:
- Around line 89-93: The mergedEnv construction gives shell envs precedence so
the shared-DB safety check (which reads mergedEnv.DATABASE_URL) can be bypassed
when a pinned .env has a local URL but the shell overrides it; change the guard
to derive the DATABASE_URL from the pinned/env-file sources instead of mergedEnv
by computing a pinnedDbUrl = userServerEnv.DATABASE_URL ?? envVars.DATABASE_URL
(or equivalent lookup of the .env-supplied value) and use pinnedDbUrl for the
shared-DB safety gate checks (also update the other similar check at the second
occurrence around lines 100-104) so shell overrides do not bypass the
protection.
---
Nitpick comments:
In `@packages/cli/src/index.ts`:
- Around line 532-573: Add a runtime deprecation warning when the deprecated
alias is used: in the action handler for the "add <name>" command (the async
function that reads options and assigns const url = options.url ??
options.apiUrl), detect when options.apiUrl is present and options.url is not,
and emit a stderr deprecation message (e.g., console.warn or
process.stderr.write) advising users to use --url instead; keep behavior
unchanged (still prefer options.url when both provided) and include the option
name (--api-url) and suggested replacement (--url) in the message.
🪄 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: bd276425-03d5-40ec-9731-0381269fb049
📒 Files selected for processing (10)
packages/cli/src/commands/context.tspackages/cli/src/commands/dev.tspackages/cli/src/index.tspackages/cli/src/internal/__tests__/context.test.tspackages/cli/src/internal/context.tspackages/owlettopackages/server/src/start-local.tspackages/server/src/utils/__tests__/user-config.test.tspackages/server/src/utils/user-config.tsscripts/task-setup.sh
💤 Files with no reviewable changes (1)
- scripts/task-setup.sh
buremba
force-pushed
the
feat/simplify-context-config
branch
2 times, most recently
from
13444c6 to
29aa6ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/context.ts`:
- Line 40: The current-context output prints context.apiUrl which is
inconsistent with the flattened schema and can be undefined; update the console
output to use the normalized context.url value instead (replace uses of
context.apiUrl in the current-context display with context.url) so the command
shows the correct URL after migration and across Lines referencing context
(e.g., where current-context prints `URL:`).
In `@packages/cli/src/commands/dev.ts`:
- Around line 94-105: The guard currently only checks that envVars.DATABASE_URL
exists, which lets a shell-provided shared DATABASE_URL in mergedEnv bypass the
refusal if the project .env contains any DATABASE_URL (even a different one).
Update the condition around hasDatabaseUrl / isSharedDatabaseUrl to treat a
shell override as unsafe unless the project .env explicitly pins the same value:
replace the !envVars.DATABASE_URL?.trim() check with a comparison that requires
envVars.DATABASE_URL?.trim() === mergedEnv.DATABASE_URL?.trim() (i.e., refuse
when envVars.DATABASE_URL is absent OR present but not equal to
mergedEnv.DATABASE_URL), keeping references to hasDatabaseUrl,
mergedEnv.DATABASE_URL, envVars.DATABASE_URL, isSharedDatabaseUrl and
options.unsafeSharedDb to locate the logic to change.
In `@packages/cli/src/internal/context.ts`:
- Around line 203-205: The code saves entry.cwd even when lifecycle is absent,
which later causes contexts without an explicit "managed" lifecycle to be
dropped; to fix this either (1) materialize the inferred lifecycle before
persisting (call normalizeLifecycle(server?.lifecycle) and set entry.lifecycle
to the resolved value — e.g., lifecycle || inferredLifecycle — before assigning
entry.cwd) or (2) enforce a guard that only assigns entry.cwd when the resolved
lifecycle equals "managed" (check the normalized lifecycle value before setting
entry.cwd). Apply the same change to the other places mentioned (the blocks
around the code paths referenced by lines 341-343 and 357-365) so cwd is only
persisted when the final lifecycle is managed or the lifecycle is explicitly
materialized first, and ensure getServerConfig callers see consistent behavior.
- Around line 281-285: The code currently accepts any non-empty
raw.url/raw.apiUrl and constructs a LobuContextEntry using
normalizeApiUrl(rawUrl) without handling malformed results; update
loadContextConfig() so that after computing rawUrl and calling
normalizeApiUrl(rawUrl) you check the normalization result and drop the entry
(return undefined or skip it) when normalizeApiUrl returns a falsy/invalid
value, and apply the same validation in resolveContext() where entries are
re-normalized (ensure you validate the output of normalizeApiUrl before using
it); reference symbols: rawUrl, normalizeApiUrl, loadContextConfig,
resolveContext, LobuContextEntry, normalizeString.
- Around line 377-385: The code reads parsed.port only, so URLs without explicit
ports (e.g., http://host/path or https://host/path) leave out.port undefined;
update the logic in the block handling context.url (the parsed, out, and
context.url variables) to infer and set the implicit default port when
parsed.port is empty by checking parsed.protocol ('http:' -> 80, 'https:' ->
443), then validate the derived port (Number.isInteger and 1..65535) and assign
it to out.port; keep existing hostname handling for out.host.
In `@packages/server/src/utils/user-config.ts`:
- Around line 83-90: The code assigns parsed.hostname directly to out.host,
which leaves IPv6 literals wrapped in brackets (e.g., "[::1]") and later causes
httpServer.listen to fail; update the logic around parsed.hostname in the rawUrl
handling (where parsed is created from new URL(rawUrl) and out.host is set) to
strip leading "[" and trailing "]" if present (only for bracketed IPv6 literals)
before assigning to out.host so applyUserServerConfigToEnv()/process.env.HOST
and subsequent httpServer.listen receive an unbracketed IPv6 address.
🪄 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: 728f85c1-ad03-4648-a69b-a74c6a0ac4b8
📒 Files selected for processing (12)
packages/cli/src/commands/context.tspackages/cli/src/commands/dev.tspackages/cli/src/index.tspackages/cli/src/internal/__tests__/context.test.tspackages/cli/src/internal/context.tspackages/landing/src/content/docs/guides/testing.mdpackages/landing/src/content/docs/reference/cli.mdpackages/owlettopackages/server/src/start-local.tspackages/server/src/utils/__tests__/user-config.test.tspackages/server/src/utils/user-config.tsscripts/task-setup.sh
💤 Files with no reviewable changes (1)
- scripts/task-setup.sh
✅ Files skipped from review due to trivial changes (2)
- packages/landing/src/content/docs/guides/testing.md
- packages/server/src/start-local.ts
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/cli/src/internal/context.ts (3)
377-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInfer default port for
http/httpswhen URL omits an explicit port.Line 379 only reads
parsed.port, sohttp://host/...andhttps://host/...produce no derivedporteven though it is implicit.Proposed fix
- const port = parsed.port ? Number.parseInt(parsed.port, 10) : undefined; + const port = parsed.port + ? Number.parseInt(parsed.port, 10) + : parsed.protocol === "http:" + ? 80 + : parsed.protocol === "https:" + ? 443 + : undefined;🤖 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/internal/context.ts` around lines 377 - 381, When parsing context.url in the try block that creates `parsed` and sets `out.port`, infer the default port when `parsed.port` is empty by checking `parsed.protocol` (use 80 for "http:" and 443 for "https:") before validation; compute port = parsed.port ? Number.parseInt(parsed.port, 10) : (parsed.protocol === "https:" ? 443 : parsed.protocol === "http:" ? 80 : undefined), then run the same Number.isInteger/0<port<=65535 validation and assign to `out.port` if valid (preserve existing behavior for non-http(s) schemes).
281-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate migrated URLs before keeping a stored context.
Line 284 still accepts malformed
url/apiUrlduring normalization, so hand-edited/bad entries survive load and can later break resolution paths.Proposed fix
- const entry: LobuContextEntry = { url: normalizeApiUrl(rawUrl) }; + let normalizedUrl: string; + try { + normalizedUrl = normalizeAndValidateApiUrl(rawUrl); + } catch { + return undefined; + } + const entry: LobuContextEntry = { url: normalizedUrl };🤖 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/internal/context.ts` around lines 281 - 285, The code accepts a rawUrl and blindly stores a normalized value into LobuContextEntry even if the input was malformed; update the loading logic around rawUrl/normalizeApiUrl so you validate the normalized result before keeping the entry: after computing rawUrl and calling normalizeApiUrl(rawUrl), verify the normalized URL is a valid API URL (non-empty and passes whatever URL validation function the project uses) and only construct/return the LobuContextEntry when that validation succeeds; otherwise skip or discard the entry (and avoid returning undefined prematurely). Also ensure activeOrg still uses normalizeString(raw.activeOrg) only for validated entries so the stored context is guaranteed to contain a valid url and org.
203-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent persisting
cwdwhen lifecycle won’t resolve to managed.Lines 203-205 and 357-365 still allow
cwdwithout an effective managed lifecycle, which makescwda silent no-op forgetServerConfig(). Migration logic at Lines 289-292 carries the same inconsistency forward.Proposed fix
const lifecycle = normalizeLifecycle(server?.lifecycle); - if (lifecycle) entry.lifecycle = lifecycle; - if (server?.cwd?.trim()) entry.cwd = server.cwd.trim(); + const cwd = server?.cwd?.trim(); + if (cwd && lifecycle === "external") { + throw new Error("`cwd` can only be set for managed contexts."); + } + if (cwd) { + entry.cwd = cwd; + entry.lifecycle = lifecycle ?? "managed"; + } else if (lifecycle) { + entry.lifecycle = lifecycle; + }- const lifecycle = normalizeLifecycle(server.lifecycle); - if (lifecycle) context.lifecycle = lifecycle; - else delete context.lifecycle; - if (server.cwd?.trim()) context.cwd = server.cwd.trim(); - else delete context.cwd; + const lifecycle = normalizeLifecycle(server.lifecycle); + const cwd = server.cwd?.trim(); + if (cwd && lifecycle === "external") { + throw new Error("`cwd` can only be set for managed contexts."); + } + if (cwd) { + context.cwd = cwd; + context.lifecycle = lifecycle ?? "managed"; + } else { + if (lifecycle) context.lifecycle = lifecycle; + else delete context.lifecycle; + delete context.cwd; + }- const lifecycle = normalizeLifecycle(raw.lifecycle) ?? legacyServer.lifecycle; - if (lifecycle) entry.lifecycle = lifecycle; - const cwd = normalizeString(raw.cwd) ?? legacyServer.cwd; - if (cwd) entry.cwd = cwd; + const lifecycle = normalizeLifecycle(raw.lifecycle) ?? legacyServer.lifecycle; + const cwd = normalizeString(raw.cwd) ?? legacyServer.cwd; + const effectiveLifecycle = cwd ? (lifecycle ?? "managed") : lifecycle; + if (effectiveLifecycle) entry.lifecycle = effectiveLifecycle; + if (cwd && effectiveLifecycle === "managed") entry.cwd = cwd;Also applies to: 289-292, 357-365
🤖 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/internal/context.ts` around lines 203 - 205, The code sets entry.cwd whenever server?.cwd is present even if the normalized lifecycle isn't a managed lifecycle, making cwd a no-op; update each spot where normalizeLifecycle(server?.lifecycle) is used (e.g., the block that sets entry.lifecycle and entry.cwd, and the migration logic and the other identical block) to only assign entry.cwd = server.cwd.trim() when the resolved lifecycle indicates a managed lifecycle (i.e., check the normalized lifecycle value for the managed state before setting cwd), leaving cwd unset otherwise. Ensure you change all three occurrences that currently set cwd unconditionally after normalizeLifecycle.
🧹 Nitpick comments (2)
packages/cli/src/internal/__tests__/context.test.ts (2)
76-89: ⚡ Quick winAssert the flattened field in the normalized URL test.
This test still validates
matched?.apiUrl(Line 88), which keeps the legacy contract alive in a migration-focused suite. Prefer assertingmatched?.url(or assert both if backward compatibility is explicitly required).Suggested update
- expect(matched?.apiUrl).toBe("https://custom.lobu.ai/api/v1"); + expect(matched?.url).toBe("https://custom.lobu.ai/api/v1");🤖 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/internal/__tests__/context.test.ts` around lines 76 - 89, The test for findContextByUrl is asserting the legacy field matched?.apiUrl; update the assertion to check the normalized field matched?.url instead (or assert both matched?.url and matched?.apiUrl only if backward compatibility must be preserved) so the test validates the new contract for the returned context object from findContextByUrl.
145-185: ⚡ Quick winAdd a malformed managed URL test case.
Managed server settings are URL-derived; add one negative test for
lifecycle: "managed"+ invalidurlto lock parse-failure behavior and prevent regressions in normalization/derivation paths.🤖 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/internal/__tests__/context.test.ts` around lines 145 - 185, Add a new test in packages/cli/src/internal/__tests__/context.test.ts that verifies a managed context with a malformed url does not produce derived server settings: create configData with contexts.local { url: "not-a-valid-url", lifecycle: "managed" }, mock readFileSpy.mockResolvedValue(JSON.stringify(configData)), then assert await getServerConfig("local") returns undefined (same behavior as external contexts); reference getServerConfig to locate the logic and use the existing test patterns for mocking readFileSpy and expectation.
🤖 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.
Duplicate comments:
In `@packages/cli/src/internal/context.ts`:
- Around line 377-381: When parsing context.url in the try block that creates
`parsed` and sets `out.port`, infer the default port when `parsed.port` is empty
by checking `parsed.protocol` (use 80 for "http:" and 443 for "https:") before
validation; compute port = parsed.port ? Number.parseInt(parsed.port, 10) :
(parsed.protocol === "https:" ? 443 : parsed.protocol === "http:" ? 80 :
undefined), then run the same Number.isInteger/0<port<=65535 validation and
assign to `out.port` if valid (preserve existing behavior for non-http(s)
schemes).
- Around line 281-285: The code accepts a rawUrl and blindly stores a normalized
value into LobuContextEntry even if the input was malformed; update the loading
logic around rawUrl/normalizeApiUrl so you validate the normalized result before
keeping the entry: after computing rawUrl and calling normalizeApiUrl(rawUrl),
verify the normalized URL is a valid API URL (non-empty and passes whatever URL
validation function the project uses) and only construct/return the
LobuContextEntry when that validation succeeds; otherwise skip or discard the
entry (and avoid returning undefined prematurely). Also ensure activeOrg still
uses normalizeString(raw.activeOrg) only for validated entries so the stored
context is guaranteed to contain a valid url and org.
- Around line 203-205: The code sets entry.cwd whenever server?.cwd is present
even if the normalized lifecycle isn't a managed lifecycle, making cwd a no-op;
update each spot where normalizeLifecycle(server?.lifecycle) is used (e.g., the
block that sets entry.lifecycle and entry.cwd, and the migration logic and the
other identical block) to only assign entry.cwd = server.cwd.trim() when the
resolved lifecycle indicates a managed lifecycle (i.e., check the normalized
lifecycle value for the managed state before setting cwd), leaving cwd unset
otherwise. Ensure you change all three occurrences that currently set cwd
unconditionally after normalizeLifecycle.
---
Nitpick comments:
In `@packages/cli/src/internal/__tests__/context.test.ts`:
- Around line 76-89: The test for findContextByUrl is asserting the legacy field
matched?.apiUrl; update the assertion to check the normalized field matched?.url
instead (or assert both matched?.url and matched?.apiUrl only if backward
compatibility must be preserved) so the test validates the new contract for the
returned context object from findContextByUrl.
- Around line 145-185: Add a new test in
packages/cli/src/internal/__tests__/context.test.ts that verifies a managed
context with a malformed url does not produce derived server settings: create
configData with contexts.local { url: "not-a-valid-url", lifecycle: "managed" },
mock readFileSpy.mockResolvedValue(JSON.stringify(configData)), then assert
await getServerConfig("local") returns undefined (same behavior as external
contexts); reference getServerConfig to locate the logic and use the existing
test patterns for mocking readFileSpy and expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89a540e1-d565-4b78-b610-5b607ceed6e5
📒 Files selected for processing (13)
packages/cli/src/commands/context.tspackages/cli/src/commands/dev.tspackages/cli/src/index.tspackages/cli/src/internal/__tests__/context.test.tspackages/cli/src/internal/context.tspackages/landing/src/content/docs/guides/testing.mdpackages/landing/src/content/docs/reference/cli.mdpackages/owlettopackages/server/src/start-local.tspackages/server/src/utils/__tests__/user-config.test.tspackages/server/src/utils/user-config.tsscripts/e2e-lobu-apply.shscripts/task-setup.sh
✅ Files skipped from review due to trivial changes (3)
- packages/landing/src/content/docs/reference/cli.md
- packages/server/src/start-local.ts
- packages/landing/src/content/docs/guides/testing.md
…owletto to reachable main
The context-config flatten changed loadContextConfig()'s normalized
return shape from { apiUrl } to flat { url }. Two test fixtures still
mocked loadContextConfig with the old { apiUrl } shape, so context.url
came back undefined and normalizeApiUrl(undefined) threw inside
findContextByUrl — failing 6 applyCommand dry-run / org-resolution
tests. Update the mocks to the normalized flat { url } contract that
loadContextConfig actually returns.
Also reset the owletto submodule pointer from the unreachable SHA
2c9c4c9 to owletto/main HEAD 2a2cc35, which is reachable and safe for
production cloning.
buremba
force-pushed
the
feat/simplify-context-config
branch
from
49f4348 to
290c465
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/cli/src/commands/context.ts (1)
40-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse normalized
context.urlin current-context output.Line 40 still reads
context.apiUrl, which is inconsistent with the new flat schema and with Lines 19/82. This can surfaceundefinedafter migration paths that persist onlyurl.🔧 Proposed fix
- console.log(chalk.dim(` URL: ${context.apiUrl}`)); + console.log(chalk.dim(` URL: ${context.url}`));🤖 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/context.ts` at line 40, The current-context output still references context.apiUrl which is outdated; update the console.log call that prints the URL to use context.url instead of context.apiUrl so it matches the new flat schema and the other usages (e.g., the earlier lines that reference context.url) and avoids printing undefined after migrations.packages/server/src/utils/user-config.ts (1)
90-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip IPv6 brackets before exporting host.
Line 90 forwards
parsed.hostnameas-is. For IPv6 URLs this can remain bracketed and break downstream host binding when set viaHOST.Proposed fix
- if (parsed.hostname) out.host = parsed.hostname; + if (parsed.hostname) { + out.host = parsed.hostname.replace(/^\[|\]$/g, ""); + }🤖 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/utils/user-config.ts` at line 90, When exporting the host in user-config.ts the code assigns parsed.hostname directly to out.host (the line with parsed.hostname), which leaves IPv6 addresses bracketed and can break downstream binding; update the assignment so that before setting out.host you strip surrounding '[' and ']' from parsed.hostname (e.g., trim leading '[' and trailing ']' if present) so both IPv4/hostname and IPv6 addresses are exported unbracketed; modify the logic around the out.host assignment where parsed.hostname is used to perform this normalization.packages/cli/src/internal/context.ts (3)
203-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
cwdbehind managed lifecycle to avoid silent no-op config.Line 205 and Line 364 persist
cwdeven when lifecycle is absent/non-managed, but Line 341 ignores those contexts ingetServerConfig(). Reject this combination (or materialize lifecycle) at write time.Proposed fix
const lifecycle = normalizeLifecycle(server?.lifecycle); - if (lifecycle) entry.lifecycle = lifecycle; - if (server?.cwd?.trim()) entry.cwd = server.cwd.trim(); + const cwd = server?.cwd?.trim(); + if (cwd && lifecycle !== "managed") { + throw new Error("`cwd` can only be set for managed contexts."); + } + if (lifecycle) entry.lifecycle = lifecycle; + if (cwd) entry.cwd = cwd; if (!server) { delete context.lifecycle; delete context.cwd; } else { const lifecycle = normalizeLifecycle(server.lifecycle); + const cwd = server.cwd?.trim(); + if (cwd && lifecycle !== "managed") { + throw new Error("`cwd` can only be set for managed contexts."); + } if (lifecycle) context.lifecycle = lifecycle; else delete context.lifecycle; - if (server.cwd?.trim()) context.cwd = server.cwd.trim(); + if (cwd) context.cwd = cwd; else delete context.cwd; }Also applies to: 357-365
🤖 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/internal/context.ts` around lines 203 - 205, The code currently sets entry.cwd from server.cwd regardless of lifecycle; because getServerConfig ignores contexts without a managed lifecycle, persistently storing cwd with no lifecycle becomes a silent no-op—update the write-time logic (the block using normalizeLifecycle and assigning entry.lifecycle and entry.cwd) to only set entry.cwd when the computed lifecycle indicates a managed lifecycle (i.e., when normalizeLifecycle(server?.lifecycle) returns a managed value), otherwise either refuse/save an error (reject the write) or also materialize/set a default managed lifecycle before assigning entry.cwd; specifically change the logic around normalizeLifecycle, lifecycle, entry.cwd and server.cwd so cwd is guarded by the managed lifecycle invariant used by getServerConfig.
379-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInfer implicit default ports when deriving managed host/port.
Line 379 only reads explicit ports. For
http://host/...orhttps://host/..., inferred managed config misses an implied port and can drift fromcontext.url.Proposed fix
- const port = parsed.port ? Number.parseInt(parsed.port, 10) : undefined; + const port = parsed.port + ? Number.parseInt(parsed.port, 10) + : parsed.protocol === "http:" + ? 80 + : parsed.protocol === "https:" + ? 443 + : undefined;🤖 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/internal/context.ts` around lines 379 - 381, When deriving the managed host/port, if parsed.port is absent infer the standard port from the URL scheme before assigning out.port: if parsed.protocol (or parsed.scheme) is "http:" => 80, "https:" => 443; then validate with Number.isInteger and range check (1-65535) and assign to out.port the inferred value when valid. Update the logic around parsed.port/port and out.port in context.ts so that the code uses the parsed.protocol to set the implicit default port for http/https when no explicit port is provided.
284-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDrop malformed stored URLs during normalization.
Line 284 normalizes but does not validate persisted
url/apiUrl, so malformed values can still be loaded and later resolved.Proposed fix
- const entry: LobuContextEntry = { url: normalizeApiUrl(rawUrl) }; + let normalizedUrl: string; + try { + normalizedUrl = normalizeAndValidateApiUrl(rawUrl); + } catch { + return undefined; + } + const entry: LobuContextEntry = { url: normalizedUrl };🤖 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/internal/context.ts` around lines 284 - 285, The code creates a LobuContextEntry with url: normalizeApiUrl(rawUrl) but does not validate the normalized URL, so malformed stored URLs are kept; update the logic around normalizeApiUrl (used when constructing the entry) to validate the returned value (e.g., ensure normalizeApiUrl(rawUrl) returns a well-formed URL/string or passes URL parsing) and if it is invalid/empty, drop that stored entry (do not push the LobuContextEntry) or set url to undefined and skip later resolution; adjust the surrounding code that builds entries (where entry is constructed) to filter out invalid urls rather than blindly using normalizeApiUrl(rawUrl).
🧹 Nitpick comments (1)
packages/cli/src/internal/__tests__/context.test.ts (1)
187-229: ⚡ Quick winAdd regression tests for invalid persisted shapes (
cwd/URL).Please add cases that (1) reject/ignore
cwdwithout managed lifecycle and (2) drop malformed persistedurl/apiUrlduring normalization. These are high-value guardrails for this migration path.Also applies to: 270-287
🤖 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/internal/__tests__/context.test.ts` around lines 187 - 229, Add two regression tests for addContext: one that verifies addContext rejects or strips a persisted cwd when lifecycle is not "managed" (ensure addContext("name", url) does not accept saved cwd unless lifecycle === "managed" and that writeFileSpy shows no cwd saved), and another that ensures malformed persisted url/apiUrl values are dropped during normalization (call addContext on a context with a bad url/apiUrl and assert the saved context contains a normalized/omitted url field as expected). Reference addContext and DEFAULT_CONTEXT_NAME in the tests and reuse readFileSpy/writeFileSpy patterns from existing tests to mock persisted shapes and assert the resulting saved JSON shape.
🤖 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/utils/user-config.ts`:
- Around line 86-88: The current derivation only sets out.port when parsed.port
exists, causing scheme-only URLs to lose implied ports; update the logic around
parsed.port/out.port to apply defaults: if parsed.port is present parse and
validate it as now, otherwise if parsed.protocol (or parsed.scheme) indicates
'http:' set out.port = 80 and if 'https:' set out.port = 443, keeping the
existing Number.isInteger and range checks so the code still validates ports
before assigning to out.port.
---
Duplicate comments:
In `@packages/cli/src/commands/context.ts`:
- Line 40: The current-context output still references context.apiUrl which is
outdated; update the console.log call that prints the URL to use context.url
instead of context.apiUrl so it matches the new flat schema and the other usages
(e.g., the earlier lines that reference context.url) and avoids printing
undefined after migrations.
In `@packages/cli/src/internal/context.ts`:
- Around line 203-205: The code currently sets entry.cwd from server.cwd
regardless of lifecycle; because getServerConfig ignores contexts without a
managed lifecycle, persistently storing cwd with no lifecycle becomes a silent
no-op—update the write-time logic (the block using normalizeLifecycle and
assigning entry.lifecycle and entry.cwd) to only set entry.cwd when the computed
lifecycle indicates a managed lifecycle (i.e., when
normalizeLifecycle(server?.lifecycle) returns a managed value), otherwise either
refuse/save an error (reject the write) or also materialize/set a default
managed lifecycle before assigning entry.cwd; specifically change the logic
around normalizeLifecycle, lifecycle, entry.cwd and server.cwd so cwd is guarded
by the managed lifecycle invariant used by getServerConfig.
- Around line 379-381: When deriving the managed host/port, if parsed.port is
absent infer the standard port from the URL scheme before assigning out.port: if
parsed.protocol (or parsed.scheme) is "http:" => 80, "https:" => 443; then
validate with Number.isInteger and range check (1-65535) and assign to out.port
the inferred value when valid. Update the logic around parsed.port/port and
out.port in context.ts so that the code uses the parsed.protocol to set the
implicit default port for http/https when no explicit port is provided.
- Around line 284-285: The code creates a LobuContextEntry with url:
normalizeApiUrl(rawUrl) but does not validate the normalized URL, so malformed
stored URLs are kept; update the logic around normalizeApiUrl (used when
constructing the entry) to validate the returned value (e.g., ensure
normalizeApiUrl(rawUrl) returns a well-formed URL/string or passes URL parsing)
and if it is invalid/empty, drop that stored entry (do not push the
LobuContextEntry) or set url to undefined and skip later resolution; adjust the
surrounding code that builds entries (where entry is constructed) to filter out
invalid urls rather than blindly using normalizeApiUrl(rawUrl).
In `@packages/server/src/utils/user-config.ts`:
- Line 90: When exporting the host in user-config.ts the code assigns
parsed.hostname directly to out.host (the line with parsed.hostname), which
leaves IPv6 addresses bracketed and can break downstream binding; update the
assignment so that before setting out.host you strip surrounding '[' and ']'
from parsed.hostname (e.g., trim leading '[' and trailing ']' if present) so
both IPv4/hostname and IPv6 addresses are exported unbracketed; modify the logic
around the out.host assignment where parsed.hostname is used to perform this
normalization.
---
Nitpick comments:
In `@packages/cli/src/internal/__tests__/context.test.ts`:
- Around line 187-229: Add two regression tests for addContext: one that
verifies addContext rejects or strips a persisted cwd when lifecycle is not
"managed" (ensure addContext("name", url) does not accept saved cwd unless
lifecycle === "managed" and that writeFileSpy shows no cwd saved), and another
that ensures malformed persisted url/apiUrl values are dropped during
normalization (call addContext on a context with a bad url/apiUrl and assert the
saved context contains a normalized/omitted url field as expected). Reference
addContext and DEFAULT_CONTEXT_NAME in the tests and reuse
readFileSpy/writeFileSpy patterns from existing tests to mock persisted shapes
and assert the resulting saved JSON shape.
🪄 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: 61602705-b843-419d-9477-af577abe43b2
📒 Files selected for processing (14)
packages/cli/src/commands/_lib/apply/__tests__/apply-cmd-dryrun.test.tspackages/cli/src/commands/context.tspackages/cli/src/commands/dev.tspackages/cli/src/index.tspackages/cli/src/internal/__tests__/context.test.tspackages/cli/src/internal/__tests__/org-resolution.test.tspackages/cli/src/internal/context.tspackages/landing/src/content/docs/guides/testing.mdpackages/landing/src/content/docs/reference/cli.mdpackages/server/src/start-local.tspackages/server/src/utils/__tests__/user-config.test.tspackages/server/src/utils/user-config.tsscripts/e2e-lobu-apply.shscripts/task-setup.sh
✅ Files skipped from review due to trivial changes (5)
- scripts/e2e-lobu-apply.sh
- scripts/task-setup.sh
- packages/landing/src/content/docs/guides/testing.md
- packages/landing/src/content/docs/reference/cli.md
- packages/server/src/start-local.ts
…alization, managed-cwd invariant - dev: evaluate the EFFECTIVE merged DATABASE_URL in the shared-DB refusal (shell precedence over .env), not just project-.env presence; extract shouldRefuseSharedDatabaseUrl() and unit-test the shell-override footgun - context: rename ResolvedContext.apiUrl -> url for flat-schema consistency; fix `lobu context current` to print context.url - context/user-config: derive default port 80/443 for scheme-only URLs and strip IPv6 brackets from the derived host in both managed-config paths - context: reject `cwd` on non-managed contexts (was a silent no-op) in addContext + setServerConfig - context: drop malformed stored URLs during normalization so resolveContext always returns a usable endpoint - tests: regression coverage for every behavior above
buremba
added a commit
that referenced
this pull request
May 20, 2026
Resolves conflicts from main's context-config flatten (#955) + shared-DB guard refactor against this branch's embedded-PG work: - cli/index.ts: context add adopts main's flat --url/--cwd/--lifecycle (drops the nested --port/--host/--database-url/--data-dir options). - cli/commands/dev.ts: keep embedded helpers (isExternalDatabaseUrl, resolveEmbeddedDataRoot, mode selection) + adopt main's shouldRefuseSharedDatabaseUrl guard; drop dead BackendBundleKind (pglite removed) and the userServerConfig.databaseUrl/dataDir block (flattened away). - server/utils/user-config.ts: drop the dataDir→DATABASE_URL mapping (dataDir no longer in the flattened config schema). - server/start-local.ts: stays deleted (consolidated into server.ts). - owletto: bumped to merged 872a6ea (Keychain key + main's flat-context). Verified: cli + server tsc clean, dev.test.ts 12/12, init still writes DATABASE_URL to .env (embedded-vs-external feature intact).
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
url,lifecycle, and optionalcwdfieldsapiUrl+server.lifecycle/cwdonly for migration into the flat shapeserver.portlobu context add --urlValidation
claude -p "Review the current git diff..."to check for legacy writers / breakagebun test packages/cli/src/internal/__tests__/context.test.ts packages/server/src/utils/__tests__/user-config.test.tsmake build-packagescd packages/landing && bun run buildcd packages/owletto/apps/mac && xcodebuild -project Owletto.xcodeproj -scheme Owletto -configuration Debug -destination "platform=macOS" build CODE_SIGNING_ALLOWED=NOSummary by CodeRabbit
Refactor
lobu context addto use--urlinstead of--api-urlfor specifying context base URLs.--port,--host,--database-url,--data-dir) from context commands.Documentation
Tests