fix: resolve exec PATH fallback, layered browser diagnostics, and cron force-run deadlock#42027
Conversation
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 |
| 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.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.
Reuse short status timeout for config snapshot call
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. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. by source inspection. The active browser plugin status route/type/CLI lack layered diagnostics and Next step before merge Security Review findings
Review detailsBest possible solution: Replace or split the PR: land a narrow browser plugin patch for layered status diagnostics and bounded HTML upstream-error summaries, leaving current main exec auto-routing and CronNested behavior unchanged. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection. The active browser plugin status route/type/CLI lack layered diagnostics and Is this the best way to solve the issue? No, not as submitted. The browser direction is useful, but the patch targets obsolete core browser paths and carries two boundedness/timeout issues, so a narrow 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 89a15fddaf84. |
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.