feat: include source location in list_console_messages#2060
feat: include source location in list_console_messages#2060masamaru0513 wants to merge 3 commits into
Conversation
OrKoN
left a comment
There was a problem hiding this comment.
per #903 we do want to have source maps locations. Additionally, we had non-source mapped location previously and received complains that with many console messages it consumes to many tokens as the URLs can be long and repeating. So we need to be smart about how we display it and take the grouping of console messages into account. In general, agents can make multiple tool calls at once so over the long term calling get_console_message multiple times should be less of a problem. With that, let's make sure use source location for grouping and that we show source mapped location.
|
Thanks for the feedback. To make sure I go in the right direction:
Based on your review, I'll go ahead and:
Will push an update. |
Address reviewer feedback on PR ChromeDevTools#2060: - Resolve source-mapped location via the stack trace's top non-ignored frame using uiSourceCode.uiLocation(), similar to the approach in ChromeDevTools#960. - Use the short display name (filename:line:col) for the concise text output to keep token usage low; full URL is preserved in the structured JSON output. - Include source location in the grouping key so that messages with identical text but different origins do not collapse together. - Bound stack-trace resolution with a timeout so a hanging page (e.g. blocked on a dialog) does not stall the tool call; falls back to the raw msg.location() / exceptionDetails location. - Split SymbolizedError.fromDetails' `includeStackAndCause` into `includeStack` and `includeCause` so the formatter can fetch the stack for location resolution without paying for cause lookup.
4ffbc86 to
2e214ea
Compare
|
@OrKoN one self-review note before you look — I added a few things that may be out of scope for this PR:
Let me know which of these you want trimmed and I'll push a focused update. |
- Add location (filename:line:col) to list_console_messages output - Source-mapped location with shortened display (shortUrl) - Handle pptr: protocol and encoded URLs - Remove deprecated includeStackAndCause option
2e214ea to
84d6f62
Compare
|
@OrKoN Addressed your feedback. Reverted the Would appreciate another look when you get a chance. |
|
@OrKoN Could you approve the workflows so CI can run? Happy to address anything that comes up. |
|
Hi @OrKoN, sorry to bother you again. The CI failure on the previous run was in an unrelated test ( |
|
@masamaru0513 please do not rebase the PR for now, as it will reset the test results. |
|
@szuend @Lightning00Blade could you please review and see if it makes sense to land this? |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@Lightning00Blade Gentle ping — OrKoN requested your review here on May 29. Would you have a chance to take a look when you get a moment? Happy to address any feedback. Thanks! |
|
This PR is on my ToDo list for review. Unfortunately it's very large, with non-trivial changes. This will take some time to properly review. |
|
Thanks for the heads up — happy to wait! |
Summary
Include source-mapped location in
list_console_messageswith shortened display and grouping support.Fixes #903
Changes since review feedback
Addressed @OrKoN's feedback:
uiSourceCode.uiLocation()) instead of raw CDP locationsapp.js:42:15instead ofhttps://example.com/static/js/app.js:42:15). Handles puppeteer-style wrappers and percent-encoded URLs in a single non-recursive pass.[N times]Output example
Design
list_console_messages: Shows shortened source-mapped location (filename:line:col)get_console_message: Shows full detail withLocation:line +### Stack tracesectionstructuredContent: Includeslocationobject withurl,displayName,lineNumber,columnNumbertype + text + argCount + location— identical messages from the same source are collapsedCompatibility
No breaking changes to existing tool outputs or public APIs. The new
locationfield is additive instructuredContent.Why the timeout wrapper is part of this PR
This PR resolves the stack trace eagerly on every
list_console_messagescall (not just whenfetchDetailedData=true) to derive the source-mapped location. As a side effect, pages paused on a dialog can hang the underlying CDP calls.The timeout wrapper bounds that work so the tool call always returns. The existing
when dialog is opentest (#1977) covers this scenario; without the wrapper it times out at 120s.So the wrapper is not a separate concern — it's required by the eager stack resolution that makes the location feature work.
Test plan
script.js:1:18(not full pptr URL)[5 times]get_console_messageshows ID, Location, Stack trace