fix: ensure assistantTexts populated at message_end for custom providers (#69410)#69724
fix: ensure assistantTexts populated at message_end for custom providers (#69410)#69724EronFan wants to merge 4 commits into
Conversation
openclaw-barnacle
Bot
added
agents
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryThis PR fixes a bug where custom OpenAI-compatible providers that omit Confidence Score: 5/5Safe to merge; all findings are minor style suggestions with no blocking logic issues. The core fix is correct and well-tested: converting No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/browser/src/browser/pw-tools-core.snapshot.ts
Line: 65
Comment:
**`nth` field not forwarded when storing aria snapshot nodes**
`AriaSnapshotNode` does not currently include `nth`, so this mapping is complete for the moment. However, `storeAriaSnapshotNodes` and `refLocator` already support `nth` for disambiguation, and if the AX-tree pipeline ever adds it to the type, the field would be silently dropped here. Consider documenting the intentional omission with a comment so reviewers aren't left wondering.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: ensure assistantTexts populated at ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| page, | ||
| cdpUrl: opts.cdpUrl, | ||
| targetId: opts.targetId, | ||
| nodes: formatted.map((n) => ({ ref: n.ref, role: n.role, name: n.name || undefined })), |
There was a problem hiding this comment.
nth field not forwarded when storing aria snapshot nodes
AriaSnapshotNode does not currently include nth, so this mapping is complete for the moment. However, storeAriaSnapshotNodes and refLocator already support nth for disambiguation, and if the AX-tree pipeline ever adds it to the type, the field would be silently dropped here. Consider documenting the intentional omission with a comment so reviewers aren't left wondering.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/browser/src/browser/pw-tools-core.snapshot.ts
Line: 65
Comment:
**`nth` field not forwarded when storing aria snapshot nodes**
`AriaSnapshotNode` does not currently include `nth`, so this mapping is complete for the moment. However, `storeAriaSnapshotNodes` and `refLocator` already support `nth` for disambiguation, and if the AX-tree pipeline ever adds it to the type, the field would be silently dropped here. Consider documenting the intentional omission with a comment so reviewers aren't left wondering.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: 8979096ea4
ℹ️ 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".
| page, | ||
| cdpUrl: opts.cdpUrl, | ||
| targetId: opts.targetId, | ||
| nodes: formatted.map((n) => ({ ref: n.ref, role: n.role, name: n.name || undefined })), |
There was a problem hiding this comment.
snapshotAriaViaPlaywright stores each ax<number> entry with only {ref, role, name}, but refLocator can only disambiguate duplicate role/name matches when nth is present. In pages with repeated accessible names (e.g. two "Submit" buttons), both refs resolve to the same getByRole(...) locator set, which can produce strict-mode failures or target the wrong element. This breaks the new ax-ref resolution path for common UIs with duplicate names.
Useful? React with 👍 / 👎.
| roleRefsByTarget.set(roleRefsKey(opts.cdpUrl, targetId), { | ||
| refs: {}, | ||
| ariaSnapshotNodes: opts.nodes, | ||
| }); |
There was a problem hiding this comment.
storeAriaSnapshotNodes inserts a new roleRefsByTarget entry when none exists, but unlike rememberRoleRefsForTarget it never applies the MAX_ROLE_REFS_CACHE eviction loop. Repeated aria snapshots across many targets can therefore grow this map unbounded (bypassing the intended size limit of 50), which is a memory regression introduced by the new cache write path.
Useful? React with 👍 / 👎.
| mounts.push({ | ||
| containerRoot: parsed.containerRoot, | ||
| hostRoot: parsed.hostRoot, | ||
| writable: parsed.writable, | ||
| source: "bind", |
There was a problem hiding this comment.
This adds bind mounts to getMounts, but resolveMountByContainerPath still chooses mounts only by containerRoot length; equal-length roots keep insertion order, so the default workspace mount wins over a custom bind mounted at the same path (e.g. /workspace). That causes path resolution and writable checks to ignore the override mount, which can route operations against the wrong effective mount configuration.
Useful? React with 👍 / 👎.
|
The core fix for #69410 looks correct: making the message_end text append unconditional in finalizeAssistantTexts() should prevent the payloads=0 regression for custom OpenAI-compatible providers. That said, this PR is broader than the bug fix and includes unrelated browser/sandbox changes. I’d recommend splitting those out into follow-up PRs, since they introduce separate risks:
Suggestion: land the minimal pi-embedded-subscribe.ts fix + regression test first, then revisit the unrelated changes separately. |
2 participants
问题
#69410：Custom provider 返回有效内容但 agent payloads=0。
根因
finalizeAssistantTexts中当onBlockReply存在时（标准 agent 调用），else if (!addedDuringMessage && !chunkerHasBuffered && text)不满足，因为第一个分支条件不满足。custom provider 场景下流式文本未加入assistantTexts，message_end 提取的文本两个分支都不满足 →assistantTexts空 →payloads=0修复
将
else if改为独立if，确保 message_end 文本无条件加入assistantTexts。测试覆盖
3 tests passed (21ms).