fix: strip bare integers from shell approval patterns (#1331)#1332
Merged
Conversation
Bare integer tokens (ticket IDs, port numbers, timeouts) were being baked into approval verb patterns via the display/retry-key path, causing every unique value to require a separate approval prompt. The AST parser already correctly excludes integers from VerbChain (via IsVerbLikeToken requiring [a-z] start). The gap was in ReconstructClauseText which iterates all clause.Args — including bare integers — for display and retry-exact key generation. Fix: IsBareIntegerToken() predicate + break in ReconstructClauseText when an integer is encountered (termination, not skip — everything after the integer is outside the approval intent). Changes: - IsBareIntegerToken() — detects pure-digit strings, excludes flags - ReconstructClauseText() — break on bare integer args - 4 new POSIX-only tests (strip, generalize, timeout, candidates) - OpenSpec change set + delta spec synced to main spec POSIX-only: Windows uses the legacy ShellTokenizer path which retains the old behavior. PwshParser integration is a separate change. Test results: 566/566 pass, zero regressions.
…ation The integer-positional-argument tests use [Fact(SkipUnless = ..., Skip = "...")] which triggers slopwatch SW001 (disabled test violation). Replace with plain [Fact] + early-return on Windows. Tests run on all platforms, pass on Windows, assert on POSIX. No disabled tests, no SW001. This is the correct pattern: tests should be active everywhere they compile, just conditional in behavior — not disabled via SkipUnless.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Bare integer tokens (ticket IDs, port numbers, timeouts) were being baked into approval verb patterns via the display/retry-key path, causing every unique value to require a separate approval prompt.
Example:
freshdesk ticket get 123→ pattern:freshdesk ticket get 123freshdesk ticket get 456→ pattern:freshdesk ticket get 456Two unrelated entries. Two approval prompts. No generalization.
Root Cause
The AST parser (ShellSyntaxTree) already correctly excludes integers from
VerbChainviaIsVerbLikeToken(requires[a-z]start). The matching path works fine. The gap is inReconstructClauseText()which iterates allclause.Args— including bare integers — for display and retry-exact key generation.Fix
IsBareIntegerToken()— detects pure-digit strings, excludes flags (which start with-)ReconstructClauseText()—breakon bare integer args (termination, not skip — everything after the integer is outside the approval intent)Specs & Tests
Scope
POSIX-only. Windows uses the legacy
ShellTokenizerpath which retains the old behavior. PwshParser integration is a separate change.Fixes #1331