fix(acp): stop the binary-cache precheck from contradicting the launch path - #717
Open
Adam-Dalloul wants to merge 1 commit into
Open
Conversation
…h path Agent settings could show "Version Status: pass" next to "Binary cache: warn" for the same OpenCode install, and the warn told the user to download a binary they already had. Two separate reasons, both in the read path. `check_binary_environment` only accepted a user-installed CLI in place of a codeg-managed download when the agent's registry entry carried a `dir_entry`, which is Cursor and nothing else. Every other binary agent fell through to "Binary is not installed. Download it from Agent Settings before connecting." even though `connection.rs` resolves exactly that CLI and spawns it, the connect gate in `verify_agent_installed` accepts it, and the version card counts it as installed through `detect_local_version`. OpenCode is a single-file binary agent, so a user whose only copy was `~/.local/bin/opencode.exe` got the contradiction on every settings open. The check now reads the same two facts in the same order the launcher uses, split into a pure `build_binary_cache_check` so the verdict is testable, and the passing message names the file that will be launched instead of pointing at a download button for one already present. `installed_binary_path` also deleted any cached binary whose first four bytes it could not read. `is_binary_file_compatible` returned false both for "opened it and the header belongs to another platform" and for "could not open it at all", and the caller could not tell those apart. On Windows the second case happens with nothing actually wrong: the anti-virus real-time scanner holds a freshly written ~180 MB agent binary for a moment, and a running agent holds its own image. A preflight landing in that window removed an install that had just succeeded, after which the version card fell back to the system copy and read pass while the cache check read nothing installed. The probe now returns a three-way `BinaryFormat` and only a positive wrong-platform verdict evicts. A file too short to hold any header stays evictable, since that is a permanent fact, and the post-download check stays strict because there codeg owns the file it just wrote. Reported in xintaofei#631.
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
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.
Follow-up to #631, from the reporter's later detail: after a download that reported success, Version Status read
passwhile Binary cache readwarn. Both cards describe the same file, so one of them was wrong. Two independent causes, both in the read path, neither of them the download.This is separate from #713, which fixes the fake uninstall in the same issue.
1. The precheck refused the fallback the launcher uses
check_binary_environmentaccepted a user-installed CLI in place of a codeg-managed download only when the registry entry carried adir_entry:dir_entryisSomefor Cursor and nothing else. Every other binary agent, OpenCode included, fell through to"Binary is not installed. Download it from Agent Settings before connecting."Three other places already accept that same CLI:
acp/connection.rspicks the best cached binary and otherwise spawnsresolve_system_agent_binary(cmd), loggingNo cached binary; using system <path> from PATH.verify_agent_installedincommands/acp.rsgates connect onfind_best_cached_binary_for_agent(...).is_some() || resolve_system_agent_binary(cmd).is_some().detect_local_version(and the status/list paths) report the system binary's--versionasinstalled_version, which is what the Version Status card renders.So on a machine whose only OpenCode is
~/.local/bin/opencode.exe, the version card reported it installed, connect accepted it, the launcher spawned it, and the precheck alone called it missing and pointed at a download button. That is the reported pass/warn split, on every settings open, with or without a download in between.The check now reads the same two facts in the same order the launcher uses. The decision is split into a pure
build_binary_cache_checkso the verdict is unit-testable without a cache dir or a PATH, and the passing message names the file that will actually be launched:Warnis unchanged when there is genuinely nothing to launch, which is the state connect rejects.passedis unaffected either way, since this item was neverFail.2. A cached binary codeg could not open was deleted
installed_binary_pathtreated an unreadable file as a foreign-platform binary:is_binary_file_compatiblereturnedfalseboth for "opened it and the header belongs to another platform" and for "could not open it at all", and the caller could not tell those apart. On Windows the second case happens with nothing wrong: the anti-virus real-time scanner holds a freshly written ~180 MB agent binary for a moment, and a running agent holds its own image. A preflight landing in that window removed an install that had just succeeded. Right after that,acp_detect_agent_local_versionfalls back to the system copy and the version card readspasswhile the cache check reads nothing installed, which is exactly the state described after a successful download.The probe now returns a three-way
BinaryFormat, and only a positive wrong-platform verdict evicts. A file too short to hold any header stays evictable, since that is a permanent fact rather than a transient one. The post-download check keeps the strict boolean: there codeg owns the file it just wrote, so anything short of a readable, correct header must fail the install loudly rather than cache something unverified.Tests
Nine new Rust unit tests, all failing before the change:
preflight.rs: a system install passes for a single-file agent and names the path; nothing cached and nothing on PATH still warns; a cached binary wins over a system install so the card never describes a file codeg will not spawn; an older cache passes and names the recommended version; every branch keeps thebinary_cachedid the frontend keys on.binary_cache.rs: onlyIncompatibleevicts; an unopenable path probes asUnreadable; empty and 2-byte files probe asIncompatible; a readable header decides compatibility.Reverting either production change fails its tests: the preflight arm back to
Warnfailssystem_install_reports_ready_for_single_file_agents, and foldingUnreadableback into "not usable" failsonly_a_wrong_platform_verdict_evicts_a_cached_binaryandtruncated_file_probes_as_incompatible.No frontend file changes, no new user-facing string in
i18n/messages/(preflight check messages are backend English, like every other item in this list), and no behaviour change for Cursor, which keeps passing on a system install exactly as before.