fix: land ten small reliability fixes#100399
Merged
Merged
Conversation
openclaw-barnacle
Bot
added
app: android
steipete
force-pushed
the
codex/land-small-fixes-batch-4-final
branch
from
July 5, 2026 17:47
312e3f0 to
d289884
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 312e3f0359
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+101
to
+102
| } catch (err: CancellationException) { | ||
| throw err |
Contributor
There was a problem hiding this comment.
When a camera invoke is cancelled after showCameraHud("Taking photo…", ..., null), this new rethrow skips both the success/error HUD updates, and NodeRuntime.showCameraHud only auto-clears entries when an autoHideMs is supplied. A timeout or disconnect during camera.snap can therefore leave the indefinite “Taking photo…” overlay visible; handleClip has the same pattern for “Recording…”. Clear or replace the HUD on cancellation before rethrowing so the invoke still does not send a stale result.
Useful? React with 👍 / 👎.
Contributor
|
Codex review: needs changes before merge. Reviewed July 5, 2026, 2:07 PM ET / 18:07 UTC. Summary PR surface: Source +47, Tests +214, Docs +7, Other +58. Total +326 across 25 files. Reproducibility: yes. for the review finding by source inspection: CameraHandler shows indefinite progress HUDs, the new cancellation catches rethrow before success/error HUD replacement, and NodeRuntime only auto-clears HUDs with a non-null timeout. I did not run an Android UI reproduction in this read-only review. Review metrics: 2 noteworthy metrics.
Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land the batch only after preserving cancellation propagation while clearing the Android camera progress HUD, then have the maintainer owner explicitly accept the compatibility-sensitive helper/verifier behavior and release-note handling. Do we have a high-confidence way to reproduce the issue? Yes for the review finding by source inspection: CameraHandler shows indefinite progress HUDs, the new cancellation catches rethrow before success/error HUD replacement, and NodeRuntime only auto-clears HUDs with a non-null timeout. I did not run an Android UI reproduction in this read-only review. Is this the best way to solve the issue? No, not as submitted: most fixes are narrow, but cancellation must still clear or replace the camera HUD before rethrowing. A small Android-side fix is safer than weakening cancellation propagation or reverting the no-stale-result behavior. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c757675f348d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +47, Tests +214, Docs +7, Other +58. Total +326 across 25 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
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. How this review workflow works
|
Preserve proposal_content exactly at the agent tool boundary and make renderProposalMarkdown defensively emit a terminal newline. Add focused regressions for the tool write path and markdown renderer.
Optional positive-integer tool params (e.g. Telegram replyTo/threadId) threw ToolInputError when a tool-calling model populated them with an empty-string or whitespace-only default. Those defaults carry no value, so readPositiveIntegerParam/readNonNegativeIntegerParam now treat a blank string as unset (undefined) instead of throwing, while still rejecting genuinely invalid present values (0, "42.5", "-3"). This prevents silent message-delivery failures when models emit empty routing-param defaults. Adds unit tests covering blank vs invalid.
The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since #97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection.
…g native .slice sliceUtf16Safe silently swapped reversed bounds (to < from) instead of returning "" like String.prototype.slice, creating a subtle footgun for callers with dynamic start/end pairs. Caller scan across src/, extensions/, and packages/ confirmed no production code relies on the old swap behavior — all callers use (text, 0, N) or (text, -N) forms only. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…blocks Catch (err: Throwable) swallows kotlinx.coroutines.CancellationException, breaking structured concurrency when the coroutine scope is cancelled during camera operations (handleList/handleSnap/handleClip). Add CancellationException rethrow before each Throwable catch to match the existing pattern used in GatewaySession and TalkModeManager. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
… and registration Remove synchronous bridge record write after server.listen() that races before the TCP server binds, and guard renew handler with server.listening check to prevent stale relay registrations. Closes #98650
Co-authored-by: cxbAsDev <[email protected]>
steipete
force-pushed
the
codex/land-small-fixes-batch-4-final
branch
from
July 5, 2026 18:00
d289884 to
71ef3db
Compare
clawsweeper
Bot
added
the
proof: sufficient
clawsweeper
Bot
added
the
merge-risk: 🚨 compatibility
Contributor
Author
|
Merged via squash.
|
This was referenced Jul 5, 2026
Closed
awaitPendingManagerWork silently swallows all errors from pending sync/provider init on close
#96766
Closed
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Jul 6, 2026
* fix(cron): reject sub-millisecond durations * fix(skill-workshop): preserve proposal terminal newline Preserve proposal_content exactly at the agent tool boundary and make renderProposalMarkdown defensively emit a terminal newline. Add focused regressions for the tool write path and markdown renderer. * fix(skill-workshop): reject blank raw proposal content * fix: treat empty-string optional integer tool params as unset Optional positive-integer tool params (e.g. Telegram replyTo/threadId) threw ToolInputError when a tool-calling model populated them with an empty-string or whitespace-only default. Those defaults carry no value, so readPositiveIntegerParam/readNonNegativeIntegerParam now treat a blank string as unset (undefined) instead of throwing, while still rejecting genuinely invalid present values (0, "42.5", "-3"). This prevents silent message-delivery failures when models emit empty routing-param defaults. Adds unit tests covering blank vs invalid. * fix(gateway): log start session persistence failures The gateway session-lifecycle "start" event persistence (persistGatewaySessionLifecycleEvent, which surfaces write failures via requireWriteSuccess) was fired as void ...catch(() => undefined), swallowing the rejection with zero logging. A failed start-marker write silently dropped the run's start record from restart-recovery accounting, with no operator-visible trace. The sibling terminal-phase catch already logs this since openclaw#97839; the start path was the unfixed sibling. Mirror that fix: log the swallowed start-phase persistence failure with the same redacted message shape via formatForLog, keeping the fire-and-forget semantics unchanged. Adds a focused regression test asserting the log fires on start-persist rejection. * fix(memory): report close-time pending work failures * fix(shared): return "" from sliceUtf16Safe when end <= start, matching native .slice sliceUtf16Safe silently swapped reversed bounds (to < from) instead of returning "" like String.prototype.slice, creating a subtle footgun for callers with dynamic start/end pairs. Caller scan across src/, extensions/, and packages/ confirmed no production code relies on the old swap behavior — all callers use (text, 0, N) or (text, -N) forms only. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix(plugins): require plugin manifest in npm verifier * fix(android): propagate CancellationException in CameraHandler catch blocks Catch (err: Throwable) swallows kotlinx.coroutines.CancellationException, breaking structured concurrency when the coroutine scope is cancelled during camera operations (handleList/handleSnap/handleClip). Add CancellationException rethrow before each Throwable catch to match the existing pattern used in GatewaySession and TalkModeManager. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> * fix(android): propagate CancellationException past GatewaySession invoke boundary * chore: sync native i18n inventory after gateway session line shift * fix(agents): prevent native hook relay bridge race condition on renew and registration Remove synchronous bridge record write after server.listen() that races before the TCP server binds, and guard renew handler with server.listening check to prevent stale relay registrations. Closes openclaw#98650 * fix: harden small reliability fixes Co-authored-by: cxbAsDev <[email protected]> * docs(agents): explain buffered LSP spawn failures * docs(agents): clarify LSP spawn timeout invariant --------- Co-authored-by: qingminlong <[email protected]> Co-authored-by: anyech <[email protected]> Co-authored-by: snotty <[email protected]> Co-authored-by: masatohoshino <[email protected]> Co-authored-by: lin-hongkuan <[email protected]> Co-authored-by: simon-w <[email protected]> Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: 宇宙熊Yzx <[email protected]> Co-authored-by: xialonglee <[email protected]> Co-authored-by: nankingjing <[email protected]> Co-authored-by: cxbAsDev <[email protected]>
This was referenced Jul 7, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Ten small, independently reported reliability defects were ready for maintainer cleanup but split across contributor branches. The original LSP patch also swallowed spawn failures and waited for timeout, while the Codex relay patch was described more broadly than its actual diagnostic cleanup.
Source PRs: #100311, #100293, #100273, #100313, #100308, #100014, #99904, #99916, #100300, #99922.
Why This Change Was Made
This takeover preserves the contributor implementations and credit, then adds maintainer fixups:
User Impact
Cron rejects sub-millisecond schedules, skill proposals retain their terminal newline, blank optional integer tool arguments behave as absent, persistence and memory close failures become visible, UTF-16 slicing matches native semantics, malformed plugin packages fail verification, Android cancellations propagate, LSP startup fails quickly, and native hook relay records are written only after the server is listening.
Evidence
GatewaySessionInvokeTest: 18 tests passed: https://github.com/openclaw/openclaw/actions/runs/28747804774@openclaw/[email protected]passed with its published manifest and runtime files.git diff --checkcodex-rs/hooks/src/engine/command_runner.rsandcodex-rs/hooks/src/events/pre_tool_use.rs.