Repository navigation
fix(install): version-pattern fragility flagged by Gemini on PR #19 - #20
Conversation
Gemini Code Assist correctly noted that the case statement introduced in PR #19 still uses literal-dot + single-digit [1-9] patterns, which fail on multi-digit components like 2.0.10.0, 2.10.0.0, 100.0.0. A future v2.0.10.0 install at the legacy path would fall through both arms and produce a false-positive "not a recognized install" warning. Replace the skip arm with multi-digit-safe patterns: 2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9]* Refinement vs Gemini's exact suggestion: 1[0-9]* (no literal dot) instead of 1[0-9].* — covers 100.0.0 too, no false positives since the legacy arm matches `1.*` not `1[0-9]`. Mirrored in bin/idstack-doctor and setup. Adds test/test-version-classifier.sh — pins the case statement against 20 representative versions (legacy + modern + multi-digit). Wired into smoke-test.sh. This is the second time Gemini caught a version-pattern bug in this code path; the test stops the third. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates version classification patterns in bin/idstack-doctor and setup to handle multi-digit components and introduces a new unit test suite. Feedback indicates that the current patterns miss major versions starting with '2' followed by other digits (e.g., 20.x.x), leading to false-positive warnings. The reviewer suggests updating the patterns to [12][0-9]* and adding a test case for version 20.0.0.0 to ensure these cases are covered.
| # Covered by test/test-version-classifier.sh. | ||
| case "$v" in | ||
| 2.0.1.*|2.[1-9].*|[3-9].*|1[0-9].*) ;; | ||
| 2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9]*) ;; |
There was a problem hiding this comment.
The current patterns miss major versions starting with 2 followed by another digit (e.g., 20.0.0, 21.0.0). While 1[0-9]* covers multi-digit versions starting with 1, and [3-9]* covers those starting with 3-9, the 20-29 range (and 200-299 etc.) will fall through to the *) case, triggering a false-positive "not a recognized install" warning. Using [12][0-9]* ensures all multi-digit major versions starting with 1 or 2 are correctly skipped.
2.0.[1-9]*|2.[1-9]*|[3-9]*|[12][0-9]*) ;;
| # Covered by test/test-version-classifier.sh. | ||
| case "$legacy_version" in | ||
| 2.0.1.*|2.[1-9].*|[3-9].*|1[0-9].*) ;; | ||
| 2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9]*) ;; |
There was a problem hiding this comment.
The current patterns miss major versions starting with 2 followed by another digit (e.g., 20.0.0, 21.0.0). While 1[0-9]* covers multi-digit versions starting with 1, and [3-9]* covers those starting with 3-9, the 20-29 range (and 200-299 etc.) will fall through to the *) case, triggering a false-positive "not a recognized install" warning. Using [12][0-9]* ensures all multi-digit major versions starting with 1 or 2 are correctly skipped.
2.0.[1-9]*|2.[1-9]*|[3-9]*|[12][0-9]*) ;;
| # patterns in lockstep with both files — if you change one, change all three. | ||
| classify_version() { | ||
| case "$1" in | ||
| 2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9]*) echo "skip" ;; |
There was a problem hiding this comment.
The current patterns miss major versions starting with 2 followed by another digit (e.g., 20.0.0, 21.0.0). While 1[0-9]* covers multi-digit versions starting with 1, and [3-9]* covers those starting with 3-9, the 20-29 range (and 200-299 etc.) will fall through to the *) case, triggering a false-positive "not a recognized install" warning. Using [12][0-9]* ensures all multi-digit major versions starting with 1 or 2 are correctly skipped.
| 2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9]*) echo "skip" ;; | |
| 2.0.[1-9]*|2.[1-9]*|[3-9]*|[12][0-9]*) echo "skip" ;; |
| check "19.0.0.0" skip | ||
| check "100.0.0.0" skip |
There was a problem hiding this comment.
Add a test case for major version 20 to ensure the pattern gap is covered and verified.
| check "19.0.0.0" skip | |
| check "100.0.0.0" skip | |
| check "19.0.0.0" skip | |
| check "20.0.0.0" skip | |
| check "100.0.0.0" skip |
Post-ship patch covering PRs #19 + #20 — Gemini-flagged fragility in the install-hygiene code that v2.4.0.0 introduced. Mirrors the v2.2.0.0 → v2.2.0.1 precedent for follow-up Gemini review fixes. - Multi-digit version classification now safe for 2.0.10.0, 2.10.0.0, 100.0.0 - New test/test-version-classifier.sh pins the case statement (20 inputs) - setup --keep-legacy and idstack-doctor --local match in any arg position - idstack-doctor: explicit -d check, surfaces silent version-parse failures, bash-native [[ == ]] symlink match - Voice consistency: README/landing page use 3-digit minor for user copy, 4-digit canonical form stays in CHANGELOG/VERSION/plugin.json - CLAUDE.md commands list now mentions bin/idstack-doctor Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…i on PR #21 (#22) Third Gemini-flagged iteration on the same legacy-version classifier (PR #15 → PR #19 → PR #20 → PR #21). The v2.4.0.1 pattern 1[0-9]* correctly handled multi-digit majors starting with 1 (10–19, 100–199, …) but silently missed any other multi-digit major: 20.x, 25.x, 200.x, etc. fell through both arms of the case and ended up classified as `unknown`. Replaced 1[0-9]* with [1-9][0-9]* in setup, bin/idstack-doctor, and test/test-version-classifier.sh. Strictly more general, no false positives against the legacy arm — [1-9][0-9]* requires a second digit and the legacy arm requires a literal `.` or matches bare `2.0.0`. Test fixture extended with 7 cases that previously fell through to "unknown": 20.0.0.0, 21.5.0, 25.99.0, 29.0.0, 30.0.0, 200.0.0, 999.0.0. Total fixture count: 27. Comment header records the third iteration so future maintainers can see the history. VERSION + plugin.json bumped to 2.4.0.2; CHANGELOG entry added. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
[1-9]patterns. Multi-digit components like2.0.10.0,2.10.0.0, or100.0.0fall through both arms and produce a false-positive "not a recognized install" warning.2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9]*. Mirrored inbin/idstack-doctorandsetup.test/test-version-classifier.sh(20 cases: legacy + modern + multi-digit) and wires it intosmoke-test.sh. This is the second time Gemini has caught a version-pattern bug here; the test stops the third.Refinement vs Gemini's exact suggestion
1[0-9]*(no literal dot) instead of1[0-9].*— covers100.0.0and beyond, no false positives since the legacy arm requires1.not1[0-9].Test plan
test/test-version-classifier.sh— 20/20 passtest/smoke-test.sh— version-classifier check picked up; only pre-existing failure (user-local~/.claude/skills/idstack) remains, unrelated--localinstall withVERSION=2.0.10.0(modern, non-dispatcher) — no false-positive PROBLEM, doctor exits 0VERSION=2.0.0.0— still flagged as pre-v2.0.1.0 install (regression check)🤖 Generated with Claude Code