feat(tools): add native sd and comby tools - #39
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @trotsky1997, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![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: 3bd1ed68ee
ℹ️ 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".
|
|
||
| let mut args = Vec::new(); | ||
| if params.string_mode { | ||
| args.push("--string-mode".to_string()); |
There was a problem hiding this comment.
When string_mode is true this passes --string-mode, but the upstream sd CLI documents literal mode as -F/--fixed-strings (see chmln/sd README Quick Guide: “Use -F or --fixed-strings”). With a real sd binary, every literal replacement request will fail during argument parsing instead of running the advertised safe literal find/replace.
Useful? React with 👍 / 👎.
| args.push(params.find); | ||
| args.push(params.replace); |
There was a problem hiding this comment.
For requests where find or replace starts with - (for example replacing foo with -w), these positionals are passed without a -- separator, so sd parses them as flags and errors; the upstream README's Edge cases section says every argument starting with - is interpreted as a flag and -- is the required end-of-flags marker. This breaks valid replacements that are common in generated CLI text or option examples.
Useful? React with 👍 / 👎.
| args.extend(params.extra_args); | ||
| args.push(params.find); | ||
| args.push(params.replace); | ||
| args.extend(params.paths); |
There was a problem hiding this comment.
The new tool schema and docs advertise paths as “files or glob patterns” with examples like src/**/*.rs, but Command::args bypasses the shell and this line hands those glob strings to sd unchanged. The upstream sd usage is [files]... and its project-wide examples rely on shell expansion/fd/find, so callers using the documented glob form will either edit nothing or fail unless a literal file named like the glob exists.
Useful? React with 👍 / 👎.
| return Err(anyhow!("comby requires a non-empty match_template")); | ||
| } | ||
|
|
||
| let rewrite_template = non_empty(params.rewrite_template); |
There was a problem hiding this comment.
When a caller uses rewrite_template: "" to delete matched code, or intentionally uses leading/trailing whitespace in the replacement, this converts the template through non_empty, so empty rewrites are rejected as invalid_request and surrounding whitespace is stripped before spawning comby. The comby CLI accepts the second positional rewrite template as a real argument (the docs even show '' as that positional for match-only), so this blocks valid structural deletion/formatting rewrites.
Useful? React with 👍 / 👎.
| } | ||
| if let Some(matcher) = non_empty(params.matcher) { | ||
| args.push("-matcher".to_string()); | ||
| args.push(matcher); |
There was a problem hiding this comment.
When callers follow this tool's schema examples and pass matcher: "rust" or "python", this forwards that value directly to -matcher, but comby documents forcing a parser with extension-style values such as -matcher .js or -matcher .c. In that documented-name scenario the wrapper builds an argv that a real comby binary does not recognize as the intended language matcher, so language-aware searches run incorrectly or fail.
Useful? React with 👍 / 👎.
| args.push("-include-files".to_string()); | ||
| args.push(include_files); | ||
| } | ||
| if let Some(exclude_files) = non_empty(params.exclude_files) { | ||
| args.push("-exclude-files".to_string()); |
There was a problem hiding this comment.
When include_files or exclude_files is supplied, the wrapper emits -include-files/-exclude-files, but the Comby cheat sheet/help documents file filtering with flags such as -f, -exclude, and -exclude-dir rather than these names. In the advertised filtered-check/rewrite scenarios, a real comby invocation will fail option parsing instead of applying the requested filter.
Useful? React with 👍 / 👎.
| if rendered.len() <= max_output_bytes || best == 0 { | ||
| return Ok(rendered); |
There was a problem hiding this comment.
If an sd run emits little stdout but a large stderr and the caller sets a small max_output_bytes, the binary search can leave best == 0; after trimming stderr once, this branch returns even when the JSON is still larger than the configured cap. That violates the documented bounded envelope for real failures with verbose diagnostics, so downstream clients can still receive oversized tool results.
Useful? React with 👍 / 👎.
Co-authored-by: multica-agent <github@multica.ai>
3bd1ed6 to
93878d1
Compare
1 participant
Summary
Verification
Please request review from the other top model reviewer for this PR.