fix(cli): align update protocol identity with the installed binary - #1799
ScriptedAlchemy wants to merge 5 commits into
Conversation
The GitHub upgrade path stored the release tag as the maintenance window's expected version. The daemon advertises build_version(), which is that release plus +sha, and readiness compares the two exactly, so update refused the binary it had just installed. Ask the published binary for --version, the same identity the package-manager path already reports. An unreadable binary stays unversioned instead of substituting the tag. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb545b4e5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // daemon advertises `{release}+{sha}`, and readiness compares those | ||
| // strings exactly, so the window must expect the binary's own | ||
| // identity rather than `latest`. | ||
| version: installed_protocol_identity(binary.as_deref()), |
There was a problem hiding this comment.
When a release asset is accidentally built from a different version than its tag—for example, the v1.2.4 archive contains a stale 1.2.3+sha binary—this now adopts 1.2.3+sha as the expected identity, so the restarted daemon is accepted even though the command reports a successful upgrade to v1.2.4. Previously the bare latest value caused readiness to expose this packaging mismatch. Validate that the self-reported identity's release component equals latest before accepting its build metadata, preserving the binding between hosted release provenance and installed bytes.
AGENTS.md reference: AGENTS.md:L135-L140
Useful? React with 👍 / 👎.
…-protocol-identity-817e
This PR's restore_readiness_accepts_the_advertised_build_and_rejects_its_release_tag asserted DaemonProtocolState::IdentityMismatch when a daemon advertising 0.1.0-beta.47+<sha> is probed against the bare release 0.1.0-beta.47. Master 5e03dfa landed versions_name_same_build and makes exactly that pair Ready, with probe.rs a_daemon_naming_its_commit_is_ready_against_its_bare_release pinning it. After merging master the test failed (left: Ready, right: IdentityMismatch). Its other half, Ready against an exactly equal build, is already covered by that probe.rs module, so the file returns to master's version rather than being rewritten. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The new installed_protocol_identity and the UpgradeOutcome::Installed version field justified themselves with "readiness compares those strings exactly". Master 5e03dfa replaced that raw == with versions_name_same_build, so the stated reason no longer describes the code. The change itself is still right, for the opposite reason: because build metadata a side omits is ignored, a bare release tag as expected_version matches EVERY build of that release and silently disables the same-release different-commit skew detection 367a44a added. Reporting the installed binary's own identity keeps the window pinned to one build. Comments only; no behavior change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Review lane result: MERGE-AFTER-FIX on its own, but this PR, #1799 and #1809 implement the same one-line producer fix in run_versioned_upgrade (probe the installed binary's --version instead of reporting the release tag). Exactly one can land; #1822 is being taken because it also removes the duplicated probe in run_delegated_upgrade. This PR will be closed as superseded once #1822 merges via integration batch #1848. Thanks for the fix; the reasoning in the body was correct. |
|
Closing per Session Miner: update-identity already on master via #1797. |
2 participants
Summary
tracedecay updateon the GitHub-release path now reports the installed binary's--versionas the maintenance window's protocol identity.{release}+{sha}). Readiness still compares it exactly, so a bare release tag is no longer stored asexpected_version.Motivation
tracedecay updateinstalledv0.1.0-beta.47, restarted the daemon, and refused the binary it had just started:The daemon also logged
daemon_version_skewon every readiness poll. Both strings came from one binary.The only actor that assigned the mismatch is
run_versioned_upgrade. It stored the GitHub release tag. The package-manager path already asks the linked binary for--version, and the daemon advertisesbuild_version(), which is that release plus+<sha>. Readiness and the handshake compare those strings with exact equality, which is what distinguishes two checkout builds of one release.#1797 loosens that comparison so a missing commit is "less specific, not different." That papers over the update path still emitting a different identity, and it would treat a stale
+shadaemon as ready whenever the expected version is the bare tag. This PR does not absorb that comparison. The release tag stays the operator-facing version; the protocol identity is the binary's own advertisement.Changes
crates/tracedecay-cli/src/upgrade.rs: direct installs reportinstalled_protocol_identity(the binary's--version) instead of the release tag.crates/tracedecay-daemon-control/src/service/update_restore_tests.rs: readiness accepts that advertised build and still rejects the bare tag.Test plan
cargo nextest run --workspace --no-fail-fastpassescargo clippyhas no new warningsupgrade::tests::version_probe— 6 passed, includinga_direct_install_reports_the_binary_identity_not_the_release_tag(a binary that printstracedecay 0.1.0-beta.47+84598a0b…is the reported identity; a missing binary isNone, not the tag)service::update_restore_tests— 2 passed. The answering daemon isReadyagainst0.1.0-beta.47+84598a0b…andIdentityMismatchagainst bare0.1.0-beta.47.Checklist
CHANGELOG.mdupdated (under[Unreleased]if no version bump).envfiles includedChangelog is release-please generated; this commit does not hand-edit it.
Dispatch SHA:
eb545b4e5b93f2e39a5924aebb41b0e7c2bde6b0