Repository navigation
fix: address Gemini review on PRs #15, #16, #18 - #19
Merged
Merged
Conversation
- setup: --keep-legacy now matches in any argument position (was: only $1/$2) - setup + bin/idstack-doctor: legacy-version case rewritten with explicit modern/future skip arm to be self-documenting and future-proof - bin/idstack-doctor: --local matches in any argument position (was: only $1) - bin/idstack-doctor: explicit -d check for plugin dir (catches the rare "exists but isn't a directory or symlink" case) - bin/idstack-doctor: silently-failing version parse now flagged as a problem instead of swallowed - bin/idstack-doctor: bash-native [[ == ]] for symlink target match (avoids subshell + grep, dodges the leading-dash echo footgun) - README: Status callout uses 3-digit v2.4.0 to match the landing-page timeline convention; canonical 4-digit form stays in CHANGELOG / VERSION / plugin.json - docs/index.html: hero-beta says "between minor versions" to match README 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 versioning references in the documentation and improves the robustness of the idstack-doctor and setup scripts by enhancing argument parsing and directory validation. Feedback was provided regarding the version matching logic in the case statements; the current patterns are too restrictive and will fail to correctly identify modern versions with multi-digit components, such as '2.10.0', which could trigger incorrect legacy warnings.
| # legacy ones. Mirrors setup; future-proof if v3.x or v10.x ever ships. | ||
| case "$v" in | ||
| 2.0.0.0|2.0.0|1.*|0.*) signature="${signature:+$signature, }VERSION=$v" ;; | ||
| 2.0.1.*|2.[1-9].*|[3-9].*|1[0-9].*) ;; |
There was a problem hiding this comment.
The version patterns in this case statement are too restrictive because they rely on literal dots and single-digit character classes. For example, 2.0.1.* will not match 2.0.10.0, and 2.[1-9].* will not match 2.10.0. This causes modern versions with multi-digit components to fall through the first arm and trigger an incorrect warning that the directory is not a recognized install. Using more flexible patterns that account for multi-digit components would be more robust.
2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9].*) ;;
| # ships through this path. | ||
| case "$legacy_version" in | ||
| 2.0.0.0|2.0.0|1.*|0.*) legacy_is_old_version=1 ;; | ||
| 2.0.1.*|2.[1-9].*|[3-9].*|1[0-9].*) ;; |
There was a problem hiding this comment.
This version check suffers from the same pattern fragility as the one in idstack-doctor. Modern versions with multi-digit components (e.g., 2.0.10.0 or 2.10.0.0) will fail to match the first arm, leading to a false-positive warning about an unrecognized install directory.
2.0.[1-9]*|2.[1-9]*|[3-9]*|1[0-9].*) ;;
4 tasks done
savvides
added a commit
that referenced
this pull request
May 5, 2026
) 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>
3 of 6 tasks
savvides
added a commit
that referenced
this pull request
May 5, 2026
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>
3 of 4 tasks
savvides
added a commit
that referenced
this pull request
May 5, 2026
…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>
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.
Summary
Fix-up PR addressing Gemini Code Assist review comments left on already-merged PRs #15 (v2.4 release), #16 (install-hygiene), and #18 (landing beta + timeline).
Shell robustness (real wins)
setup—--keep-legacynow matches in any argument position. Was:[ "$1" = "--keep-legacy" ] || [ "$2" = "--keep-legacy" ](broke if--keep-legacywas the third+ arg). Now:[[ " $* " == *" --keep-legacy "* ]].bin/idstack-doctor— same fix for--local. Now:[[ " ${*:-} " == *" --local "* ]].bin/idstack-doctor— symlink-target match uses bash-native[[ "$t" == *"idstack"* ]]instead ofecho "$t" | grep -q "idstack". Avoids spawning a subshell + grep, and dodges the case where$tstarts with a dash (echo would interpret as a flag).Doctor diagnostic gaps
bin/idstack-doctor— explicitly checks-dforPLUGIN_DIR. Was: assumed any non-symlink existing path was a directory. Now flags "exists but is not a directory or symlink" as a PROBLEM.bin/idstack-doctor— version-parse failure onplugin.jsonnownote_problem's and prints the manifest path, instead of silently producing no output.Future-proof version pattern
setup+bin/idstack-doctor— legacy-versioncaserewritten with two arms: an explicit modern/future skip arm (2.0.1.*|2.[1-9].*|[3-9].*|1[0-9].*), then the legacy flag arm (0.*|1.*|2.0.0.*|2.0.0). Self-documenting; survives a hypothetical future v3.x or v10.x.1.*would match10.0.0.0— this is incorrect (verified: bashcase 10.0.0.0 in 1.*)does not match because the literal.after1doesn't appear). The original was correct; the new form is just more legible.Doc copy alignment
Status: betacallout uses 3-digitv2.4.0to match landing-page timeline convention. Canonical 4-digit form stays in CHANGELOG / VERSION /.claude-plugin/plugin.json.Test plan
bash -n setup && bash -n bin/idstack-doctor— syntax ok./test/smoke-test.sh— 166/167 (1 expected legacy-install fail on host)bin/idstack-doctor— flags legacy install correctly, exits 1--keep-legacyin any position:setup --foo --bar --keep-legacykeeps the legacy dir--localin any position:bin/idstack-doctor --foo --localswitches scope--keep-legacyflag: setup removes legacy as expected1.*verified — does NOT match10.0.0.0(Gemini misread)🤖 Generated with Claude Code