Add batched POST /chromium/configure#244
Conversation
Single multipart request can apply display, policies, flags, extensions, and profile changes with one Chromium restart, then navigate via CDP after DevTools is ready. Co-authored-by: Cursor <cursoragent@cursor.com>
Drop DETACHED run-docker mode, the powerset runner script, and related AGENTS.md notes. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Monitoring Plan:
|
Batching keeps a single Chromium restart while preserving Kernel's configure order, propagating required-step failures, and keeping start_url best-effort. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid unnecessary stop/start cycles for empty flag updates and remove stale CDP/configure helpers flagged during review. Co-authored-by: Cursor <cursoragent@cursor.com>
Match stop/start gating to policy apply semantics so empty policy objects remain no-op configuration. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the display configure branches consistent if display planning ever becomes a no-op. Co-authored-by: Cursor <cursoragent@cursor.com>
hiroTamada
changed the title
Keep shared API errors generic while documenting the configure-specific step field on ChromiumConfigureError. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep flag validation consistent with actionable detection and reject duplicate extension zip parts before copying uploads. Co-authored-by: Cursor <cursoragent@cursor.com>
Classify multipart parse failures by response type, keep policy validation errors as bad requests, and clear Xvfb viewport overrides under the resize lock. Co-authored-by: Cursor <cursoragent@cursor.com>
rgarcia
left a comment
rgarcia
left a comment
There was a problem hiding this comment.
reviewed — implementation shape looks good overall. a few API/schema things worth tightening before this lands:
server/openapi.yaml:1173— nit: consider/configureinstead of/chromium/configure; display config is not Chromium-specific, and this endpoint may grow to cover more non-Chromium session configuration over time.server/openapi.yaml:1182— the “control-plane sync path” wording feels too inside-baseball for public API docs. maybe reword this to just describe the execution order directly.server/openapi.yaml:1214—strip_componentsfeels like archive layout mechanics leaking into a high-level profile/configure API. even though this exists on the generic zstd upload endpoint, for profile archives it may be better to require/document one expected archive structure instead.server/openapi.yaml:2586—ChromiumConfigureErrordoes not definestep, but the handler returns it and generatedoapi.gocurrently includes it. since this schema hasadditionalProperties: false, the source schema and generated code should be reconciled so clients/validators agree on the 500 response shape.
Confirm stopped states after supervisor errors and verify DevTools readiness by dialing the current upstream instead of relying only on log notifications. Co-authored-by: Cursor <cursoragent@cursor.com>
Expose batched session configuration at /configure and clean up public OpenAPI wording around execution order and profile archives. Co-authored-by: Cursor <cursoragent@cursor.com>
| return "https://" + rawURL | ||
| } | ||
| return rawURL | ||
| } |
There was a problem hiding this comment.
No scheme validation allows file:// and javascript: navigation
Medium Severity
normalizeStartURL only adds https:// for bare hosts. URLs with explicit schemes like file:///etc/passwd or javascript:... pass through unvalidated and are dispatched via CDP Page.navigate. The test at line 69 confirms file:///etc/passwd is accepted. While Chrome may block some schemes, file:// URIs are navigable in Chromium when launched without --disable-file-url-scheme, potentially exposing local filesystem contents through the browser.
Reviewed by Cursor Bugbot for commit 182f0a8. Configure here.
Only mark Chromium as restarted after DevTools readiness succeeds so deferred recovery can retry failed starts. Co-authored-by: Cursor <cursoragent@cursor.com>
| $ref: "#/components/responses/InternalError" | ||
|
|
||
| /configure: | ||
| post: |
There was a problem hiding this comment.
Endpoint path /configure doesn't name the Chromium subsystem
Low Severity
The new endpoint is registered at /configure, but all existing related endpoints live under /chromium/* (/chromium/policies, /chromium/flags, /chromium/upload-extensions-and-restart). The generic /configure path doesn't identify the subsystem being configured. The PR title itself references POST /chromium/configure, and the OpenAPI description even says "Prefer this over separate /chromium/* and /display calls," confirming this is Chromium-scoped. The path violates the rule requiring paths to name the specific subsystem.
Additional Locations (1)
Triggered by learned rule: OpenAPI descriptions must be caller-focused; paths must name the specific subsystem
Reviewed by Cursor Bugbot for commit 569d99f. Configure here.
| len(st.extItems) > 0 || | ||
| policiesContentNonEmpty(st.chromePoliciesJSON) || | ||
| flagsContentNonEmpty(st.chromiumFlagsJSON) | ||
| } |
There was a problem hiding this comment.
Display-only configure path skips display when needsStop true
Medium Severity
chromiumNeedsStopCycle does not consider displayJSON as a stop-cycle trigger, but within the needsStop=true branch, the display section is guarded by st.displayJSON != nil && strings.TrimSpace(*st.displayJSON) != "". If a user sends only display with a RestartChromium: true field alongside, say, flags that trigger the stop cycle, the stopped-path display logic runs. However, in the stopped path, chromiumDisplayApplyWhileStopped ignores the RestartChromium field from the display body entirely. This is inconsistent with the standalone PatchDisplay behavior where RestartChromium controls whether Chromium is restarted for the resize. The batched handler always restarts regardless because the stop cycle forces it.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 569d99f. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 5 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f63e8a. Configure here.
| if cur != nil && cur.gotZip && cur.name != "" { | ||
| st.extItems = append(st.extItems, extensionZipItem{zipTemp: cur.zipTmp, name: cur.name}) | ||
| cur = nil | ||
| } |
There was a problem hiding this comment.
Duplicate extension names across pairs silently overwrite each other
Medium Severity
The new chromiumCfgParseMultipart validates duplicate extensions.name within a single pair but not across pairs. Two pairs with the same name (e.g. both named "foo") pass the parser and reach applyExtensionZipItems, where the batch stat-check loop passes for both (neither exists on disk yet), then the second pair's zip silently overwrites the first's extracted files. This is inconsistent with the per-pair duplicate rejection the PR explicitly added.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0f63e8a. Configure here.
| status, statusOut, _ := chromiumSupervisorStatus(ctx) | ||
| log.Info("devtools not ready in time", "operation", operation, "elapsed", time.Since(start).String(), "supervisor_status", statusOut) | ||
| return fmt.Errorf("devtools not ready in time (chromium status: %s)", status) | ||
| } |
There was a problem hiding this comment.
Start error may be missed due to select race
Low Severity
In startChromiumAndWait, when the start goroutine fails it sends on errCh then closes doneCh (via defer). Both channels become ready nearly simultaneously. If select picks doneCh first and tryReady happens to succeed (e.g. DevTools is reachable from a stale upstream with allowCurrent=true), the function returns nil (success) despite the supervisorctl start command having failed. The error on errCh is never consumed.
Reviewed by Cursor Bugbot for commit 0f63e8a. Configure here.
2 participants
Summary
POST /chromium/configure: one multipart request to batch display sizing, Chrome policies, Chromium flags, profile archive, extensions, and optionalstart_urlnavigation after a single Chromium restart.cdpclient.NavigateFirstPagefor post-readyPage.navigate(flattened CDP session, waits for load /domcontentloaded). Fixes concurrent WebSocket reads that caused flaky navigate failures.applyExtensionZipItemsused by configure and existing upload-and-restart.start_urltests; local runner script andDETACHED=1headlessrun-docker.shfor manual API use.make test: e2e package timeout raised to 120m (full suite exceeds default 10m).Test plan
go vet ./...and unit tests (go testexcluding/e2e)./scripts/run-local-chromium-configure-powerset.sh(31 permutations + start_url variants) against locally built headless imagego test -race -timeout 120m ./e2e/withE2E_CHROMIUM_HEADLESS_IMAGE/E2E_CHROMIUM_HEADFUL_IMAGEpointing at images built from this branchMade with Cursor
Note
Medium Risk
Moderate risk: introduces a new multipart configure endpoint that performs filesystem/policy/flag/profile mutations and changes the Chromium restart/DevTools-readiness flow, which could affect instance startup stability and extension handling.
Overview
Adds a new multipart
POST /configureendpoint to apply batched Chromium/session changes (policies, flags, extensions, profile archive, and display sizing) with a single stop/start cycle, plus optional best-effortstart_urlnavigation via CDP.Refactors extension installation into shared
applyExtensionZipItems(with partial-install cleanup) and reworks Chromium restart logic to explicitlystopthenstartviasupervisorctl, waiting for DevTools readiness by actively dialing the upstream.Extends
cdpclientwithDispatchStartURL, updates generated OpenAPI client/server types (including structuredChromiumConfigureError), adds unit + e2e coverage for multipart combinations, and increases e2e test timeout to120m.Reviewed by Cursor Bugbot for commit 0f63e8a. Bugbot is set up for automated code reviews on this repo. Configure here.