fix(agents): validate context engine assemble result shape#87549
Merged
steipete merged 1 commit intoMay 31, 2026
Conversation
Contributor
|
Codex review: needs maintainer review before merge. Reviewed May 29, 2026, 2:18 AM ET / 06:18 UTC. Summary PR surface: Source +40, Tests +61. Total +101 across 2 files. Reproducibility: yes. By source inspection, current main returns a raw truthy assemble payload and the runner assigns Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused harness boundary guard after maintainer acceptance of the scoped messages-array validation, or expand the validator to the full Do we have a high-confidence way to reproduce the issue? Yes. By source inspection, current main returns a raw truthy assemble payload and the runner assigns Is this the best way to solve the issue? Yes, if maintainers accept the compatibility scope. The shared harness boundary covers embedded and Codex callers and uses existing try/catch fallback behavior, while broader AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 8d63d466b858. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +40, Tests +61. Total +101 across 2 files. View PR surface stats
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
|
Contributor
|
ClawSweeper PR egg: ✨ hatched 🥚 common Gilded Review Wisp. Rarity: 🥚 common. Trait: collects tiny proofs. DetailsShare on X: post this hatch
About:
|
Context engine plugins that returned an object without a `messages` array (for example a malformed lossless-claw assemble payload) previously poisoned activeSession.messages with `undefined`. Downstream prompt assembly then crashed with `Cannot read properties of undefined (reading 'length')`, aborting the turn before any model call. Validate the assemble() return shape at the harness boundary so an invalid result throws a descriptive error naming the offending engine. The runner's existing assemble try/catch already logs the failure and falls back to the unmodified pipeline messages, so a buggy plugin can no longer corrupt session state. Fixes openclaw#75541 Co-authored-by: Cursor <cursoragent@cursor.com>
0938413 to
5b6b7b1
Compare
Contributor
|
Maintainer update before landing:
Local proof on the rebased head:
Fresh GitHub checks started for the rebased head:
Known proof gap: I did not install the third-party |
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
4 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.
Summary
assembleHarnessContextEngineboundary against context engine plugins that return a malformed assemble result, so a buggy plugin can no longer poisonactiveSession.messageswithundefinedand crash the prompt assembly stage withCannot read properties of undefined (reading 'length').attempt.tsalready logs the failure and falls back to the unmodified pipeline messages, so behavior on a well-formed plugin is unchanged.Linked context
Closes #75541
Was this requested by a maintainer or owner?
No direct maintainer request; ClawSweeper triage on the issue flagged this as a
safe repair candidateat the harness boundary.Real behavior proof (required for external PRs)
Behavior or issue addressed: With a third-party context engine (lossless-claw v0.9.2 in the report) that returns
{ estimatedTokens: 0 }without amessagesarray, OpenClaw previously assignedundefinedtoactiveSession.agent.state.messagesand then crashed insideattempt.tswhen it readactiveSession.messages.length.Real environment tested: Local OpenClaw source checkout on Linux with Node
v22.22.3, pnpm11.2.2via corepack.Exact steps or command run after this patch: Ran two local OpenClaw source smokes with
PATH=\"\$HOME/.nvm/versions/node/v22.22.3/bin:\$PATH\" node --import tsx --input-type=module -e '...'against the realassembleHarnessContextEngine, once before the patch (viagit stashof the change) and once after, using alossless-claw-shaped plugin that omitsmessages.Evidence after fix: Copied console output from those local OpenClaw source smokes.
Before-fix (with the patch temporarily stashed on the same branch):
After-fix (working tree restored to this patch):
Observed result after fix: The malformed plugin payload throws a descriptive error at the harness boundary before any state assignment, naming the engine id. A well-formed
AssembleResultstill passes through unchanged. The runner's existingtry { ... } catch (assembleErr) { log.warn('context engine assemble failed, using pipeline messages: ...') }insrc/agents/embedded-agent-runner/run/attempt.tsthen logs the failure and falls back to the unmodified pipeline messages.What was not tested: A live OpenClaw session that actually installs and runs lossless-claw v0.9.2 against the embedded runner. The fix targets the runtime boundary, not the third-party plugin itself.
Proof limitations or environment constraints: The after-fix proof uses the real OpenClaw harness wrapper on the real
AssembleResultconsumer path, but it does not install the lossless-claw plugin because this environment is not configured with that plugin's runtime. The malformed shape used in the smoke matches the exact shape ClawSweeper identified as crash-prone in the issue triage.Before evidence (optional but encouraged): Captured directly above via the same script after
git stash-ing this patch.Tests and validation
Which commands did you run?
PATH=\"\$HOME/.nvm/versions/node/v22.22.3/bin:\$PATH\" node scripts/test-projects.mjs src/agents/harness/context-engine-lifecycle.test.tsPATH=\"\$HOME/.nvm/versions/node/v22.22.3/bin:\$PATH\" node scripts/test-projects.mjs src/agents/embedded-agent-runner/run/attempt.spawn-workspace.context-engine.test.tsPATH=\"\$HOME/.nvm/versions/node/v22.22.3/bin:\$PATH\" corepack pnpm exec oxfmt --check src/agents/harness/context-engine-lifecycle.ts src/agents/harness/context-engine-lifecycle.test.tsgit diff --checkOutputs:
context-engine-lifecycle.test.ts:Test Files 1 passed (1)/Tests 9 passed (9)(3 existing + 6 new shape-validation cases).attempt.spawn-workspace.context-engine.test.ts:Test Files 1 passed (1)/Tests 53 passed (53)— no regression in the existing adjacent assemble integration coverage.oxfmt --check:All matched files use the correct format.What regression coverage was added or updated?
Added a
describe(\"assembleHarnessContextEngine result validation\")block covering: well-formed passthrough,undefinedresult,nullresult,{ estimatedTokens: 0 }missingmessages(the exact lossless-claw shape from #75541),messages: \"string\"(wrong type), andmessages: null. Each rejection asserts that the engine id appears in the thrown message so the runner's existing assemble-failure log carries actionable context.What failed before this fix, if known?
The before-fix smoke above reproduces
Cannot read properties of undefined (reading 'length'). Without this guard, the new tests' rejection assertions would not be possible because the wrapper previously returned the malformed object unchanged.If no test was added, why not?
N/A.
Risk checklist
Did user-visible behavior change? (
Yes/No)Yes (recovery semantics). A plugin returning a malformed assemble result now produces a runtime warning naming the engine and falls back to pipeline messages, instead of crashing the turn. Behavior on a well-formed
AssembleResultis unchanged.Did config, environment, or migration behavior change? (
Yes/No)No.
Did security, auth, secrets, network, or tool execution behavior change? (
Yes/No)No.
What is the highest-risk area?
A previously crashing plugin will now silently fall back to pipeline messages, potentially masking a real plugin bug behind a single warn log.
How is that risk mitigated?
The thrown error string is descriptive and names the offending engine id, so the existing
log.warn(\"context engine assemble failed, using pipeline messages: ...\")inattempt.tssurfaces both the engine id and the exact shape mismatch. Validation is strict enough to reject the failure mode in #75541 but only enforces what the publicAssembleResulttype already requires.Current review state
What is the next action?
Await maintainer / ClawSweeper review on whether the validator scope (only
messagesshape) is the preferred boundary, or whetherestimatedTokensshould also be validated.What is still waiting on author, maintainer, CI, or external proof?
A live install of the third-party lossless-claw plugin is still not provided from this environment.
Which bot or reviewer comments were addressed?
Initial PR; no PR comments yet.
AI-assisted: yes. I reviewed the touched code path and ran the focused validation commands above.
Made with Cursor