fix(config): ignore corrupt install method stamp - #33494
Conversation
aa4e19d to
3be103b
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused regression and best-effort fallback approach. The underlying defect still exists on current main.
Problems
- Current
detect_install_method()has two UTF-8 stamp reads after4440d77bf32d6267775be5eba2189e1ebde0b5b5: the code-scoped read athermes_cli/config.py:433and legacy home-scoped read at:445. Both catch onlyOSError(:436,:452). Applying the original one-site fix would leave the legacy compatibility path able to raiseUnicodeDecodeError. - The added test uses one
tmp_pathfor bothproject_rootandget_hermes_home, so it does not exercise the two current-main paths independently.
Suggested changes
- Apply the decode-error fallback to both stamp-read handlers.
- Add separate code/home directory regression cases for corrupt code-scoped and corrupt legacy home-scoped stamps, each asserting fallback to git detection.
Automated hermes-sweeper review.
| @@ -307,7 +307,7 @@ def detect_install_method(project_root: Optional[Path] = None) -> str: | |||
| method = stamp.read_text(encoding="utf-8").strip().lower() | |||
| if method: | |||
| return method | |||
| except OSError: | |||
| except (OSError, UnicodeDecodeError): | |||
There was a problem hiding this comment.
Current main has since split this into code-scoped and legacy home-scoped UTF-8 reads (hermes_cli/config.py:433 and :445, introduced by 4440d77bf). Salvage this same exception handling into both current handlers; otherwise a corrupt legacy home stamp still raises UnicodeDecodeError.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address this complex: merged #27843 introduced install-method stamping and the best-effort stamp read, while #33494 narrowly hardens that read against malformed UTF-8. The fix matches the reported failure mode, but its current diff covers only one of the two stamp-read paths now present on main.
Related pull requests
- #27843 [merged]
related— (+74/-7) — merged foundation and relevant reference implementation: introduced.install_methodstamping, precedence-based install-method detection, Docker handling, and the UTF-8 stamp read whose incomplete exception handling causes this defect. - #33494
related— (+11/-1) — keep open and revise: correctly catchesUnicodeDecodeErrorfor one stamp read and adds a fallback regression test, but the contributor keep_open review identifies a second legacy home-scoped read on current main that remains crashable; the diff must cover both handlers and test code-scoped and home-scoped corrupt stamps independently.
Suggested consolidation
Merge #33494 after applying the decode-error fallback to both current-main stamp-read handlers and adding independent regression coverage for each path. #27843 is already merged and remains the relevant originating implementation, not a duplicate to close; there are no competing duplicate PRs in this complex.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 10 kB of PR diffs, 6 kB of issue/PR text, 2 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
3be103b to
1c98ed7
Compare
What does this PR do?
Makes install-method detection tolerant of a corrupt
~/.hermes/.install_methodstamp file.detect_install_method()already treats a missing/unreadable stamp as non-fatal and falls back to managed/container/git/pip detection. However, the stamp read only caughtOSError; a malformed UTF-8 stamp raisedUnicodeDecodeError, which could makehermes update --checkor update-related recommendation paths crash before reaching the normal fallback logic.This keeps the stamp as a best-effort hint: if it cannot be decoded, Hermes ignores it and continues with the existing detection order.
Related Issue
Related / prior art:
UnicodeDecodeErrorsymptom in the post-update interactive config-migration prompt (input()), but this PR fixes a separate path indetect_install_method()..install_methodread path that this PR hardens.No dedicated issue exists for a corrupt
.install_methodstamp.Type of Change
Changes Made
hermes_cli/config.pyUnicodeDecodeErroralongsideOSErrorwhen reading~/.hermes/.install_method.tests/hermes_cli/test_pip_install_detection.py.install_methodcontains invalid UTF-8 (b"\xd0") and detection falls back to the project.gitmarker instead of crashing.How to Test
Focused regression/unit tests:
python -m pytest tests/hermes_cli/test_pip_install_detection.py tests/hermes_cli/test_cmd_update.py tests/hermes_cli/test_managed_installs.py -q -o 'addopts='Result:
Manual reproduction of the corrupt-stamp case:
Result after this change:
Static checks:
Independent pre-commit review:
Full-suite check attempted:
Local result: the run did not reach a passing state. GitHub CI currently reports failures in unrelated Kanban/Windows-guard tests that are outside this PR's diff (
tests/tools/test_kanban_tools.py::test_worker_complete_rejects_stale_run_idandtests/tools/test_windows_native_support.py::TestKanbanWaitpidWindowsGuard::test_source_gates_waitpid_loop). I also reproduced those two failures onorigin/mainat2d5dcfabc, where this PR's only changed files are not present.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — N/A, no user-facing option or workflow changecli-config.yaml.exampleif I added/changed config keys — N/A, no config keys changedCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/APath.read_text()and preserves existing fallback detectionScreenshots / Logs
Focused tests:
Manual corrupt-stamp smoke test: