Skip to content

Remove a dead case pattern in common.sh (SC2221/SC2222) - #4605

Open
jzayas03 wants to merge 1 commit into
github:mainfrom
jzayas03:fix/common-sh-dead-case-pattern
Open

jzayas03 wants to merge 1 commit into
github:mainfrom
jzayas03:fix/common-sh-dead-case-pattern

Conversation

@jzayas03

Copy link
Copy Markdown

Description

resolve_template_content() in scripts/bash/common.sh rejects unsafe manifest paths with:

/*|*../*|../*) manifest_file="" ;;

The third alternative can never match: *../* already matches every string ../* does. shellcheck reports SC2221 ("this pattern always overrides a later one") and SC2222 ("this pattern never matches") on that line. This PR removes the dead alternative — a one-line change with no behavioural effect.

Verified behaviour-identical by classifying these inputs with the old and new pattern: /etc/passwd, ../x, a/../b, ../, x/../../y, .., a/.., ..a, a..b, ./x, templates/x.md. Every one produces the same reject/keep result before and after. After the change, shellcheck reports zero SC2221/SC2222 findings on the file and adds nothing new.

These are warnings, so lint.yml's --severity=error does not currently see them. This is one of the findings standing between the current threshold and the tightening described in CONTRIBUTING.md. Related: #1049.

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest7883 passed, 195 skipped, 1 failed. The failure is tests/integrations/test_events.py::TestCommandRunner::test_ps_variant_prefixed_with_powershell_launcher, and it fails identically with this change reverted: it asserts a pwsh/powershell -File prefix and pwsh is not installed on the machine I ran this on (macOS). It is pre-existing and environmental, not caused by this PR; your macos-latest runner has pwsh and should be green.
  • Tested with a sample project — not done, deliberately. The change is proven behaviour-identical (table above), so running commands through an agent could not observe any difference. Mapping per CONTRIBUTING.md for completeness:

Test selection reasoning

Changed file Affects Test Why
scripts/bash/common.sh every command invoking create-new-feature.sh, check-prerequisites.sh, setup-plan.sh, setup-tasks.sh, resolve-template.sh (all source common.sh) none run the diff is a no-op on behaviour; resolve_template_content() accepts and rejects exactly the same inputs

If maintainers would still like the manual agent runs, say so and I will do them.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (describe below)

This PR was produced by Claude (Anthropic's AI coding agent) operating under my direction: it found the finding while running shellcheck over a project that vendors these scripts, verified the fix, wrote the commit and this description. I reviewed and approved it before it was opened. Any follow-up comments from this account on this PR may also be AI-drafted, and I will keep that disclosed.

🤖 Generated with Claude Code

resolve_template_content() rejects unsafe manifest paths with
`/*|*../*|../*)`. The third alternative can never match: `*../*`
already matches every string `../*` does, so shellcheck reports SC2221
(this pattern always overrides a later one) and SC2222 (this pattern
never matches). Removing it changes nothing: checked against
/etc/passwd, ../x, a/../b, ../, x/../../y, .., a/.., ..a, a..b, ./x and
templates/x.md -- every input classifies identically before and after.

These are warnings, so lint.yml's --severity=error does not see them.
This is one of the findings between the current threshold and the
tightening CONTRIBUTING.md describes.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jzayas03
jzayas03 requested a review from mnriem as a code owner September 16, 2026 03:35
@mnriem mnriem added the triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate label Sep 16, 2026
@mnriem
mnriem requested a balanced review from Copilot September 17, 2026 11:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The removed pattern is fully subsumed by *../*, so behavior remains unchanged.

Pull request overview

Removes an unreachable Bash case alternative without changing path-validation behavior.

Changes:

  • Simplifies the unsafe manifest-path pattern and resolves SC2221/SC2222 warnings.
File summaries
File Description
scripts/bash/common.sh Removes the redundant ../* pattern.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants