ci: fail the build when primary.manifest drifts from app_strings.h - #10
Merged
Conversation
Pressing OK could call CheckAndApplyAutoSwitch() three times: once in the auto-switch settings block, once in the base-mouse-count block, and once in the forced re-apply after the cached device state is discarded. Each call enumerates raw input devices, retrying GetRawInputDeviceList up to three times, so a single click cost up to three enumerations. The repeats were harmless — the direction is persisted before all three calls and ApplyMouseOrientation() sets an absolute value rather than toggling — but only the last call could do useful work in the common case, since the first two early-return on unchanged state. Remove the two earlier calls and keep the forced re-apply at the end of the handler. By that point every setting the check reads is persisted, so one pass applies them all; the first call previously ran before SetBaseMouseCount(), so it could not see a changed base count anyway. Dropping the base-mouse-count call also stops a re-apply from running when the auto-switch flag itself failed to persist: that call was gated on autoSwitchEnabled alone, not on autoSwitchWritten. The surviving call is gated on both, matching 3a00a42. Closes #2 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jonathan Bnayahu <bnayahu@il.ibm.com>
build.yml declared no permissions: block, so the workflow ran with the default GITHUB_TOKEN permissions. The job only checks out the repository, builds, and uploads an artifact, so read access to contents is all it needs. actions/upload-artifact authenticates with the Actions runtime token rather than GITHUB_TOKEN, so it is unaffected. Nothing was broken; this narrows the token to what the job actually uses. The comment warns against copying the block into release.yml, which genuinely needs contents: write for `gh release create`. Closes #3 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jonathan Bnayahu <bnayahu@il.ibm.com>
…order Both workflows derived the source version with a single alternation piped through `paste`: grep -oP 'APP_VERSION_(MAJOR|MINOR|PATCH)\s+\K\d+' ... | paste -sd. That takes the macros in the order they appear in the header rather than in semantic order. They currently appear as MAJOR, MINOR, PATCH on adjacent lines, so the result is correct today, but nothing enforced it — reordering them silently produced a transposed version string. The existing failure mode was fail-closed: release.yml's tag gate rejects a mismatch loudly rather than publishing something mislabelled. This makes the extraction correct in the first place, so a reorder is a non-event instead of a confusing release failure. Extract each component by name, and assert all three are non-empty — the old form would emit a short "1.2" if a macro went missing, which could then match a substring of the binary's version strings. Closes #6 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jonathan Bnayahu <bnayahu@il.ibm.com>
primary.rc references primary.manifest as an opaque file, so windres never runs the preprocessor over it and its assemblyIdentity version cannot be derived from APP_VERSION_*. It has to be bumped by hand, and nothing enforced that — the only safeguard was a manual release-checklist step. Assert it in CI instead: compare the manifest's assemblyIdentity version against MAJOR.MINOR.PATCH.0 from app_strings.h, in build.yml (every push and PR) and in release.yml (refuse to ship a mismatch). Drift now fails loudly instead of shipping silently. Of the options in the issue, this leaves the build path untouched. Generating the manifest from a template in build.sh was rejected because README.md documents a direct windres/g++ invocation that bypasses build.sh — build.yml verifies that command still works — so a generated manifest would break or stale out that documented path. The manifest comment now records that CI enforces the match, and that the check expects name= and version= on one line. Closes #5 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Jonathan Bnayahu <bnayahu@il.ibm.com>
bnayahu
force-pushed
the
ci/manifest-version-drift-check
branch
from
August 11, 2026 18:42
cb6a634 to
67d3eea
Compare
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.
Closes #5
Problem
app_strings.his the single source of truth for the version, andprimary.rcpicks it up through the preprocessor. Butprimary.rcreferences the manifest as an opaque file (1 RT_MANIFEST "primary.manifest"), sowindresnever preprocesses it and itsassemblyIdentityversion is a hardcoded literal that must be bumped by hand. Nothing enforced that.Currently in sync; the risk is the next version bump.
Which option, and why
The issue listed three. This PR implements the CI assertion.
README.mddocuments a directwindres/g++invocation that bypassesbuild.shentirely, andbuild.ymlhas a step verifying that documented command still works. A generated manifest would either be missing on that path or silently stale, so this option trades a version-drift risk for a build-path divergence.Change
A source-only step comparing the manifest's
assemblyIdentityversion againstMAJOR.MINOR.PATCH.0fromapp_strings.h(mirroring whatVERSIONINFOemits), added to:The extraction uses per-macro greps rather than the order-dependent one-liner, so this new step isn't subject to #6.
The manifest comment now records that CI enforces the match.
Verification
Ran the check against mutated inputs — all five drift cases fail closed, and the in-sync tree passes:
app_strings.hbumped, manifest left behind (the real scenario)app_strings.hleft behindversion=attribute removedAlso confirmed:
name="Primary"identity, not theMicrosoft.Windows.Common-Controlsdependency (exactly 1 match in the file)../build.shstill succeeds and the manifest is still embedded (dpiAwareness,Common-Controls,asInvokerall present in the binary); the manifest is still well-formed XML.Known limitation, documented in the manifest comment: the check expects
name=andversion=on the same line of the element. Reformatting that element makes the check fail rather than silently skip.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com