Conversation
The tray's self-update hands update.Apply a TargetPath of os.Executable(). cmd/mcpproxy-tray is the only importer of internal/tray, so that path is always mcpproxy-tray — but the extraction picked the wrong member out of the archive: - applyTarGzUpdate selected on strings.HasSuffix(name, "mcpproxy"), and a release .tar.gz ships mcpproxy AND mcpproxy-tray side by side (.github/workflows/release.yml stages both, core first). A "successful" update therefore wrote the headless core over the tray executable, leaving the user with an mcpproxy-tray that is really a core. - applyZipUpdate applied the FIRST non-directory entry regardless of name. Windows archives list mcpproxy.exe first, so this was the same bug, and any future layout change would silently install some other file. Both now extract the member matched on its exact base name via a new trayBinaryName(), and fail closed when the archive does not carry it. The matching logic is the one cmd/mcpproxy already had; it moves to internal/updatecheck (ExtractBinary, MaxArchiveMemberBytes) so both self-update paths share one implementation, the way ParseChecksums and VerifyFileSHA256 were shared. cmd/mcpproxy keeps thin delegating wrappers so its own tests and call sites are unchanged. The tray also gains the 512 MB per-member cap for free.
Round 1 of cross-model review: the tray self-update now extracts the
mcpproxy-tray member by name and fails closed when it is absent, but
prerelease.yml archived only ${CLEAN_BINARY}. RC builds track the rc channel
by construction (App.includePrereleases), so every prerelease tray would have
hit 'archive does not contain mcpproxy-tray'. Mirrors what release.yml
already does for stable archives.
Round 2 of cross-model review. prerelease.yml built mcpproxy-tray for darwin only, so the windows branch of the archive step could never fire. The Windows RC tray does reach users — build-windows-installer.ps1 builds it, but that runs AFTER the archives — and it self-updates out of this job's .zip, so the zip must carry the mcpproxy-tray.exe member. Matches release.yml, including the archived tray being unsigned (SignPath signs installers, not archive members).
Round 3 of cross-model review. scripts/build-windows-installer.ps1 has always passed -H windowsgui, but the tray copy placed in the release/prerelease archives did not, so it links as a console executable and pops a console window on launch. This was latent until now: the tray self-update used to install the CORE binary over the tray, so the archived tray was never actually installed. Now that it selects the mcpproxy-tray member correctly, a Windows self-update would have swapped the installer's GUI binary for a console one.
…hives Round 3 of cross-model review (zcode), test-rigour findings: - TestApp_SelfUpdate_InstallsTheTrayBinaryNotTheCore gave both tray members the same payload, so a selection that ignored runtime.GOOS still passed. Members now carry payloads naming them, and the expectation is an independent literal rather than payloadFor(trayBinaryName()) — deriving it from the function under test made the assertion move with any bug in it. - The nested base-name case covered tar.gz only; zip is a separate code path and now has its own subtest. - The workflows' [ -f mcpproxy-tray ] gate degraded silently to shipping a tray-less archive. Since the self-update fails closed without that member, a missing binary now fails the build in both release.yml and prerelease.yml.
Deploying mcpproxy-docs with
|
| Latest commit: |
5a4a00c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://005c1e94.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-tray-self-update-extract.mcpproxy-docs.pages.dev |
Round 4 of cross-model review (zcode), three findings: - internal/tray is behind '!nogui && !headless && !linux', and NOTHING in the PR gate compiles it: pr-build.yml tests with -tags nogui, and unit-tests.yml narrows its pull_request matrix to ubuntu-latest. The self-update regression tests therefore only ran on the push-to-main macOS/Windows legs — after merge. native-tests.yml gains a path-gated macOS go-tray-test job, following the workflow's existing required-safe pattern (a skipped job reports green, so non-tray PRs spend no macOS runner). - TestTrayBinaryName's base-name assertion tested the test's own literal, so filepath.Base(want) == want always held and the branch could never fire. It now checks the return value; verified by making trayBinaryName() return 'bin/mcpproxy-tray', which the fixed assertion catches. - The build-step guard added in the previous commit sat ~250 lines from the conditional that actually degrades silently, and only caught 'go build exited 0 but wrote no file'. It moves to the archive-assembly gate itself, as an else branch, so a future refactor splitting build from archive cannot reopen the original bug. Linux and the server edition fall through untouched - neither ships a tray.
This branch has not been deployed
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.
Problem
The tray's self-update installed the core binary over the tray binary.
internal/trayis imported only bycmd/mcpproxy-tray, and its self-update passesupdate.Options{TargetPath: os.Executable()}— so it replaces itself. But release archives ship both binaries at the root, core first (release.ymlFILES_TO_ARCHIVE=mcpproxy+mcpproxy-tray[.exe]), and the member selection picked the wrong one:applyTarGzUpdatematchedstrings.HasSuffix(header.Name, "mcpproxy"), which hits the core member.applyZipUpdateapplied the first non-directory entry regardless of name.Compress-Archivelistsmcpproxy.exefirst, so this was the same live bug on Windows, not just latent fragility.A "successful" update therefore left the user with an
mcpproxy-traythat is really a headless core. Reachable on tarball/zip installs; DMG app-bundles and Homebrew are gated off byisAppBundle()/isHomebrewInstallation().Found while adding checksum verification in #1343 and deliberately left out of scope there.
Fix
Both formats now extract the member matched on its exact base name and fail closed when it is absent.
The matching logic already existed and was correct in
cmd/mcpproxy(extractBinary/extractFromZip/extractFromTarGz). It moves tointernal/updatecheckasExtractBinaryso both self-update paths share one implementation — the same treatmentParseChecksums/VerifyFileSHA256got in #1343.cmd/mcpproxykeeps thin delegating wrappers, so its call sites and tests are unchanged. The tray also picks up the 512 MB per-member cap for free.Release-pipeline fixes this exposed
Making extraction fail closed turned three latent packaging gaps into live ones:
prerelease.ymlarchived only${CLEAN_BINARY}archive does not contain mcpproxy-tray.prerelease.ymlbuilt the tray for darwin only-H windowsguibuild-windows-installer.ps1has always passed it. Without it, a self-update swaps the installer's GUI binary for a console one that pops a console window. Fixed inrelease.ymltoo — stable had the same latent defect.Both workflows now also fail the build if the tray binary is missing, rather than silently shipping a tray-less archive.
Tests
internal/tray/update_extract_test.go(build-tagged!nogui && !headless && !linux, run on darwin) covers: the tray member winning over a core member listed first, in both formats; refusal when no tray member exists; base-name matching against a nested member and amcpproxy-mcpproxy-traydecoy; andtrayBinaryName()per GOOS.Each archive member carries a payload naming it, and the expectation is an independent literal rather than being derived from
trayBinaryName()— deriving it from the function under test made the assertion move with any bug in it.Verified the tests bite by mutating the implementation: forcing
trayBinaryName()to"mcpproxy"reproduces the original bug (installed "BINARY:mcpproxy", want "BINARY:mcpproxy-tray"), and forcing it to ignoreruntime.GOOSalso fails.Verification
go test -race ./internal/...— 66 packages, exit 0go test -race ./internal/tray/... ./internal/updatecheck/... ./cmd/mcpproxy/...— green-tags serverCI-parity race tests green--build-tags server) clean on every changed filecodex gpt-5.6-sol(3 findings, all verified and fixed) pluszcode(no correctness defects; 3 test-rigour items, all fixed)Note on the base branch
This is stacked on
fix/verify-update-artifact-checksum(#1343), notmain: theapplyUpdateFnseam and thebuildTarGz/buildZiptest helpers this builds on exist only there, and both PRs rewrite the same two functions. Merge #1343 first, or say the word and I'll rebase ontomain.