Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR stabilizes timing-sensitive tests and rejects oversized SVG geometry before native decoding. Example: An A2UI image contains 65,537 bytes of path attribute data.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this collaborator-authored PR contains useful work absent from main, and both previous code findings are fixed; no new actionable code defect was found. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherA2UI images and agent avatars enter MediaResolver as image URLs; permitted SVG bytes pass through preprocessing before becoming native image sources or returning the existing failure result. flowchart LR
A[Image URL] --> B[MediaResolver]
B --> C[Encoding and DOCTYPE policy]
C --> D[Geometry and element budgets]
D --> E[Native SVG decoder]
D --> F[Image failure result]
E --> G[Native image source]
Technical reviewBest possible solution: Land the focused preprocessing mitigation and deterministic test repairs after scoped runtime proof, while retaining the broader native-decoder investigation. Do we have a high-confidence way to reproduce the issue? Main invokes native SVG parsing before its asynchronous timeout can bound synchronous work, matching the recorded UI-thread heartbeat and failure trace; this review did not execute a reproduction. Is this the best way to solve the issue? The smallest scoped repair keeps one preprocessing policy before the existing decoder and preserves behavioral assertions while repairing test scheduling; it is a mitigation rather than a complete bound on native rendering cost. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against b6f158ab257a. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Preflight sourceSource unknown. Source comparison at the observed PR snapshot only. Completion is self-reported; body, policy, tests, execution, and report bytes are not verified. Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (3 earlier review cycles)
Reviewed November 7, 2026, 1:24 AM ET / 06:24 UTC (Revision 4). |
Collaborator
Author
|
@clawsweeper re-review Pushed dbb5a18: SvgGeometryBudget now walks XML structure (skips comments, CDATA, PIs, DOCTYPE; reads d/points only in start tags) and UTF-16/UTF-32 SVGs are rejected before decode. Regression tests cover both comment cases from the P2 finding. |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Collaborator
Author
Hanselman dual-model review (initial, pre-fix state)Scope
ReviewersTwo separate
Reviewers could read repo files. They did not edit files, build, or run tests. Both models agree (HIGH consensus)None. The two reviews found different issues. Each was checked against the code by the coordinator. Only one model flagged (LOW consensus)
Notes
Suggested next stepFix the two SVG bypasses (stricter |
Collaborator
Author
Update: status after commit
|
| # | Finding | Status | Evidence |
|---|---|---|---|
| 1 | XML comment bypasses the SVG guard (Astra, HIGH) | Fixed (by reading) | Exceeds now walks XML structure. It skips comments, CDATA, processing instructions and DOCTYPE, and reads d/points only inside start tags with quoted values skipped. Tests added: Exceeds_CommentContainingAttributeLikeText_DoesNotHideRealAttribute, ..._OversizedAttributeInsideComment_IsIgnored, ..._CdataAndProcessingInstruction_AreSkipped, ..._QuotedValueContainingAttributeText_IsNotRescanned. |
| 2 | UTF-16 bypasses the guard (Opus, MEDIUM) | Fixed for UTF-16/UTF-32 | New IsUnsupportedEncoding rejects UTF-16/32 BOMs and any NUL in the first 8 bytes. MediaResolver.SvgFromBytesAsync calls it before the budget check. Test added. Other non-ASCII-compatible encodings (for example EBCDIC) are still not rejected. This is [INFERENCE] and unlikely to matter. |
| 3 | 64 KiB limit is unmeasured (Opus, MEDIUM) | Open | MaxAttributeChars is unchanged. No timing data, no env override. |
| 4 | CI_FAILURES.md overclaims "Fix" (Opus, MEDIUM) |
Mostly fixed | The bullet now reads "Mitigated for oversized path data" and says the entry stays open. Two gaps remain: the Next: line still says "check this entry off once the fix lands", and no test covers SVGs under the budget. |
| 5 | StopAsync test 10 s timer can mask a bug (Astra, MEDIUM) |
Fixed | The listener is started with CancellationToken.None. Only StopAsync can cancel the pending confirmation. |
| 6 | Reconnect test shares one 15 s timer (Opus, LOW) | Fixed | WaitForNextSocketAsync() has no timeout. WaitAsync(TransitionTimeout) is applied where it is awaited. |
| 7 | GatewayFixtureUiTests loses measured gap values on timeout (Opus, LOW) |
Open | Not touched in dbb5a184. |
| 8 | CI crash-capture step uses SilentlyContinue (Opus, LOW) |
Open | Not touched in dbb5a184. |
New observations (coordinator only, unverified by a second model)
- The new scanner looks linear. Every branch advances
i, andSkipPastreturns the end of input when a terminator is missing. - Prefixed attribute names such as
svg:dare not counted. This is a bypass only if D2D treats a prefixed attribute as the plaind. Untested. - A DOCTYPE internal subset that contains
>inside a quoted entity value ends the DOCTYPE skip early. The DOCTYPE is stripped before decode, so this looks harmless. Untested. - A UI-level test for the new "non-UTF-8 rejected" path in
MediaResolverwas not added. Only the pure helper is tested.
Next
Remaining before treating the SVG change as done: run ./build.ps1, the Shared and Tray test projects, and the A2UISvgTests UI lane on dbb5a184. Decide on findings 3, 7 and 8, and remove the stale Next: line in the CI_FAILURES.md SVG entry.
Models used for the review
| Reviewer | Model ID | Effort | Context |
|---|---|---|---|
| Opus | inference-nvidia/aws/anthropic/bedrock-claude-opus-5-5 |
high | 1M |
| Astra | inference-nvidia/azure/openai/gpt-6-astra |
high | 1.1M |
Both ran on the first review (head 2566c57 plus working tree). This update was assessed by the coordinator only (inference-nvidia/aws/anthropic/bedrock-claude-sonnet-5-5); no reviewer model was run on dbb5a184.
larroy
requested review from
bkudiess and
karkarl
and removed request for
karkarl
November 5, 2026 22:45
17 tasks
Collaborator
Author
Hanselman delta review of
|
| # | Status |
|---|---|
| 1, 2, 5, 6 | Fixed |
| 3, 7, 8 | Open |
| 4 | Partial. The Next: check this entry off once the fix lands line still contradicts "Mitigated", and no test reaches the SVG decode timed out branch. |
New findings
| Issue | Opus | Astra | Notes |
|---|---|---|---|
DOCTYPE internal subset bypasses the guard (HIGH consensus). Exceeds runs on the raw bytes and skips a <!...> declaration to its first >. MediaResolver then calls StripDoctype on the bytes D2D receives, so the scan and the strip disagree. Opus payload: <!DOCTYPE x [ > <a b=" ]><svg ...><path d="...huge..."/></svg>. Astra payload: <!DOCTYPE svg [<!-- > <![CDATA[ -->]> before an oversized path. Either way the real path is never counted and D2D receives a clean oversized SVG. |
HIGH | HIGH | Verified by the coordinator: MediaResolver.cs runs the budget check before StripDoctype (line 345), and Exceeds ends a <! declaration at the first > (SvgGeometryBudget.cs:41). Neither payload was executed in C#; Opus ran a line-for-line Python copy. Fix confidence 90%: run the scan on the stripped bytes, reject any remaining <!DOCTYPE, and add the payloads as regression tests. |
Activation test leaks the listener and pipe when setup fails (no try/finally after the timer was removed). |
LOW | LOW | Both flagged it. Fix confidence 95%. |
encoding= in the XML declaration (for example UTF-7, ISO-2022-JP) passes IsUnsupportedEncoding, so the helper does not enforce UTF-8 as the commit title says. |
MEDIUM | LOW (stated as a limit) | Native acceptance is [INFERENCE]. Fix: reject any encoding= other than UTF-8 or US-ASCII. |
StripDoctype removes a decoy <!DOCTYPE inside a comment and leaves the real DTD, so entity expansion could defeat the budget. Existing bug, now relevant. |
MEDIUM | not flagged | Covered by the same fix as the DOCTYPE bypass. Needs D2D entity behavior to matter ([INFERENCE]). |
Prefixed attribute names (s:d) are not counted. |
LOW | LOW (stated as a limit) | Matching on the local name only adds false positives. |
| UTF-32 BE BOM branch is dead code (the NUL check already catches it). | LOW | not flagged | Trivial. |
Unit tests call Exceeds on raw bytes and never go through StripDoctype, which is why the DOCTYPE bypass was missed. |
LOW | LOW | Move the strip into the pure file and test the combined path. |
| Crash-capture log window (30 min) can include the Functional UI Tests testhost. | LOW | not flagged | Record a start time before the Tray UI step. |
No limit on element count (about 70,000 <rect/> fit in 2 MiB), so the mitigation is not a render-time bound. |
note | note | Already documented. Do not call the entry fixed. |
Performance: both models judge the new scanner linear time with no non-advancing loop.
Verdict (both models): not ready. Fix the DOCTYPE bypass first. A stronger option from Opus is to use XmlReader with DTD processing prohibited on the stripped bytes.
Collaborator
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Collaborator
Author
Hanselman dual-model review (adversarial)Review of the current branch state, post-rebase. Both requested models completed on the same revision.
Note: the reviewed head is 2 commits ahead of this PR's current head ( Both models agree: HIGH consensus
No CRITICAL or HIGH findings from either model. GPT verdict: correct for its stated scope. Opus verdict: strip-then-scan design sound; the remaining exposure is what the budget does not measure. Only one model flagged: LOW consensus (all Opus)
Taken from the HIGH itemThe element-count part is being addressed: an element start-tag budget ( Measured locally on the pre-decode scan: an icon-sized SVG (20 shapes + 1 path) and a 500-shape chart are accepted; 8,192 start tags are accepted and 8,193 rejected in about 5 ms. Validation on the reviewed head
A clean source review does not replace CI on the pushed head; the checks on this PR ran on |
larroy
force-pushed
the
fix/ci-flaky-tests-2026-11-04
branch
from
November 7, 2026 05:31
e4685fc to
d9293b5
Compare
- ChatComposerControllerTests: run voice-dispose reentrancy work on dedicated threads (thread-pool starvation) with an env-overridable bound. - ActivationRouterTests: replace hard-coded 2 s waits with a named constant. - GatewayProtocolLiveRoundTripTests: observe the drop via StatusChanged and withhold the reconnect challenge instead of polling a ~1 s window. - GatewayFixtureUiTests: wait for settled sidebar geometry after unpinning. - MediaResolver: reject SVGs over a 64 KiB path-data budget before D2D decode (SvgGeometryBudget), so pathological geometry cannot block the UI thread. - CI: capture testhost crash event-log records when Tray UI tests fail. - docs/CI_FAILURES.md: update crash and SVG entries; record UIA timeout cluster.
Skip comments/CDATA/PIs/DOCTYPE and read d/points only in start tags; reject UTF-16/UTF-32 SVGs before decode. Tighten StopAsync and reconnect test timers; reword CI_FAILURES SVG entry as mitigated.
…StepTimeout Fixes flaky PatchConfigDetailedAsync_RejectedSave_AfterTimeoutDoesNotLogRemoteText on loaded CI runners. Override: OPENCLAW_TEST_GATEWAY_STEP_TIMEOUT_SECONDS.
The budget scan ends a <!...> declaration at its first '>', while StripDoctype honors the DTD internal subset, and the scan ran on the unstripped bytes. A DOCTYPE comment containing '>' therefore made the scan disagree with what D2D parses: it could reject a small SVG (counting a path inside the DTD comment) or miss real oversized geometry. StripDoctype and its ASCII helpers now live in SvgGeometryBudget, and TryPrepareSvgForDecode is the single pre-decode policy (encoding check, strip, budget) so MediaResolver decodes exactly the bytes that were scanned. The duplicate late strip is gone. Both halves are covered through the real entry point: unit tests on the combined policy and UI tests against D2D for the false-rejection payload and for oversized geometry that follows a DOCTYPE. Also dispose the timer-free activation listener on every exit path (await using), so a failing assertion can no longer leave the listener task and named pipe alive; the timer-free token needed by the shutdown assertion is unchanged.
StripDoctype removes the first textual <!DOCTYPE, so a decoy inside a comment or processing instruction before the real declaration leaves the DTD in the bytes handed to D2D. The geometry scan then ends that DTD at its first '>', which can hide real oversized geometry. TryPrepareSvgForDecode now fails closed when a declaration survives the strip, so Exceeds only ever sees bytes without one. The tradeoff is documented: an SVG keeping a literal <!DOCTYPE in its own comment is rejected instead of decoded. Test repairs from review: - The oversized-after-DOCTYPE unit and UI tests used a payload the raw scan already rejected, so they proved nothing about the ordering fix. They now use a '<?' DTD tail that makes the unprefixed scan skip to end of input, and pin that miss before asserting rejection. - New coverage for a decoy DOCTYPE that survives the strip, at the policy level and through MediaResolver/D2D. - Drop the Exceeds assertion that locked in the raw scan's DTD flaw. - ActivationRouter shutdown test: replace await using with try/finally so the failure path this test guards returns a bounded TimeoutException instead of hanging on the same unbounded stop task.
The pre-decode guard summed only d/points attribute text, so a document of many cheap shapes reached SetSourceAsync with a measured size of zero while D2D still had to parse every node. A 2 MiB data URL holds about 65,000 <rect/> elements, and the 8 MiB remote cap about 200,000. Count element start tags in the same single-pass scan and reject above MaxElements (8,192). End tags, comments, CDATA and processing instructions do not count, matching how the attribute budget already treats them. The limits are now reported as a typed SvgBudgetLimit so the caller can log which budget tripped and tests can assert it directly. Measured on the scan: an icon (20 shapes, 1 path) and a 500-shape chart are accepted; 8,192 start tags are accepted and 8,193 rejected in about 5 ms; 65,536 rects, or the same text inside a comment, behave accordingly. Coverage: unit boundary and structure tests, a policy-level rejection, and a UI test that a many-shape SVG is rejected before D2D. CI_FAILURES scope now names path data and element counts and keeps the entry open for input under both limits.
larroy
force-pushed
the
fix/ci-flaky-tests-2026-11-04
branch
from
November 7, 2026 05:47
d9293b5 to
ec5b9ed
Compare
larroy
added
status: 👀 ready for maintainer look
Collaborator
Author
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
This branch has not been deployed
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
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.
Related: #2000, #1981
What Problem This Solves
Fixes: Build and Test fails on unrelated PRs and
mainpushes when hosted runners are loaded. Separately, a pathological A2UI SVG can freeze the chat UI thread for longer than the 8 s render cap.User Impact
User impact: CI stops failing intermittently on four tests that were timing-sensitive. An A2UI SVG with more than 64 KiB of path data now shows the broken-image fallback immediately instead of freezing the UI. Ordinary SVGs are unaffected: the largest SVG still allowed decodes in about 0.2–0.3 s locally.
Why This Change Was Made
Each failure was traced to a cause before any test was changed, and none of the assertions were loosened:
ChatComposerControllerTests.Dispose_VoiceCancellationCallbackCanWaitForReentrantControllerWork: the test blocked shared thread-pool threads that its own cleanup work needed. It now uses dedicated threads, the same fix the sibling lifetime test already had.ActivationRouterTests.StopAsync_CancelsAndDrainsInFlightDirectConfirmation: hard-coded 2 s waits now use a named constant (default 10 s,OPENCLAW_TRAY_TEST_ACTIVATION_WAIT_SECONDS).GatewayProtocolLiveRoundTripTests.HandshakeGate_Reconnect_SuppressesEarlyMutationThenSendsAfterHelloOk: polling could miss the roughly 1 s window before the client reconnects. The test now waits for theDisconnectedevent, and the mock server holds the reconnect by not sending the challenge on the second socket until the test sends it.GatewayFixtureUiTests.HomeSidebar_CustomizeDoesNotNavigate_AndHomeRestoresCanonicalConversation: the sidebar geometry was read before layout settled after unpinning. The test now waits for the settled layout and keeps the same pixel ranges.SvgImageSource.SetSourceAsynccan block the UI thread pastSvgRenderTimeout, because the timeout stops only the wait, not the parse. The newSvgGeometryBudgetrejects oversizedd/pointspath data before decode.ReactorChatLayoutProofTestsflyout tests): diagnostics only. When Tray UI tests fail, a new CI step saves the crash entries from the Windows event log (crashing module, exception code, offset; no memory dumps) toTrayUI/wer-testhost-events.txtin thetest-results-uiartifact. The test was not reordered: closing the flyout first would remove the live-update behavior thatSandboxFlyout_*tests.HistoricalFractionalVoiceConversationRendersWithoutLoadingPlaceholder: not changed here; improve(workspace): share one footer with Dashboard and notifications across Chat and Settings #1996 fixed it onmainafter these failing runs.docs/CI_FAILURES.mdnow covers the crash investigation (addsSandboxFlyout_*, the run links, and the new crash-records file), notes the SVG fix, and adds an entry for the cluster of UI Automation "Operation timed out" errors in run 41786895448.Evidence
GatewayProtocolLiveRoundTripTestsclass with all 32 CPUs busy (24 tests per run). The trace showssocket #2 accepted; challenge delivered.PathologicalGeometry_RejectedBeforeDecodefinishes in 30–42 ms (5/5 runs ofA2UISvgTests). The largest SVG still allowed (3,560 path segments, 65,608 bytes) decodes in 204–319 ms.Assert.Equal), not the new geometry wait. The next 16 runs all passed.main(unchanged by this PR).Change Type
Scope
winnodeRequired proof pools
none: the changes are test stabilization, a pure SVG input guard covered by UI tests and unit tests, and a CI diagnostic step. No custom Windows proof pool is needed.Validation
./build.ps1: all builds succeeded.dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj: 5070/5070 passed.dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj(withOPENCLAW_REPO_ROOTset): 4927 passed, 1 failed, 24 skipped.MarkdownParserFuzzTests.Build_ManyListItems_ScalingCurve, a timing limit (642 ms against 500 ms) that this PR does not touch; it is already indocs/CI_FAILURES.md. It passes when run alone (3/3).GatewayHandshakeObservationTests.RefusedTransport_ReportsNetworkWithoutClassifyingStackFrames(gotServerinstead ofNetwork). It also passes alone (3/3), and this PR doesn't touch it.mainis not confirmed locally.ChatComposerControllerTests.Dispose_*withActivationRouterTests: 30/30.GatewayProtocolLiveRoundTripTestsunder full CPU load: 30/30.A2UISvgGeometryBudgetTests: 8/8.A2UISvgTests: 5 runs, all passed.pwsh ./scripts/test-ci-workflow-contract.ps1: passed. The new crash-capture step's script also ran locally and wrote its output file.Real Behavior Proof
b880a642.OPENCLAW_RUN_GATEWAY_FIXTURE_UI=1and isolated tray data directories;A2UISvgTestswith isolated UI-test directories.nullin about 35 ms with the warningSVG rejected before decode. The sidebar and fractional voice fixture tests pass against the real app.Security Impact
Yes, explain the risk and mitigation: N/A. The CI step reads onlytesthostcrash entries from the Application event log (Application Error, WER and .NET Runtime). It writes no memory dumps, so the existing no-dumps rule for secret safety still holds.Compatibility and Migration
d/pointspath data now show the broken-image fallback.OPENCLAW_TRAY_TEST_REENTRANCY_TIMEOUT_SECONDS,OPENCLAW_TRAY_TEST_ACTIVATION_WAIT_SECONDSandOPENCLAW_TEST_GATEWAY_TRANSITION_TIMEOUT_SECONDS.Review Conversations