agent: Stop over-escaping dashes in tool_permissions regex patterns - #51603
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @mvanhorn on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement, and we don't have @mvanhorn on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Thanks for adding this, and sorry for the slow reply. Are you able to rebase this? |
Fixes zed-industries#51537 regex::escape() escapes dashes which are only special inside [] character classes. This produces unnecessarily noisy patterns like ^https?://typescript\-eslint\.io when ^https?://typescript-eslint\.io is correct and more readable.
6590b81 to
1623df7
Compare
|
Rebased on main in 1623df7c. Upstream had refactored Verified: |
|
Thank you! Also may be worth sending a PR upstream (though they may want this behavior intentionally so you can escape a set of characters, it seems a bit odd) |
|
Thanks @ConradIrwin! Worth a look upstream - my read is Separately, the failing CI runs all blow up on |
…pattern Per @ConradIrwin's review on PR zed-industries#51603: the escape_for_pattern() helper introduced in 1623df7c strips \\- back to - everywhere, including inside single-quoted env var values. The test_extract_terminal_pattern fixture for "PAGER='less -R' git log" was still asserting the old escaped form, which broke macOS / Linux / Windows CI.
Head branch was pushed to by a user without write access
|
Thanks @ConradIrwin. That test expectation hadn't been updated when 1623df7c introduced Couldn't reproduce locally (this Mac is missing the Metal toolchain so the agent crate won't build), so this is verified by reading the diff. The single-quoted env-var path runs through the same |
|
Thanks! |
…ed-industries#51603) Fixes zed-industries#51537 `regex::escape()` escapes dashes, but dashes are only special inside `[]` character classes in regex. This means tool_permissions patterns end up with unnecessary backslashes: **Before:** `^https?://typescript\-eslint\.io`, `^git\-lfs\s+pull(\s|$)` **After:** `^https?://typescript-eslint\.io`, `^git-lfs\s+pull(\s|$)` The fix adds a small `escape_for_pattern()` helper that calls `regex::escape()` then strips the unnecessary dash escaping via `.replace("\\-", "-")`. This is applied to all five call sites in `pattern_extraction.rs`. Tests updated to expect unescaped dashes, plus a new `test_dashes_are_not_escaped` test covering terminal commands, URLs, and paths with dashes. This PR was developed with AI assistance. Release Notes: - Fixed unnecessary escaping of dashes in agent tool permission patterns (e.g. `typescript\-eslint` is now `typescript-eslint`) --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
…ed-industries#51603) Fixes zed-industries#51537 `regex::escape()` escapes dashes, but dashes are only special inside `[]` character classes in regex. This means tool_permissions patterns end up with unnecessary backslashes: **Before:** `^https?://typescript\-eslint\.io`, `^git\-lfs\s+pull(\s|$)` **After:** `^https?://typescript-eslint\.io`, `^git-lfs\s+pull(\s|$)` The fix adds a small `escape_for_pattern()` helper that calls `regex::escape()` then strips the unnecessary dash escaping via `.replace("\\-", "-")`. This is applied to all five call sites in `pattern_extraction.rs`. Tests updated to expect unescaped dashes, plus a new `test_dashes_are_not_escaped` test covering terminal commands, URLs, and paths with dashes. This PR was developed with AI assistance. Release Notes: - Fixed unnecessary escaping of dashes in agent tool permission patterns (e.g. `typescript\-eslint` is now `typescript-eslint`) --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
…ed-industries#51603) Fixes zed-industries#51537 `regex::escape()` escapes dashes, but dashes are only special inside `[]` character classes in regex. This means tool_permissions patterns end up with unnecessary backslashes: **Before:** `^https?://typescript\-eslint\.io`, `^git\-lfs\s+pull(\s|$)` **After:** `^https?://typescript-eslint\.io`, `^git-lfs\s+pull(\s|$)` The fix adds a small `escape_for_pattern()` helper that calls `regex::escape()` then strips the unnecessary dash escaping via `.replace("\\-", "-")`. This is applied to all five call sites in `pattern_extraction.rs`. Tests updated to expect unescaped dashes, plus a new `test_dashes_are_not_escaped` test covering terminal commands, URLs, and paths with dashes. This PR was developed with AI assistance. Release Notes: - Fixed unnecessary escaping of dashes in agent tool permission patterns (e.g. `typescript\-eslint` is now `typescript-eslint`) --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

Fixes #51537
regex::escape()escapes dashes, but dashes are only special inside[]character classes in regex. This means tool_permissions patterns end up with unnecessary backslashes:Before:
^https?://typescript\-eslint\.io,^git\-lfs\s+pull(\s|$)After:
^https?://typescript-eslint\.io,^git-lfs\s+pull(\s|$)The fix adds a small
escape_for_pattern()helper that callsregex::escape()then strips the unnecessary dash escaping via.replace("\\-", "-"). This is applied to all five call sites inpattern_extraction.rs.Tests updated to expect unescaped dashes, plus a new
test_dashes_are_not_escapedtest covering terminal commands, URLs, and paths with dashes.This PR was developed with AI assistance.
Release Notes:
typescript\-eslintis nowtypescript-eslint)