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 into
Conversation
openclaw-barnacle
Bot
added
docs
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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.
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.
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
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep this PR open because current main still lacks a Hyprland/Wayland native viewport screenshot fallback and the branch contains a relevant implementation direction, but it is not merge-ready: live Hyprland proof is missing, GitHub reports conflicts, and the patch still has native-capture cleanup/isolation and public SDK-surface blockers. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Canonical path: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. So I’m closing this here because the remaining work is already tracked in the canonical issue. Review detailsBest possible solution: Close this stale PR. The latest review rated it F, the branch still lacks merge-ready proof, and there has been no human follow-up after the durable review. Do we have a high-confidence way to reproduce the issue? Partly. Current main still routes ordinary viewport screenshots through CDP only, and the linked Hyprland issue has concrete live timeout and downstream workaround evidence, but I did not reproduce the failure on a real Hyprland desktop. Is this the best way to solve the issue? No, not yet. The Security review: Security review needs attention: The diff introduces a native compositor screenshot path that can capture unintended pixels unless workspace isolation and live proof are tightened.
AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against 47d3d1b1f1b2. |
…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 <[email protected]>
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. |
Angfr95
changed the title
066e6a7 to
e30d5bd
Compare
Angfr95
changed the title
|
Adding implementation feedback from a live downstream Omarchy/Arch/Hyprland validation pass. This PR is pointed in the right direction, especially the use of
In the local proof, relying only on the virtual output's current active workspace was not enough. The robust setup was:
This reduces wrong-window risk if the compositor places another client on the same output/workspace.
Creating/selecting the capture workspace and later opening/navigating tabs can focus the hidden output. In the local proof, restoring focus during setup was not sufficient by itself; the screenshot path also had to restore focus after Suggested merge proof for this PR:
No private config or local paths are needed for the proof; the above can be shown with redacted command output. |
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
P1
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
|
This pull request has been automatically marked as stale due to inactivity. |
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
ClawSweeper applied the proposed close for this PR.
|
3 participants
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