Skip to content

fix(tools): support Tirith auto-install on Windows (#26044) - #26069

Closed
spranab wants to merge 1 commit into
NousResearch:mainfrom
spranab:fix/tirith-windows-install
Closed

fix(tools): support Tirith auto-install on Windows (#26044)#26069
spranab wants to merge 1 commit into
NousResearch:mainfrom
spranab:fix/tirith-windows-install

Conversation

@spranab

@spranab spranab commented May 15, 2026

Copy link
Copy Markdown

Summary

Fixes #26044. Tirith auto-install bailed out with unsupported_platform on Windows because _detect_target() only handled Darwin and Linux/Android. Tirith publishes an x86_64-pc-windows-msvc.zip release artifact, so the binary is in fact available — Hermes just couldn't reach it. With this PR, Windows users (including MSYS and git-bash) get the same auto-install + cosign + SHA-256 flow every other platform already has.

Why it's three changes, not one

The Windows release shape differs from the POSIX one in three ways, and a half-fix would leave a non-working install. So this PR moves all three in lockstep:

  1. Target triple_detect_target() adds a Windows branch returning x86_64-pc-windows-msvc. aarch64-Windows returns None until upstream Tirith ships that build (a single line to flip when it does).
  2. Archive format + binary name — Windows releases are .zip containing tirith.exe; other platforms are .tar.gz containing tirith. Two small helpers (_archive_extension, _binary_name) keep the branching explicit and testable. The Windows extraction path uses zipfile.ZipFile with the same defence-in-depth filters as the existing tar path (basename match, reject .., reject absolute paths, fall back to binary_not_in_archive / archive_extract_failed on bad input).
  3. os.chmod is skipped on Windows. Windows derives executability from file extension + ACLs, not the POSIX user/group/other-execute bits, so the existing os.chmod(dest, ... | S_IXUSR | S_IXGRP | S_IXOTH) would have been a no-op at best and a permissions-deny in some environments at worst.

All three shutil.which("tirith") and os.path.join(_hermes_bin_dir(), "tirith") call sites are updated to use _binary_name(), so user-installed-vs-auto-installed binary resolution stays consistent across platforms.

Tests

17 new cases under TestWindowsTargetDetection, TestPlatformHelpers, and TestWindowsInstallFlow:

Class Cases Covers
TestWindowsTargetDetection 4 _detect_target returns the right triple for AMD64 + x86_64 spellings; refuses aarch64-Windows until upstream ships; doesn't accidentally match unrelated systems (regression-guard for the new branch)
TestPlatformHelpers 7 _binary_name, _archive_extension, _is_windows_target in isolation across every supported target triple including a None / empty-string guard
TestWindowsInstallFlow 6 End-to-end install with a mocked downloader exercising the Windows path: zip URL selected (not tar.gz), tirith.exe written to <hermes_bin>/tirith.exe, no chmod on the destination, path-traversal entries rejected, missing-binary archive surfaces binary_not_in_archive, corrupt zip surfaces archive_extract_failed
python -m pytest tests/tools/test_tirith_security.py -p no:xdist -o addopts=
77 passed, 3 failed

The 3 failures are pre-existing Path.home() expansion issues on Windows-local (TestHermesHomeIsolation::test_failure_marker_respects_hermes_home, TestHermesHomeIsolation::test_get_hermes_home_fallback, TestDiskFailureMarker::test_install_failed_recovers_from_hermes_bin). They fail identically on unmodified upstream main, so this patch doesn't introduce them — confirmed by stashing the patch and re-running.

python -m ruff check tools/tirith_security.py tests/tools/test_tirith_security.py
All checks passed!

python -m mypy --ignore-missing-imports --follow-imports=silent tools/tirith_security.py
Found 1 error in 1 file

The 1 mypy error (Incompatible return value type (got "str | bool", expected "str") on the _INSTALL_FAILED sentinel) is pre-existing on main — same line, same message before this patch.

Verification

  • Verified Tirith release artifact list at https://github.com/sheeki03/tirith/releases/latest includes tirith-x86_64-pc-windows-msvc.zip, checksums.txt, checksums.txt.sig, checksums.txt.pem — same cosign + checksum chain as the POSIX artifacts.
  • Reproducer from the bug body still hits the unsupported_platform marker on main; this branch resolves it.
  • No Windows-side runtime test in CI since GitHub Actions Windows runners would also have to actually download from Tirith's release page — the mocked-installer tests under TestWindowsInstallFlow exercise every branch of the new code instead.

Fixes #26044

Before this change, `_detect_target()` returned `None` for
`system == 'Windows'` because it only handled Darwin and Linux/Android.
That caused `_install_tirith` to bail out with the "unsupported_platform"
failure marker even though Tirith ships an
`x86_64-pc-windows-msvc.zip` release artifact — leaving Windows users
permanently on pattern-matching-only command scanning.

Three things had to move in lockstep, since the Windows release shape
differs from the POSIX one:

1. `_detect_target` adds a Windows branch returning
   `x86_64-pc-windows-msvc`. aarch64-Windows returns `None` until
   upstream Tirith adds that build.
2. `_install_tirith` selects `.zip` vs `.tar.gz` and `tirith.exe`
   vs `tirith` via two small helpers (`_archive_extension`,
   `_binary_name`). The Windows path extracts the binary via
   `zipfile.ZipFile` with the same defence-in-depth filters as the
   tar path (basename match, reject `..`, reject absolute paths).
3. `os.chmod` is skipped on Windows — Windows derives executability
   from file extension + ACLs, not the POSIX user/group/other-execute
   bits, so the previous `os.chmod(dest, ... | S_IXUSR | S_IXGRP | S_IXOTH)`
   call would have been a no-op at best and a permissions-deny at worst.

All three `shutil.which("tirith")` and
`os.path.join(_hermes_bin_dir(), "tirith")` call sites are updated
to use `_binary_name()`, so user-installed-vs-auto-installed binary
resolution stays consistent across platforms.

Tests
-----
17 new test cases under `TestWindowsTargetDetection`,
`TestPlatformHelpers`, and `TestWindowsInstallFlow` cover:

- `_detect_target` returns `x86_64-pc-windows-msvc` for AMD64 and
  x86_64 spellings; returns `None` for ARM64-Windows (until upstream
  ships) and unsupported systems (regression-guarding the new branch).
- Helper functions `_binary_name` / `_archive_extension` /
  `_is_windows_target` are tested in isolation for every supported
  target triple.
- End-to-end install flow with a mocked downloader exercising the
  Windows path: zip archive URL selected, `tirith.exe` written to
  `<hermes_bin>/tirith.exe`, no `chmod` on the destination,
  path-traversal entries rejected, missing-binary archive surfaces
  `binary_not_in_archive`, corrupt zip surfaces `archive_extract_failed`.

Full file: 60 pre-existing tests still pass; 3 pre-existing failures
on Windows-local (Path.home() expansion quirks) are unchanged by this
patch. ruff + mypy (against tools/tirith_security.py specifically) at
the same baseline as upstream main.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management duplicate This issue or pull request already exists labels May 15, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing fix with #26068 for the same issue #26044. Both address MSYS/git-bash Tirith auto-install. Note: #23050 (merged) already added base Windows support — this addresses an MSYS-specific edge case where platform.system() returns a different value.

@spranab

spranab commented May 15, 2026

Copy link
Copy Markdown
Author

Thanks for the cross-reference — agreed, #26068 is the right baseline for #26044 and #26069 is the weaker fix.

The decisive gap: I only match system == "Windows", while the bug title explicitly says MSYS/git-bash — on those shells platform.system() returns MSYS_NT-10.0 / MINGW64_NT-* / CYGWIN_*, none of which my branch handles. So as-written, #26069 doesn't actually fix the reported case. LeonSGP43's _is_windows_system() matches all four prefixes correctly.

(#23050 also seems to be an empty merge — 0 files / 0 commits on the PR record — so the assertion that base Windows support already landed isn't quite right; current _detect_target() on main still only handles Darwin / Linux / Android. But that doesn't change which of the two open fixes should land.)

Closing this PR. A few things in #26069 that may be worth pulling forward into #26068 (or as a small follow-up once it merges); happy to send any of them as a tightly-scoped PR against #26068's branch:

  • Zip-extraction defence-in-depth: my _install_tirith branch rejects entries with .. and entries starting with / or \ — relevant since zipfile will happily extract absolute paths on POSIX hosts. fix(tirith): support Windows/MSYS auto-install #26068's _extract_release_binary could use the same filters.
  • Failure-reason parity: I added two new failure tags (archive_extract_failed for corrupt zips and binary_not_in_archive for zips missing tirith.exe) so the existing _INSTALL_FAILED marker + 24h retry logic distinguishes "transient corruption" from "wrong archive shape". Currently fix(tirith): support Windows/MSYS auto-install #26068 falls back to the generic install error path for these.
  • Path-traversal / malformed-zip / missing-binary tests under TestWindowsInstallFlow. Roughly 6 cases beyond the install-flow happy-path, easy to lift over.
  • aarch64-pc-windows-msvc future-proofing: I explicitly return None for ARM64-Windows with a comment noting "flip this when upstream ships". The intent matches fix(tirith): support Windows/MSYS auto-install #26068's current behaviour but the test makes the contract explicit.

Apologies for the duplicate filing — I checked existing PRs against #26044 before opening but missed #26068 which was opened 16 minutes earlier (under a slightly different title). Will widen the pre-filing search next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Tirith security scanner fails to install on Windows (MSYS/git-bash) - "unsupported_platform"

2 participants