fix(browser): fall back to Hyprland grim capture for headed viewport screenshots#71940
fix(browser): fall back to Hyprland grim capture for headed viewport screenshots#71940Angfr95 wants to merge 3 commits intoopenclaw:mainfrom
Conversation
95b6abb to
ada551d
Compare
Greptile SummaryThis PR adds an opt-in Hyprland/Wayland screenshot fallback using
Confidence Score: 3/5Functional for the Hyprland capture path, but the missing SIGTERM handler leaks a virtual display output on every normal service stop — needs a one-line fix before merging. One P1 defect (SIGTERM not handled → leaked virtual output on every systemd/Docker stop), plus two P2 style/correctness issues. The rest of the code is well-structured with thorough tests. P1 ceiling is 4; one clear P1 in a new feature path pulls the score to 3. extensions/browser/src/browser/hyprland-capture.ts — the registerExitCleanup function needs a SIGTERM handler to match the SIGINT handler. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/browser/src/browser/hyprland-capture.ts
Line: 80-84
Comment:
**SIGTERM not handled — virtual output leaked on service stop**
`process.once("exit")` only fires when Node exits via `process.exit()` or natural completion; it does **not** fire when the process is terminated by a signal. `SIGTERM` is the default stop signal used by `kill`, systemd (`systemctl stop`), and Docker, so every normal service stop will leave the `HEADLESS-N` virtual output behind on the Hyprland compositor. Users would need to manually run `hyprctl output remove HEADLESS-N` after each restart.
```ts
process.once("exit", cleanup);
process.once("SIGINT", () => {
cleanup();
process.exit(130);
});
+ process.once("SIGTERM", () => {
+ cleanup();
+ process.exit(143);
+ });
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/browser/src/browser/hyprland-capture.ts
Line: 218-224
Comment:
**Test reset accumulates `process.once` listeners across test runs**
`_cleanupRegistered = false` causes the next call to `registerExitCleanup` to register fresh `process.once("exit")` and `process.once("SIGINT")` handlers on top of the ones still live from the previous test run. With 10 test runs that each successfully set up capture, the process accumulates up to 10 of each listener, which will trigger Node's `MaxListenersExceededWarning`. The reset should also remove the previously registered listeners — store references to them (or use `removeAllListeners` on the specific events) before the reset clears the closures.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/browser/src/browser/routes/agent.snapshot.ts
Line: 377
Comment:
**Global `resolved.headless` used instead of per-profile headless flag**
`ctx.state().resolved.headless` reflects the top-level browser config's `headless` setting. If a specific profile overrides headless at the profile level (e.g., a profile's own `headless: true`), this check passes and Hyprland capture is attempted against a headless browser process. `grim` cannot capture a headless window, so it will fail and fall back to CDP, but it adds unnecessary latency on every screenshot for that profile.
Consider using `profileCtx.profile.headless` instead, which is the resolved per-profile value already available in scope.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: cache external plugin catalog looku..." | Re-trigger Greptile |
| process.once("exit", cleanup); | ||
| process.once("SIGINT", () => { | ||
| cleanup(); | ||
| process.exit(130); | ||
| }); |
There was a problem hiding this comment.
process.once("exit") only fires when Node exits via process.exit() or natural completion; it does not fire when the process is terminated by a signal. SIGTERM is the default stop signal used by kill, systemd (systemctl stop), and Docker, so every normal service stop will leave the HEADLESS-N virtual output behind on the Hyprland compositor. Users would need to manually run hyprctl output remove HEADLESS-N after each restart.
process.once("exit", cleanup);
process.once("SIGINT", () => {
cleanup();
process.exit(130);
});
+ process.once("SIGTERM", () => {
+ cleanup();
+ process.exit(143);
+ });Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/browser/src/browser/hyprland-capture.ts
Line: 80-84
Comment:
**SIGTERM not handled — virtual output leaked on service stop**
`process.once("exit")` only fires when Node exits via `process.exit()` or natural completion; it does **not** fire when the process is terminated by a signal. `SIGTERM` is the default stop signal used by `kill`, systemd (`systemctl stop`), and Docker, so every normal service stop will leave the `HEADLESS-N` virtual output behind on the Hyprland compositor. Users would need to manually run `hyprctl output remove HEADLESS-N` after each restart.
```ts
process.once("exit", cleanup);
process.once("SIGINT", () => {
cleanup();
process.exit(130);
});
+ process.once("SIGTERM", () => {
+ cleanup();
+ process.exit(143);
+ });
```
How can I resolve this? If you propose a fix, please make it concise.| export function _resetHyprlandCaptureForTests(): void { | ||
| _state = null; | ||
| _setupInFlight = null; | ||
| _cleanupOutputName = null; | ||
| _cleanupHyprctl = null; | ||
| _cleanupRegistered = false; | ||
| } |
There was a problem hiding this comment.
process.once listeners across test runs
_cleanupRegistered = false causes the next call to registerExitCleanup to register fresh process.once("exit") and process.once("SIGINT") handlers on top of the ones still live from the previous test run. With 10 test runs that each successfully set up capture, the process accumulates up to 10 of each listener, which will trigger Node's MaxListenersExceededWarning. The reset should also remove the previously registered listeners — store references to them (or use removeAllListeners on the specific events) before the reset clears the closures.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/browser/src/browser/hyprland-capture.ts
Line: 218-224
Comment:
**Test reset accumulates `process.once` listeners across test runs**
`_cleanupRegistered = false` causes the next call to `registerExitCleanup` to register fresh `process.once("exit")` and `process.once("SIGINT")` handlers on top of the ones still live from the previous test run. With 10 test runs that each successfully set up capture, the process accumulates up to 10 of each listener, which will trigger Node's `MaxListenersExceededWarning`. The reset should also remove the previously registered listeners — store references to them (or use `removeAllListeners` on the specific events) before the reset clears the closures.
How can I resolve this? If you propose a fix, please make it concise.| !fullPage && | ||
| !element && | ||
| process.platform === "linux" && | ||
| !ctx.state().resolved.headless && |
There was a problem hiding this comment.
resolved.headless used instead of per-profile headless flag
ctx.state().resolved.headless reflects the top-level browser config's headless setting. If a specific profile overrides headless at the profile level (e.g., a profile's own headless: true), this check passes and Hyprland capture is attempted against a headless browser process. grim cannot capture a headless window, so it will fail and fall back to CDP, but it adds unnecessary latency on every screenshot for that profile.
Consider using profileCtx.profile.headless instead, which is the resolved per-profile value already available in scope.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/browser/src/browser/routes/agent.snapshot.ts
Line: 377
Comment:
**Global `resolved.headless` used instead of per-profile headless flag**
`ctx.state().resolved.headless` reflects the top-level browser config's `headless` setting. If a specific profile overrides headless at the profile level (e.g., a profile's own `headless: true`), this check passes and Hyprland capture is attempted against a headless browser process. `grim` cannot capture a headless window, so it will fail and fall back to CDP, but it adds unnecessary latency on every screenshot for that profile.
Consider using `profileCtx.profile.headless` instead, which is the resolved per-profile value already available in scope.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95b6abb8d5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Fixes Page.captureScreenshot timeouts on Hyprland. Requires hyprctl and grim. | ||
| * Default: false (opt-in). | ||
| */ | ||
| hyprlandCapture?: boolean; |
There was a problem hiding this comment.
Accept hyprlandCapture in validated browser schema
This adds browser.hyprlandCapture to config types and runtime resolution, but it is not added to the strict runtime schema, so users cannot actually enable it through config. OpenClawSchema validates browser with a strict object in src/config/zod-schema.ts, and that shape does not include hyprlandCapture; setting the new key will be rejected as an unknown property instead of activating the Hyprland capture fallback.
Useful? React with 👍 / 👎.
| process.once("SIGINT", () => { | ||
| cleanup(); | ||
| process.exit(130); |
There was a problem hiding this comment.
Avoid forcing process exit from Hyprland SIGINT handler
The new cleanup registration unconditionally calls process.exit(130) on SIGINT. In runtime contexts that already install SIGINT handlers for graceful shutdown (for example the gateway run loop), this helper will force abrupt termination once Hyprland capture has been used, which can bypass normal async stop/flush behavior. This module should not hard-exit the process from inside a feature-specific signal handler.
Useful? React with 👍 / 👎.
| // Capture error → invalidate cache so the next call re-creates the output. | ||
| _state = null; | ||
| _setupInFlight = null; |
There was a problem hiding this comment.
Tear down virtual output when grim capture fails
On grim failure, the catch path only clears _state and _setupInFlight without removing the previously created virtual output. Subsequent attempts re-run setup and create additional outputs, while exit cleanup tracks only the latest output name, so repeated capture failures can leave orphan Hyprland virtual outputs for the life of the process.
Useful? React with 👍 / 👎.
5ec6536 to
5c0bcb3
Compare
|
Codex review: needs changes before merge. Summary Reproducibility: yes. #64317 gives a concrete headed Chromium screenshot-timeout path on Omarchy/Hyprland/Wayland, current main still uses only CDP for this screenshot route, and the PR blockers are reproducible by source tracing the signal handlers, setup error path, and SDK baseline contract. Next step before merge Security Review findings
Review detailsBest possible solution: Revise this PR or a narrow replacement so the Hyprland fallback stays opt-in and viewport-only, preserves normal process shutdown, removes virtual outputs on all setup/capture failures, aligns the public SDK contract artifacts/docs, adds the required changelog entry, and links the PR to #64317 instead of the unrelated Windows issue. Do we have a high-confidence way to reproduce the issue? Yes. #64317 gives a concrete headed Chromium screenshot-timeout path on Omarchy/Hyprland/Wayland, current main still uses only CDP for this screenshot route, and the PR blockers are reproducible by source tracing the signal handlers, setup error path, and SDK baseline contract. Is this the best way to solve the issue? No. The feature direction is plausible and appropriately scoped to the browser plugin path, but the current patch is not mergeable until the lifecycle cleanup, SDK baseline, changelog, and PR metadata issues are fixed. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against a0d53726137d. |
…6246) (thanks @yfge) * fix: cache external plugin catalog lookups in auto-enable Fixes openclaw#66159 * test: restore readFileSync spy in plugin auto-enable test * refactor: distill plugin auto-enable cache path * fix: cache external plugin catalog lookups in auto-enable (openclaw#66246) (thanks @yfge) --------- Co-authored-by: Ayaan Zaidi <hi@obviy.us>
Add focused openclaw/plugin-sdk/resolve-system-bin subpath so bundled extensions avoid importing the broad infra-runtime barrel flagged by the newer boundary check. Regenerate schema snapshot with title and description for hyprlandCapture so the config snapshot test stays green.
fdcb510 to
e30d5bd
Compare
|
Thanks for the detailed review. This PR was superseded by #71940 which addresses all the blockers raised here — cc @steipete for routing.
The PNG-only behavior on the Hyprland path and the troubleshooting doc are also included in #71940. Also cc @steipete —I want to flag a serious attribution issue with PR #74690 merged today. Full timeline: 1 - I opened #66395 two weeks ago fixing #65522 with the correct guard + 6 browser render tests This is not a coincidence. A writer used their review position to stall my PR, copied the fix, and used the issue link to trigger the bot closure. Everything is timestamped and verifiable on GitHub. I am the original author of this fix. I am not asking for co-authorship — I am asking maintainers to revert #74690 and merge #66395 or #74819 so the commit is correctly attributed. |
066e6a7 to
e30d5bd
Compare
Closes #54470
Problem
On native Windows,
openclaw webhooks gmail setup --accountcrashed immediately withError: spawn gcloud ENOENT. Node'schild_process.spawnwithshell: falsedoes notconsult
PATHEXT, so baregcloud,gog, andtailscaleare not found even whenwhere.exeresolves them to valid.cmd/.exepaths.Solution
Add
resolveExecutable(cmd)insrc/infra/executable-path.ts:cmdalready carries a known extension (.com,.exe,.bat,.cmd)where.exeviaexecFileSync, prefers.cmd→.exe→ first result,falls back to original
cmdon throwshell: trueanywhereThread the resolved binary through every affected spawn site:
gcloudBin: module-level??=inrunGcloudCommand— onewhere.exefor the entire setup sequencetailscaleBin: local variable inensureTailscaleEndpoint— onewhere.exefor bothrunCommandWithTimeoutcallsgogBin: module-level??=shared bystartGmailWatchandspawnGogServe— onewhere.exefor the process lifetime, zero extra calls on watcher restartsFiles changed
src/infra/executable-path.ts— new file,resolveExecutablesrc/hooks/gmail-setup-utils.ts—gcloudBin+tailscaleBinsrc/hooks/gmail-watcher.ts—gogBinsrc/infra/executable-path.test.ts— 10 unit tests (all 5 resolution branches +lines[0]fallback)src/hooks/gmail-setup-utils.test.ts— 3 tests covering the new cached resolutionTest results
pnpm test src/hooks/gmail-setup-utils.test.ts src/hooks/gmail.test.ts src/hooks/gmail-watcher-lifecycle.test.ts ✅
pnpm test src/process/exec.windows.test.ts src/process/windows-command.test.ts src/infra/executable-path.test.ts ✅
pnpm check:changed ✅
Notes
src/process/exec.tsis untouched — existing package-manager shim is unaffectedwhere.exe gcloudresolving toC:\Tools\gcloud\gcloud.cmd