[hardikkaurani]

Summary

When resolving configuration properties from .virlrc:

1. ClientConfig._get_from_file used line.startswith(prop_name), which caused false-positive prefix matches (e.g. querying VIRL2_USER incorrectly matched VIRL2_USERNAME="admin"). Additionally, leading whitespace before keys and comments was not stripped, and single-quoted values ('val') were not unquoted.
2. virlrc.open() used system-default encoding instead of explicit utf-8, causing potential UnicodeDecodeError on non-UTF-8 default platforms.
3. In container or stripped environments where Path.home() cannot resolve the user directory, _get_prop raised an unhandled RuntimeError.
4. In tests/test_configuration.py, the home_virlrc fixture relied on modifying os.environ["HOME"], which failed on Windows where HOME is not set by default (TypeError: str expected, not NoneType in teardown) and Path.home() does not use HOME.

Changes

• Exact Key Matching & Quote Unquoting:
  • ClientConfig._get_from_file: ignores blank lines and comments (#), strips whitespace around keys and =, enforces exact key equality (key.strip() == prop_name), and unquotes both single and double quotes.
  • Opens .virlrc explicitly with encoding="utf-8" and handles file reading errors gracefully.
  • Catches RuntimeError on Path.home() in ClientConfig._get_prop to safely return None when home cannot be determined.
• Cross-Platform Test Fixtures & Regressions:
  • Updated cwd_virlrc and home_virlrc fixtures in tests/test_configuration.py to use monkeypatch (monkeypatch.chdir and monkeypatch.setattr(Path, "home", ...)), enabling 100% reliable execution across Linux, macOS, and Windows.
  • Added unit tests:
    • test_get_from_file_exact_key_and_quotes: verifies exact key matching and quote unquoting.
    • test_get_prop_home_runtime_error: verifies graceful fallback when home directory resolution raises RuntimeError.

Validation

• Full test suite: 848 passed in 65.68s (all previously failing Windows tests now pass).
• Flake8 checks: clean (0 issues on changed files).
• All commits are signed off per DCO requirements.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[hardikkaurani]

Hi @tmikuska,

I've opened this PR to address key matching accuracy in .virlrc parsing and improve cross-platform test reliability:

1. ClientConfig._get_from_file matched keys using line.startswith(prop_name), causing false-positive prefix matches (e.g. VIRL2_USER matching VIRL2_USERNAME="admin"). It now verifies exact key equality after stripping whitespace and cleanly unquotes both single and double quotes.
2. File reading now explicitly specifies utf-8 encoding, and Path.home() handles RuntimeError gracefully for restricted container environments.
3. Updated the test fixtures in tests/test_configuration.py to use monkeypatch instead of manual HOME environment manipulation, resolving the Windows teardown issue noted in earlier runs and ensuring all 848 tests pass cleanly across all platforms.

All 4 commits include DCO sign-offs, and flake8 checks are completely clean. Whenever you have a chance, please let me know if you'd like any adjustments. Thank you!

[tmikuska]

[Minor (New) - Confidence: 90%]

Catching OSError / UnicodeDecodeError and returning None makes a present-but-unreadable cwd .virlrc indistinguishable from a missing file. _get_prop then falls through to ~/.virlrc, so a non-UTF-8 or unreadable local file silently uses home credentials.

Reproduced: a binary cwd .virlrc caused _get_prop("VIRL2_USER") to return the home-file user.

If the file exists (is_file() already passed), prefer failing loud or logging a warning rather than treating a read error as “key not found”.

[hardikkaurani]

Resolved! When an existing .virlrc file cannot be read due to an OSError or UnicodeDecodeError, the error is now logged as a warning with the file path and re-raised instead of returning None. This prevents silently falling back to home credentials when a local .virlrc file exists but is unreadable. Added the regression test test_get_from_file_decode_and_os_error_fails_loud to cover this behavior.

[tmikuska]

[Minor (New) - Confidence: 90%]

Windows Notepad “UTF-8” files typically start with a BOM. encoding="utf-8" decodes that as U+FEFF on the first key (\ufeffVIRL2_USER), so the lookup returns None. That undercuts the Windows reliability goal of this change.

utf-8-sig strips a leading BOM and is a no-op for BOM-less files.

Suggested change

	with virlrc.open(encoding="utf-8") as fh:
	with virlrc.open(encoding="utf-8-sig") as fh:

[hardikkaurani]

Adopted! Updated open() to use encoding="utf-8-sig" across virl2_client.py and test fixtures, cleanly stripping leading BOM characters. Added unit test test_get_from_file_utf8_bom to verify UTF-8 BOM files are parsed accurately.

[tmikuska]

[Minor (Pre-existing) - Confidence: 85%]

if prop := cls._get_from_file(...) treats an empty string as missing, so VIRL2_USER="", VIRL2_USER='', or VIRL2_USER= in the cwd file falls through to ~/.virlrc. Reproduced: cwd empty quoted user → home user.

The walrus check is pre-existing; unquoting single quotes newly turns '' into "" and hits the same path. Consider if prop is not None.

[hardikkaurani]

Good catch! Changed to prop = cls._get_from_file(Path.cwd(), prop_name) followed by if prop is not None: return prop (and removed or None on the home directory fallback). Empty values like VIRL2_USER="", VIRL2_USER='', or VIRL2_USER= in the cwd .virlrc now return "" immediately without falling through to home credentials, as verified by test_get_prop_empty_value_does_not_fall_through_to_home.

[tmikuska]

[Minor (New) - Confidence: 85%]

Good coverage for the prefix clash (VIRL2_USER vs VIRL2_USERNAME), comments, surrounding whitespace, and quote stripping. The parser rewrite still has no tests for:

• UTF-8 BOM on the first key
• UnicodeDecodeError / OSError on a present file
• empty values ("", '', VIRL2_USER=)
• VIRL2_PASS containing = (split("=", 1) is still correct; worth locking in)

[hardikkaurani]

Added dedicated unit tests in tests/test_configuration.py covering all 4 cases:

1. test_get_from_file_utf8_bom: verifies BOM on the first key (\xef\xbb\xbfVIRL2_USER) is stripped and matches.
2. test_get_from_file_decode_and_os_error_fails_loud: verifies UnicodeDecodeError and OSError raise and fail loud when reading an existing file.
3. test_get_from_file_empty_values_and_value_with_equals: verifies "", '', and bare empty values (EMPTY_BARE=), as well as passwords containing = characters (p@ss=word=123).
4. test_get_prop_empty_value_does_not_fall_through_to_home: verifies cwd empty values return "" without falling back to home credentials.

[tmikuska]

[Nit (New) - Confidence: 95%]

tmp_path already deletes the directory after the test, and monkeypatch restores cwd / Path.home. The os.remove teardown is unnecessary (same for home_virlrc below).

[hardikkaurani]

Removed! Cleaned up the redundant os.remove teardown in both cwd_virlrc and home_virlrc fixtures and removed the unused import os.

[tmikuska]

AI-Assisted Code Review Summary

This is an AI-assisted review of the .virlrc exact-key / UTF-8 / Windows-fixture changes. Please treat it as a second opinion; a human reviewer still owns the merge decision.

Verdict: request changes on the decode/BOM fallthrough (findings 1–2). Exact-key matching and the Path.home fixture fix are otherwise ready.

No linked Jira ticket (external contribution).

Minor

1. _get_from_file swallows read errors (virl2_client/virl2_client.py) — OSError / UnicodeDecodeError → None makes a present cwd .virlrc look missing, so _get_prop silently uses ~/.virlrc. Fail loud or warn when the file exists but cannot be read.
2. Use utf-8-sig — Notepad UTF-8 files include a BOM; the first key becomes \ufeffVIRL2_USER and does not match.
3. Empty values fall through to home (pre-existing walrus on _get_prop) — "" / '' / VIRL2_USER= in cwd is treated as missing. Single-quote unquote newly hits this for ''.
4. Test gaps after the parser rewrite — BOM, decode/OSError, empty values, and VIRL2_PASS containing =.
5. CI is Ubuntu-only (pre-existing, workflow unchanged) — the Windows Path.home fixture fix is not exercised in Actions. A local Windows run (or approving this fork’s workflow) is still needed.

Nit

6. Fixture os.remove of tmp_path files is redundant with pytest cleanup.

Things done well

• Exact key.strip() == prop_name fixes the real VIRL2_USER / VIRL2_USERNAME prefix clash (and the same class of VIRL2_PASS / VIRL2_PASSWORD match).
• Path.home is patched in home_virlrc instead of HOME, which is what Windows actually uses.
• monkeypatch.chdir restores cwd even when a test fails; RuntimeError from Path.home() is handled.
• New unit tests lock in the prefix + quote cases; DCO sign-off is present on all four commits.

Inline comments are on the relevant lines. I have not requested reviewers or changed the PR description.

[hardikkaurani]

Hi @tmikuska,

Thank you for the thorough review and helpful reproduction cases! I have addressed all the feedback and pushed the updates in commit 337c331:

1. UTF-8 BOM Handling (utf-8-sig): Switched .virlrc file reading and test fixtures to encoding="utf-8-sig". This cleanly strips leading BOM headers from Windows Notepad files so keys like \ufeffVIRL2_USER match properly. Added unit test test_get_from_file_utf8_bom.
2. Fail Loud on Unreadable / Decode Errors: When .virlrc exists (is_file() is True), _get_from_file now logs a warning with the file path and re-raises (OSError, UnicodeDecodeError) instead of catching and returning None. This fails loud and prevents corrupted local files from silently falling through to home credentials. Added unit test test_get_from_file_decode_and_os_error_fails_loud.
3. Empty Values Isolation: In _get_prop, updated the cwd check to if prop is not None: (and removed or None on the home directory fallback). Empty values in the cwd .virlrc ("", '', VIRL2_USER=) now return "" immediately without falling through to home credentials. Added unit test test_get_prop_empty_value_does_not_fall_through_to_home.
4. Values with Equals & Parser Tests: Added unit test test_get_from_file_empty_values_and_value_with_equals locking in empty value variants and passwords containing = characters (e.g. p@ss=word=123).
5. Fixture Cleanup: Removed the redundant os.remove teardown from cwd_virlrc and home_virlrc fixtures and removed the unused import os.

Validation

• Full test suite: All 852 tests passed cleanly (including all 4 new regression tests).
• Code style: Formatted with black and isort (--profile black).
• DCO: Signed off with git commit -s.

Please take a look whenever you have a moment. Thank you!