Skip to content

feat: human-readable permission labels, deny rule feedback, and multi-dir search improvements#2637

Merged
Mingholy merged 2 commits intoQwenLM:mainfrom
LaZzyMan:fix-permission-issues
Mar 26, 2026
Merged

feat: human-readable permission labels, deny rule feedback, and multi-dir search improvements#2637
Mingholy merged 2 commits intoQwenLM:mainfrom
LaZzyMan:fix-permission-issues

Conversation

@LaZzyMan
Copy link
Copy Markdown
Collaborator

TLDR

Improve the permission system UX by replacing raw rule syntax (e.g. Read(//Users/alice/**)) with human-readable labels (e.g. "read files in /Users/alice/") in the "Always Allow" confirmation dialog, and surface matching deny rules in error messages when tools are blocked. Also adds comprehensive tests for multi-directory search in glob/grep/ripGrep tools (introduced in the previous commit).

Screenshots / Video Demo

Before:

Always allow in this project [Read(//Users/mochi/.qwen/**)]

After:

Always allow read files in /Users/mochi/.qwen/ in this project

Deny rule feedback (new):

Tool "run_shell_command" is denied by permission rules. Matching deny rule: "Bash(rm *)".

Dive Deeper

Human-readable permission labels (rule-parser.ts)

  • Added buildHumanReadableRuleLabel() that converts rule arrays from buildPermissionRules() into natural-language descriptions
  • Added cleanPathSpecifier() helper to strip glob suffixes (/**, /*) and the // absolute-path prefix for cleaner display
  • Maps each rule category to a verb phrase via DISPLAY_NAME_TO_VERB (e.g. Read → "read files", Bash → "run commands", WebFetch → "fetch from")

Deny rule feedback (permission-manager.ts, coreToolScheduler.ts)

  • Added PermissionManager.findMatchingDenyRule() to locate the first deny rule matching a given context
  • Updated coreToolScheduler to include the matching deny rule in error messages, helping users understand why a tool was blocked

UI & i18n (ToolConfirmationMessage.tsx, locale files)

  • Replaced raw [Rule(specifier)] bracket labels with buildHumanReadableRuleLabel() output
  • Added new i18n keys 'Always allow {{action}} in this project' and 'Always allow {{action}} for this user' across all 6 locales

Multi-directory search tests (glob, grep, ripGrep)

  • Tests for searching across multiple workspace directories when no path is specified
  • Tests for deduplication of overlapping directory results (glob)
  • Tests for absolute path conversion when merging results from multiple dirs (grep)
  • Tests for loading .qwenignore from each workspace directory (ripGrep)

Reviewer Test Plan

  1. Permission labels: Run qwen in a project, trigger a tool that requires confirmation (e.g. file edit outside workspace). Verify the "Always allow" options show human-readable labels instead of raw rule syntax.
  2. Deny rule feedback: Add a deny rule (e.g. Bash(rm *) in settings), then ask the agent to run rm — verify the error message includes "Matching deny rule: ...".
  3. Unit tests: Run npm run test -- packages/core/src/permissions/permission-manager.test.ts packages/core/src/tools/glob.test.ts packages/core/src/tools/grep.test.ts packages/core/src/tools/ripGrep.test.ts — all 291 tests should pass.

Testing Matrix

🍏 🪟 🐧
npm run
npx
Docker
Podman - -
Seatbelt - -

Linked issues / bugs

N/A — general permission UX improvement and test coverage for multi-directory search.

…-dir search tests

- Add buildHumanReadableRuleLabel() to convert raw permission rules into
  natural-language descriptions for the 'Always Allow' UI options
- Add PermissionManager.findMatchingDenyRule() to surface which deny rule
  caused a tool to be blocked, improving error messages in coreToolScheduler
- Update ToolConfirmationMessage to use friendly labels with i18n support
- Add comprehensive tests for new permission features and multi-directory
  search in glob, grep, and ripGrep tools
- Fix integration test for tool-control allowedTools configuration
@github-actions
Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR introduces significant UX improvements to the permission system by replacing raw rule syntax with human-readable labels in confirmation dialogs, and adds matching deny rule information to error messages. It also enhances multi-directory search functionality for glob/grep/ripGrep tools with comprehensive test coverage. The implementation is well-structured, thoroughly tested (all 291 tests pass), and maintains backward compatibility.

🔍 General Feedback

  • Strong test coverage: The PR includes 17 new test cases for buildHumanReadableRuleLabel() and 5 for findMatchingDenyRule(), plus 7 multi-directory workspace tests across glob/grep/ripGrep tools.
  • Good separation of concerns: New functionality is cleanly isolated in helper functions (buildHumanReadableRuleLabel, cleanPathSpecifier) without modifying existing core logic.
  • Consistent patterns: The multi-directory search implementation follows the same pattern across all three tools (glob, grep, ripGrep), making the codebase easier to maintain.
  • Internationalization awareness: New i18n keys are properly added across all 6 locale files (en, de, ja, pt, ru, zh).
  • Type safety: TypeScript types are properly maintained throughout, with no type errors.

🎯 Specific Feedback

🟢 Medium

  • File: packages/core/src/permissions/rule-parser.ts:458 - The DISPLAY_NAME_TO_VERB mapping is hardcoded and could become stale if new tool types are added. Consider deriving this mapping from a single source of truth (e.g., the tool registry or a shared enum) to prevent drift.

  • File: packages/core/src/tools/glob.ts:137-167 - The globInDirectory helper method duplicates some logic from the original execute method (e.g., normalizePathForComparison). Consider extracting this normalization logic into a shared utility function to reduce duplication.

  • File: packages/core/src/core/coreToolScheduler.ts:704-710 - The error message construction for deny rules could benefit from a helper function to avoid repetition, since similar logic appears at lines 923-932:

    const matchingRule = pm.findMatchingDenyRule({...});
    const ruleInfo = matchingRule
      ? ` Matching deny rule: "${matchingRule}".`
      : '';

🔵 Low

  • File: packages/core/src/permissions/rule-parser.ts:437 - The cleanPathSpecifier function comment shows /src/**src/ but the actual implementation would produce /src/ (with leading slash). Consider updating the comment to match the actual behavior, or clarify when the leading slash is preserved vs. removed.

  • File: packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx:253-271 - The ternary logic for building labels is repeated three times (for exec, infoProps, and mcp props). Consider extracting this into a helper function like buildAlwaysAllowOptions() to reduce duplication and improve maintainability.

  • File: packages/core/src/tools/grep.ts:122 - The comment "When searching multiple directories, convert relative file paths to absolute paths" could be expanded to explain why this is necessary (e.g., "to avoid ambiguity when the same relative path exists in multiple workspace directories").

  • File: packages/core/src/tools/ripGrep.ts:213-226 - The .qwenignore loading logic uses a seenIgnoreFiles Set for deduplication, but this pattern isn't used in the glob tool's multi-directory implementation. Consider whether glob.ts should also deduplicate ignore files for consistency.

✅ Highlights

  • Excellent UX improvement: The human-readable labels (e.g., "read files in /Users/mochi/.qwen/" instead of "Read(//Users/mochi/.qwen/**)") significantly improve user comprehension of permission scopes.

  • Valuable deny rule feedback: Including the matching deny rule in error messages ("Matching deny rule: 'Bash(rm *)'") helps users understand why a tool was blocked, reducing confusion and support burden.

  • Robust multi-directory support: The implementation correctly handles edge cases like overlapping directories (deduplication), explicit path overrides, and per-directory .qwenignore loading.

  • Comprehensive test coverage: The test suite covers edge cases including empty rules, unknown display names, path specifier variations, and round-trip testing from buildPermissionRules() to buildHumanReadableRuleLabel().

  • Clean i18n integration: The use of interpolation ({{action}}) in translation keys is the correct pattern for dynamic content, avoiding concatenation issues in different languages.

@Mingholy Mingholy merged commit d6502dd into QwenLM:main Mar 26, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants