fix: resolve exec PATH fallback, layered browser diagnostics, and cron force-run deadlock#42027
Conversation
openclaw-barnacle
bot
added
gateway
Greptile SummaryThis PR addresses three targeted bug fixes: (1) login-shell PATH recovery now triggers when Key changes:
Confidence Score: 4/5
Last reviewed commit: 3e8ab2b |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function stripHtmlErrorText(input: string): string { | ||
| return input | ||
| .replace(/<script[\s\S]*?<\/script>/gi, " ") | ||
| .replace(/<style[\s\S]*?<\/style>/gi, " ") | ||
| .replace(/<[^>]+>/g, " ") | ||
| .replace(/\s+/g, " ") | ||
| .trim(); |
There was a problem hiding this comment.
> in attribute values can defeat the tag-stripping regex.
/<[^>]+>/g stops at the first > it encounters, so an attribute value that contains an unencoded > (e.g., aria-label="a > b", which some proxy error pages include) will leave the tail of the attribute and the closing > in the output. This is the fallback path that fires when neither <title> nor <h1> is found, so the resulting "summary" could contain raw attribute fragments and could be unboundedly long if the page body has significant text content.
Consider using a tighter pattern and capping the output length in the fallback:
| function stripHtmlErrorText(input: string): string { | |
| return input | |
| .replace(/<script[\s\S]*?<\/script>/gi, " ") | |
| .replace(/<style[\s\S]*?<\/style>/gi, " ") | |
| .replace(/<[^>]+>/g, " ") | |
| .replace(/\s+/g, " ") | |
| .trim(); | |
| function stripHtmlErrorText(input: string): string { | |
| return input | |
| .replace(/<script[\s\S]*?<\/script>/gi, " ") | |
| .replace(/<style[\s\S]*?<\/style>/gi, " ") | |
| .replace(/<[^>]*>/g, " ") | |
| .replace(/&[a-z]+;/gi, " ") | |
| .replace(/\s+/g, " ") | |
| .trim() | |
| .slice(0, 200); | |
| } |
Note: [^>]* (zero-or-more instead of one-or-more) is a minor alignment fix, but the more actionable change is adding .slice(0, 200) so that an HTML page without a title or heading can't push a multi-kilobyte text dump into the thrown error message.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/browser/client-fetch.ts
Line: 157-163
Comment:
`>` in attribute values can defeat the tag-stripping regex.
`/<[^>]+>/g` stops at the first `>` it encounters, so an attribute value that contains an unencoded `>` (e.g., `aria-label="a > b"`, which some proxy error pages include) will leave the tail of the attribute and the closing `>` in the output. This is the fallback path that fires when neither `<title>` nor `<h1>` is found, so the resulting "summary" could contain raw attribute fragments and could be unboundedly long if the page body has significant text content.
Consider using a tighter pattern and capping the output length in the fallback:
```suggestion
function stripHtmlErrorText(input: string): string {
return input
.replace(/<script[\s\S]*?<\/script>/gi, " ")
.replace(/<style[\s\S]*?<\/style>/gi, " ")
.replace(/<[^>]*>/g, " ")
.replace(/&[a-z]+;/gi, " ")
.replace(/\s+/g, " ")
.trim()
.slice(0, 200);
}
```
Note: `[^>]*` (zero-or-more instead of one-or-more) is a minor alignment fix, but the more actionable change is adding `.slice(0, 200)` so that an HTML page without a title or heading can't push a multi-kilobyte text dump into the thrown error message.
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: 3e8ab2b689
ℹ️ 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".
| }; | ||
| }; | ||
| }> { | ||
| const payload = await callGatewayFromCli("config.get", opts, {}); |
There was a problem hiding this comment.
The new browser status path now always does config.get via callGatewayConfigSnapshot, but this helper does not pass a bounded timeout and therefore inherits the CLI default timeout from callGatewayFromCli (30s), unlike fetchBrowserStatus which explicitly uses 1500ms; when config.get is slow or partially unavailable, openclaw browser status can hang for tens of seconds before falling back to existing diagnostics, which is a responsiveness regression introduced by this change.
Useful? React with 👍 / 👎.
|
Additional real-world validation from today (Mar 24): this PR’s layered diagnostics would have helped a lot in a WSL2 + Windows CDP outage triggered by virtualization mode toggling. Repro chain:
Recovery that restored remote tabs:
I posted full field notes in #41553 as follow-up evidence. +1 on this direction. |
2 participants
Summary
Describe the problem and fix in 2�C4 bullets:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
openclaw browser statusnow surfaces layered diagnostics for Control UI auth/origin configuration, browser profile attach mode, and remote/local CDP reachability.cron.run --forceno longer self-deadlocks when isolated cron work reuses the cron execution lane.Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) YesYes/No) YesYes, explain risk + mitigation:config.getsnapshot through the authenticated gateway path to derive operator diagnostics; it does not expose unredacted secrets.Repro + Verification
Environment
Steps
tools.exec.hostunset, disable sandbox runtime, and run an exec command that depends on login-shell PATH.openclaw browser statusagainst a remote CDP profile in a WSL2 + Windows style setup.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm check;pnpm test -- --run src/agents/bash-tools.exec.path.test.ts src/browser/status-diagnostics.test.ts src/browser/server.status-diagnostics.test.ts src/cli/browser-cli-manage.status-diagnostics.test.ts src/browser/client.test.ts src/cron/service.issue-regressions.test.ts;pnpm build.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
src/agents/bash-tools.exec.ts,src/browser/*status*,src/cli/browser-cli-*,src/cron/service/ops.ts,src/process/lanes.ts, gateway lane config wiring.Risks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.CommandLane.CronandCommandLane.CronManualtogether, with regression coverage for the deadlock case.