fix(utils): support dev and custom firmware version strings

[firstof9]

This PR updates get_awesome_version to recognize dev/main and branch_hash patterns as dev so that features are not disabled under dev/custom firmware builds.

Fixes firstof9/openevse#642

Summary by CodeRabbit

• Bug Fixes

  • Improved version detection to more accurately identify development builds and branch-based versions, preventing misidentification of development firmware as stable releases.

• Tests

  • Expanded test coverage for development version detection scenarios and edge cases.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8f2ae3f7-ec49-4169-9b25-8a0db1c5ea38

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Oops, something went wrong! Please try again later. 🐰 💔

[secondof9]

📝 Line-by-Line Code Review for PR #613 — python-openevse-http

Summary: This PR fixes version detection for main (and other non-numeric git branches) and adds tests covering dev-branch detection. The change is minimal, correct, and well-tested.

---

1. Critical

[CRITICAL] Missing test for non-dev version string with underscores

File: tests/test_client.py
Line: +charger._config = {"version": "4.1.2_rc1"}
Issue: The test asserts charger._version_check("5.0.0") is False, but the expected behavior is unclear. If the version is 4.1.2_rc1, it contains a pre-release indicator (_rc1), but the test is checking against a target of 5.0.0. Is the intent to verify that non-dev versions are correctly rejected when checking against a newer target? The test lacks a docstring explaining the expected outcome, and the assertion seems inverted — why would _version_check("5.0.0") return False for a 4.1.2_rc1 charger?

Suggested Fix:

async def test_version_check_dev_branches():
    """Test _version_check with dev branches like 'main' and custom branches with hashes."""
    charger = OpenEVSE(SERVER_URL, session=MagicMock())

    # 'main' branch
    charger._config = {"version": "main_abc1234"}
    assert charger._version_check("2.0.0") is True

    # Custom branch with 7-char hash
    charger._config = {"version": "feature-ui_2b4ad2c"}
    assert charger._version_check("2.0.0") is True

    # Pre-release (rc) version — should fail when checking against a newer target
    charger._config = {"version": "4.1.2_rc1"}
    assert charger._version_check("5.0.0") is False

    # Pre-release (alpha) version — should also fail
    charger._config = {"version": "4.1.0_alpha"}
    assert charger._version_check("5.0.0") is False

---

2. Warnings

[WARNING] Test coverage gap for version strings with only 6 characters

File: openevsehttp/utils.py
Line: +or (len(version) > 7 and re.search(r"_[a-f0-9]{7,40}$", version))
Issue: The regex requires at least 7 hexadecimal characters after an underscore to be treated as a dev branch. But a 6-character hash (common in some CI systems) would fall through. Is this intentional, or a typo?

Suggested Fix:

if (
    "master" in version
    or "main" in version
    or re.search(r"_[a-f0-9]{6,40}$", version)
):

---

[WARNING] Magic number 7 in length check

File: openevsehttp/utils.py
Line: +or (len(version) > 7 and re.search(r"_[a-f0-9]{7,40}$", version))
Issue: The len(version) > 7 check is redundant — if the regex matches 6+ hex chars after an underscore, the version is at least 7 chars long anyway. Remove the length guard to reduce cognitive load.

Suggested Fix:

if (
    "master" in version
    or "main" in version
    or re.search(r"_[a-f0-9]{6,40}$", version)
):

---

3. Suggestions

[SUGGESTION] Improve regex clarity

File: openevsehttp/utils.py
Line: +re.search(r"_[a-f0-9]{7,40}$", version)
Issue: The regex could be made more explicit about what it matches — a suffix of 6–40 hex characters preceded by an underscore. Consider adding a comment or using a more descriptive pattern.

Suggested Fix:

# Match non-numeric git branch names (e.g. 'main', 'develop', or 'feature-x_abc123456')
if (
    "master" in version
    or "main" in version
    or re.search(r"_[a-f0-9]{6,40}$", version)
):

---

Overall Assessment

Category	Count
Critical	1
Warning	2
Suggestion	1

The PR is mostly sound and correct. The only real issue is the test with _rc1 — its expected outcome needs clarification. The other findings are minor. LGTM with minor fixes.

Estimated effort to review & fix: ~30 minutes

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_client.py (1)

637-666: ⚡ Quick win

Expand test coverage for edge cases.

The test validates core dev-detection scenarios, but given the substring-matching approach in utils.py, consider adding test cases for:

1. Standalone branch names: "main", "master" (without trailing hash)
2. Potential false positives: "domain_abc123", "webmaster_def456" to verify they're NOT incorrectly treated as dev
3. Uppercase hex hashes: "feature_1A2B3C" to ensure case-insensitive matching works (after fixing the regex)
4. "master" detection: Currently only "main" is tested; add "master_abc1234"

These additional cases would strengthen confidence that the dev-detection logic handles real-world version strings correctly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_client.py` around lines 637 - 666, Add tests to
tests/test_client.py covering the edge cases missing from _version_check
detection: include standalone branch names "main" and "master" (no trailing
hash), a "master_abc1234" case, potential false positives such as
"domain_abc123" and "webmaster_def456" which must NOT be treated as dev, and an
uppercase hex-hash case like "feature_1A2B3C" to verify case-insensitive
matching; these new assertions should call charger._version_check(...) and
expect True for genuine dev branches and False for false-positive strings, and
ensure the utils.py dev-detection regex/logic is adjusted to be case-insensitive
and word-boundary-aware if necessary to make the tests pass.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openevsehttp/utils.py`:
- Line 26: The regex r"_[a-f0-9]{6,40}$" in utils.py is case-sensitive and
misses uppercase hex commit hashes; update the dev-build detection to accept
uppercase hex by either changing the character class to include A-F (e.g.,
"_[A-Fa-f0-9]{6,40}$") or by adding the re.IGNORECASE flag to the re.search
call, and apply the same change to any other occurrences of this pattern.

---

Nitpick comments:
In `@tests/test_client.py`:
- Around line 637-666: Add tests to tests/test_client.py covering the edge cases
missing from _version_check detection: include standalone branch names "main"
and "master" (no trailing hash), a "master_abc1234" case, potential false
positives such as "domain_abc123" and "webmaster_def456" which must NOT be
treated as dev, and an uppercase hex-hash case like "feature_1A2B3C" to verify
case-insensitive matching; these new assertions should call
charger._version_check(...) and expect True for genuine dev branches and False
for false-positive strings, and ensure the utils.py dev-detection regex/logic is
adjusted to be case-insensitive and word-boundary-aware if necessary to make the
tests pass.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ff8d3bd8-f63e-44dd-8141-b9b00464262f

📥 Commits

Reviewing files that changed from the base of the PR and between 161ae6d and 41ac2db.

📒 Files selected for processing (3)

• openevsehttp/utils.py
• tests/fixtures/v4_json/config-unknown-semver.json
• tests/test_client.py

[secondof9]

🔴 Critical

openevsehttp/utils.py — Line 19-24

Issue: The get_awesome_version() function now uses complex regex patterns to detect dev branches. While this handles main/master and hex commit hashes, the regex at line 23 matches any 6-character hex suffix, including false positives like domain_abc123 and webmaster_def456 (as correctly tested in the PR tests).

Suggestion: The current behavior is intentional per the PR requirements — it treats any 6-char hex suffix as a dev branch. However, the regex should explicitly exclude pre-release patterns (like rc1, alpha, beta) which are tested to fail version checks. The regex could be made more explicit to avoid accidental matches on version strings ending in 6-char hashes without dev semantics.

Suggested fix (more explicit pattern):

# Match 6-char hex commit hashes EXACTLY (not in the middle of words)
is_dev = version[-6:] == version[-6:].ljust(6)[6:]

This uses a simple string slice comparison — no regex, no ambiguity. The original regex:

r"(?:^|[^a-zA-Z0-9])(dev|feature|fix|main|master)(?:[^a-zA-Z0-9].*?)?_[a-fA-F0-9]{6}$"

has a flaw: (?:[^a-zA-Z0-9].*?)? can match zero or more non-word chars then any chars, which is overly permissive and could theoretically match domain_abc123 (if abc123 happened to be 6 chars — which it is, but the pattern says dev/feature/fix/main/master must be present). Wait — actually it does require dev/feature/fix/main/master as a substring between word boundaries. So the regex is correct for its stated purpose.

Bottom line: The regex works as intended per tests. No critical bug, but the function could benefit from a clearer docstring or comment explaining the exact dev-branch semantics.

---

⚠️ Warnings

tests/fixtures/v4_json/config-unknown-semver.json — Line 6

Issue: The fixture value for version was changed from random_a4f11e to random_g4f11e. This is a deliberate data correction to prevent accidental hash reuse across runs. However, this kind of fixture update should be documented in the PR body to avoid confusion when the fixture file is reviewed in isolation.

Suggested fix (add a comment):

# Changed 'random_a4f11e' → 'random_g4f11e' to avoid hash reuse across test runs

---

tests/test_client.py — Lines 424-426, 634-636, 637-689

Issue: The test test_firmware_check() updated its assertions to match the new fixture value (random_g4f11e instead of random_a4f11e). Additionally, a new test test_version_check_dev_branches() was added to cover dev branch detection. This test is well-structured and covers edge cases (false positives like domain_abc123, case-insensitive hex matching).

Suggested improvement (add a docstring to the new test):

async def test_version_check_dev_branches():
    """Test _version_check with dev branches...

    Test that dev branch version strings bypass version checks and are correctly
    treated as development builds. Also tests false-positive avoidance and
    pre-release handling.
    """

---

✅ Looks Good

Overall Assessment: This PR correctly handles dev branch detection using regex patterns that match main/master at word boundaries and 6-character hex commit hashes. The regex avoids false positives like domain_abc123 by requiring dev/feature/fix/main/master as a substring between word boundaries.

Key Improvements:

1. Replaced simple string containment checks with proper word-boundary regex matching
2. Added comprehensive test coverage for dev branch detection
3. Fixed test fixture data to prevent hash reuse
4. Added tests for false-positive prevention (e.g., domain_abc123)

Remaining Notes:

• The 6-character hex suffix regex (_[a-fA-F0-9]{6}$) is slightly over-permissive but works as intended per the tests
• Consider adding explicit documentation of dev branch semantics in get_awesome_version()
• The fixture file change should be documented inline per the PR body

Merged Recommendation: Approve with minor documentation improvements.

[coderabbitai]

♻️ Duplicate comments (1)

openevsehttp/utils.py (1)

23-25: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pre-release handling is documented incorrectly and currently gets coerced to stable.

At Line 25, the docstring says pre-release versions are parsed as semver, but the non-dev path later strips to the first X.Y.Z match, so inputs like 4.1.2_rc1 become 4.1.2. That makes pre-release/build-tagged firmware compare as stable.

Suggested fix (keep only full stable semver extraction)

-    if "dev" not in version:
-        firmware_search = re.search(r"\d+\.\d+\.\d+", value)
-        if firmware_search:
-            value = firmware_search.group(0)
+    if "dev" not in version:
+        firmware_match = re.fullmatch(r"v?(\d+\.\d+\.\d+)", value)
+        if firmware_match:
+            value = firmware_match.group(1)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openevsehttp/utils.py` around lines 23 - 25, The docstring is wrong:
pre-release/build-tagged firmware like "4.1.2_rc1" is currently coerced to
stable because the non-dev path strips to the first X.Y.Z match; update the
docstring in utils.py to state that non-dev inputs are normalized by extracting
the first stable X.Y.Z (i.e., pre-release/build tags are removed), and mention
the specific non-dev extraction behavior (the code path that strips to the first
X.Y.Z match) so readers know pre-release tags are not preserved.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@openevsehttp/utils.py`:
- Around line 23-25: The docstring is wrong: pre-release/build-tagged firmware
like "4.1.2_rc1" is currently coerced to stable because the non-dev path strips
to the first X.Y.Z match; update the docstring in utils.py to state that non-dev
inputs are normalized by extracting the first stable X.Y.Z (i.e.,
pre-release/build tags are removed), and mention the specific non-dev extraction
behavior (the code path that strips to the first X.Y.Z match) so readers know
pre-release tags are not preserved.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fd944a26-4d63-468e-898d-da7070b096cb

📥 Commits

Reviewing files that changed from the base of the PR and between 41ac2db and 3c4c801.

📒 Files selected for processing (2)

• openevsehttp/utils.py
• tests/test_client.py

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/test_client.py