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. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs real behavior proof before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. for the PR blockers by source inspection: current main has the active browser status path under extensions/browser, while the PR changes removed src/browser paths, and the added config.get call is visibly unbounded in the PR diff. I did not live-run the Windows/WSL2 CDP or LLM-backed cron scenarios in this read-only review. PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Risk before merge
Maintainer options:
Next step before merge Security Review findings
Review detailsBest possible solution: Split or rebase the branch so only the remaining browser diagnostics are ported to extensions/browser with bounded probes and capped error summaries, while preserving current main's exec auto-host and cron-nested lane behavior. Do we have a high-confidence way to reproduce the issue? Yes for the PR blockers by source inspection: current main has the active browser status path under extensions/browser, while the PR changes removed src/browser paths, and the added config.get call is visibly unbounded in the PR diff. I did not live-run the Windows/WSL2 CDP or LLM-backed cron scenarios in this read-only review. Is this the best way to solve the issue? No, this branch is not the best way to solve the remaining issue as submitted. The maintainable path is an active-plugin rewrite or split follow-up that keeps the current exec and cron fixes already on main. Label justifications:
Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 48acdd3d85ea. |
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
clawsweeper
Bot
added
impact:crash-loop
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
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.