Skip to content

Commit aca9a0d

Browse files
jamesgroatjamesgroat
committed
fix(browser): resolve PR review issues with profile handling
- Fix POST routes ignoring profile param in body (BLOCKER) - Fix port collision when create-profile uses raw config instead of resolved - Fix runtime state not updating after create/delete profile - Fix legacy cdpUrl port ignored for default profile (backward compat) - Add tests for all fixes
1 parent 0c7e69d commit aca9a0d

File tree

6 files changed

+158
-20
lines changed

6 files changed

+158
-20
lines changed

‎src/browser/config.test.ts‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,37 @@ describe("browser config", () => {
6565
expect(resolved.cdpHost).toBe("example.com");
6666
expect(resolved.cdpIsLoopback).toBe(false);
6767

68-
// Default profile uses cdpHost from explicit cdpUrl with standard port
68+
// Default profile uses legacy cdpUrl port for backward compatibility
6969
const defaultProfile = resolveProfile(resolved, "clawd");
70-
expect(defaultProfile?.cdpUrl).toBe("http://example.com:18800");
70+
expect(defaultProfile?.cdpUrl).toBe("http://example.com:9222");
7171
});
7272

7373
it("rejects unsupported protocols", () => {
7474
expect(() =>
7575
resolveBrowserConfig({ controlUrl: "ws://127.0.0.1:18791" }),
7676
).toThrow(/must be http/i);
7777
});
78+
79+
it("respects legacy cdpUrl port for backward compatibility", () => {
80+
// Users with existing browser.cdpUrl config should keep working
81+
const resolved = resolveBrowserConfig({
82+
controlUrl: "http://127.0.0.1:18791",
83+
cdpUrl: "http://localhost:9222", // Legacy config pointing to Chrome at 9222
84+
});
85+
86+
// Default profile uses legacy cdpUrl port, not the new 18800 range
87+
const defaultProfile = resolveProfile(resolved, "clawd");
88+
expect(defaultProfile?.cdpPort).toBe(9222);
89+
expect(defaultProfile?.cdpUrl).toBe("http://localhost:9222");
90+
});
91+
92+
it("uses 18800 for default profile when no cdpUrl configured", () => {
93+
// Fresh install with no legacy config should use new port range
94+
const resolved = resolveBrowserConfig({
95+
controlUrl: "http://127.0.0.1:18791",
96+
});
97+
98+
const defaultProfile = resolveProfile(resolved, "clawd");
99+
expect(defaultProfile?.cdpPort).toBe(18800);
100+
});
78101
});

‎src/browser/config.ts‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,16 +80,17 @@ function parseHttpUrl(raw: string, label: string) {
8080

8181
/**
8282
* Ensure the default "clawd" profile exists in the profiles map.
83-
* Auto-creates it with the first port and default color if missing.
83+
* Auto-creates it with the legacy CDP port (from browser.cdpUrl) or first port if missing.
8484
*/
8585
function ensureDefaultProfile(
8686
profiles: Record<string, BrowserProfileConfig> | undefined,
8787
defaultColor: string,
88+
legacyCdpPort?: number,
8889
): Record<string, BrowserProfileConfig> {
8990
const result = { ...profiles };
9091
if (!result[DEFAULT_CLAWD_BROWSER_PROFILE_NAME]) {
9192
result[DEFAULT_CLAWD_BROWSER_PROFILE_NAME] = {
92-
cdpPort: CDP_PORT_RANGE_START,
93+
cdpPort: legacyCdpPort ?? CDP_PORT_RANGE_START,
9394
color: defaultColor,
9495
};
9596
}
@@ -140,7 +141,13 @@ export function resolveBrowserConfig(
140141

141142
const defaultProfile =
142143
cfg?.defaultProfile ?? DEFAULT_CLAWD_BROWSER_PROFILE_NAME;
143-
const profiles = ensureDefaultProfile(cfg?.profiles, defaultColor);
144+
// Use legacy cdpUrl port for backward compatibility when no profiles configured
145+
const legacyCdpPort = rawCdpUrl ? cdpInfo.port : undefined;
146+
const profiles = ensureDefaultProfile(
147+
cfg?.profiles,
148+
defaultColor,
149+
legacyCdpPort,
150+
);
144151

145152
return {
146153
enabled,

‎src/browser/profiles.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,46 @@ describe("getUsedPorts", () => {
111111
});
112112
});
113113

114+
describe("port collision prevention", () => {
115+
it("raw config vs resolved config - shows the data source difference", async () => {
116+
// This demonstrates WHY the route handler must use resolved config
117+
const { resolveBrowserConfig } = await import("./config.js");
118+
119+
// Fresh config with no profiles defined (like a new install)
120+
const rawConfigProfiles = undefined;
121+
const usedFromRaw = getUsedPorts(rawConfigProfiles);
122+
123+
// Raw config shows empty - no ports used
124+
expect(usedFromRaw.size).toBe(0);
125+
126+
// But resolved config has implicit clawd at 18800
127+
const resolved = resolveBrowserConfig({});
128+
const usedFromResolved = getUsedPorts(resolved.profiles);
129+
expect(usedFromResolved.has(CDP_PORT_RANGE_START)).toBe(true);
130+
});
131+
132+
it("create-profile must use resolved config to avoid port collision", async () => {
133+
// The route handler must use state.resolved.profiles, not raw config
134+
const { resolveBrowserConfig } = await import("./config.js");
135+
136+
// Simulate what happens with raw config (empty) vs resolved config
137+
const rawConfig = { browser: {} }; // Fresh config, no profiles
138+
const buggyUsedPorts = getUsedPorts(rawConfig.browser?.profiles);
139+
const buggyAllocatedPort = allocateCdpPort(buggyUsedPorts);
140+
141+
// Raw config: first allocation gets 18800
142+
expect(buggyAllocatedPort).toBe(CDP_PORT_RANGE_START);
143+
144+
// Resolved config: includes implicit clawd at 18800
145+
const resolved = resolveBrowserConfig(rawConfig.browser);
146+
const fixedUsedPorts = getUsedPorts(resolved.profiles);
147+
const fixedAllocatedPort = allocateCdpPort(fixedUsedPorts);
148+
149+
// Resolved: first NEW profile gets 18801, avoiding collision
150+
expect(fixedAllocatedPort).toBe(CDP_PORT_RANGE_START + 1);
151+
});
152+
});
153+
114154
describe("color allocation", () => {
115155
it("allocates first color when none used", () => {
116156
const usedColors = new Set<string>();

‎src/browser/routes/basic.ts‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -139,40 +139,46 @@ export function registerBrowserBasicRoutes(
139139

140140
try {
141141
const cfg = loadConfig();
142-
const profiles = cfg.browser?.profiles ?? {};
142+
// Use resolved profiles which includes implicit default (clawd at 18800)
143+
const state = ctx.state();
144+
const resolvedProfiles = state.resolved.profiles;
143145

144-
// Check if profile already exists
145-
if (name in profiles) {
146+
// Check if profile already exists (in resolved, not just raw config)
147+
if (name in resolvedProfiles) {
146148
return jsonError(res, 409, `profile "${name}" already exists`);
147149
}
148150

149-
// Allocate port and color
150-
const usedPorts = getUsedPorts(profiles);
151+
// Allocate port using resolved profiles to avoid collision with implicit default
152+
const usedPorts = getUsedPorts(resolvedProfiles);
151153
const cdpPort = allocateCdpPort(usedPorts);
152154
if (cdpPort === null) {
153155
return jsonError(res, 507, "no available CDP ports in range");
154156
}
155157

156-
const usedColors = getUsedColors(profiles);
158+
const usedColors = getUsedColors(resolvedProfiles);
157159
const profileColor =
158160
color && /^#[0-9A-Fa-f]{6}$/.test(color)
159161
? color
160162
: allocateColor(usedColors);
161163

162-
// Update config
164+
// Update config file
165+
const rawProfiles = cfg.browser?.profiles ?? {};
163166
const nextConfig: ClawdisConfig = {
164167
...cfg,
165168
browser: {
166169
...cfg.browser,
167170
profiles: {
168-
...profiles,
171+
...rawProfiles,
169172
[name]: { cdpPort, color: profileColor },
170173
},
171174
},
172175
};
173176

174177
await writeConfigFile(nextConfig);
175178

179+
// Update runtime state so new profile is immediately visible
180+
state.resolved.profiles[name] = { cdpPort, color: profileColor };
181+
176182
res.json({
177183
ok: true,
178184
profile: name,
@@ -240,8 +246,9 @@ export function registerBrowserBasicRoutes(
240246

241247
await writeConfigFile(nextConfig);
242248

243-
// Clear runtime state
249+
// Clear runtime state (both resolved config and runtime map)
244250
const state = ctx.state();
251+
delete state.resolved.profiles[name];
245252
state.profiles.delete(name);
246253

247254
res.json({

‎src/browser/routes/utils.test.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
import type { Request } from "express";
2+
import { describe, expect, it, vi } from "vitest";
3+
4+
import { getProfileContext } from "./utils.js";
5+
6+
describe("getProfileContext", () => {
7+
const mockCtx = {
8+
forProfile: vi.fn((name?: string) => ({
9+
profile: { name: name ?? "clawd", cdpPort: 18800 },
10+
})),
11+
};
12+
13+
it("reads profile from query string", () => {
14+
const req = {
15+
query: { profile: "work" },
16+
body: {},
17+
} as unknown as Request;
18+
19+
const result = getProfileContext(req, mockCtx as never);
20+
expect(mockCtx.forProfile).toHaveBeenCalledWith("work");
21+
expect(result).toHaveProperty("profile");
22+
});
23+
24+
it("reads profile from body when query is empty (POST requests)", () => {
25+
// This tests the fix for: POST routes ignoring profile in body
26+
const req = {
27+
query: {},
28+
body: { profile: "work" },
29+
} as unknown as Request;
30+
31+
mockCtx.forProfile.mockClear();
32+
const result = getProfileContext(req, mockCtx as never);
33+
34+
// Fixed: Should call forProfile("work"), not forProfile(undefined)
35+
expect(mockCtx.forProfile).toHaveBeenCalledWith("work");
36+
expect(result).toHaveProperty("profile");
37+
});
38+
39+
it("prefers query string over body", () => {
40+
const req = {
41+
query: { profile: "from-query" },
42+
body: { profile: "from-body" },
43+
} as unknown as Request;
44+
45+
mockCtx.forProfile.mockClear();
46+
const result = getProfileContext(req, mockCtx as never);
47+
48+
expect(mockCtx.forProfile).toHaveBeenCalledWith("from-query");
49+
expect(result).toHaveProperty("profile");
50+
});
51+
});

‎src/browser/routes/utils.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,27 @@ import type express from "express";
33
import type { BrowserRouteContext, ProfileContext } from "../server-context.js";
44

55
/**
6-
* Extract profile name from query string and get profile context.
7-
* Returns the profile context or null if the profile doesn't exist.
6+
* Extract profile name from query string or body and get profile context.
7+
* Query string takes precedence over body for consistency with GET routes.
88
*/
99
export function getProfileContext(
1010
req: express.Request,
1111
ctx: BrowserRouteContext,
1212
): ProfileContext | { error: string; status: number } {
13-
const profileName =
14-
typeof req.query.profile === "string"
15-
? req.query.profile.trim()
16-
: undefined;
13+
let profileName: string | undefined;
14+
15+
// Check query string first (works for GET and POST)
16+
if (typeof req.query.profile === "string") {
17+
profileName = req.query.profile.trim() || undefined;
18+
}
19+
20+
// Fall back to body for POST requests
21+
if (!profileName && req.body && typeof req.body === "object") {
22+
const body = req.body as Record<string, unknown>;
23+
if (typeof body.profile === "string") {
24+
profileName = body.profile.trim() || undefined;
25+
}
26+
}
1727

1828
try {
1929
return ctx.forProfile(profileName);

0 commit comments

Comments
 (0)