add grep tool result formatter#953
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
juniper-shopify
left a comment
There was a problem hiding this comment.
The overall structure is great — mirroring the GLOB formatter's partition + NOTE + ok_line pattern keeps the stack consistent and readable. The doc comment is thorough with clear examples.
One change needed: the match-detection heuristic is too loose and will misclassify some status messages as grep matches. Details in the inline comment.
| #: () -> String | ||
| def format_grep | ||
| lines = content.to_s.lines.map(&:strip).reject(&:empty?) | ||
| matches, notes = lines.partition { |line| line.include?("/") || line.match?(/\d:/) } |
There was a problem hiding this comment.
The match-detection heuristic here needs tightening. The current logic:
line.include?("/") || line.match?(/\d:/)…is broad enough to misclassify several types of non-match lines as matches:
line.include?("/")would match:"Found 0/100 files","N/A","true/false"line.match?(/\d:/)would match:"Step 2: install deps","Completed in 1:23","Error at 3:00pm"
Real grep output follows a consistent structure: either path/to/file:42:matched content (with file path) or 42:matched content (line-number-only, single-file mode). Is this what you're seeing from the grep tool message output as well, or does it look different?
A tighter heuristic that anchors to the start of the line:
line.match?(/\A\S+:\d+:/) || line.match?(/\A\d+:/)This matches path:line:content and line:content formats while rejecting status messages that happen to contain / or digit: in the middle. You could also combine them:
line.match?(/\A(?:\S+:)?\d+:/)| assert_equal "GLOB OK 1 file found · NOTE #{"x" * (Claude::ToolResult::TRUNCATE_LIMIT - 3)}...", output | ||
| end | ||
|
|
||
| test "format_grep reports the number of matches" do |
There was a problem hiding this comment.
The happy-path tests are solid — good coverage of the format variations. To go with the heuristic tightening above, it'd be valuable to add a test that verifies a status message containing / or digit: is correctly classified as a note rather than a match. For example:
test "format_grep does not misclassify a status message containing a slash as a match" do
tool_use_message = Claude::Messages::ToolUseMessage.new(
type: :tool_use,
hash: { name: "grep", input: {} },
)
tool_result = Claude::ToolResult.new(
tool_use: tool_use_message,
content: "lib/a.rb:1:foo\nFound 0/100 files",
is_error: false,
)
output = tool_result.format
assert_equal "GREP OK 1 match · NOTE Found 0/100 files", output
endThis kind of "adversarial" test is a good general practice for heuristic-based parsing — it documents the boundary between what the code considers data vs. metadata.
2cdf530 to
c1c5f21
Compare
b1a7b04 to
8703607
Compare
8703607 to
e9eaf19
Compare
c1c5f21 to
8b0e6ba
Compare

No description provided.