Skip to content

fix(updater): accept only Monolith binary release assets - #104

Open
kunkunGames wants to merge 1 commit into
tumourlove:masterfrom
kunkunGames:agent/updater-binary-zip-fail-closed
Open

fix(updater): accept only Monolith binary release assets#104
kunkunGames wants to merge 1 commit into
tumourlove:masterfrom
kunkunGames:agent/updater-binary-zip-fail-closed

Conversation

@kunkunGames

@kunkunGames kunkunGames commented Jul 24, 2026

Copy link
Copy Markdown

Summary

Make release selection fail closed: the updater now accepts only explicit Monolith binary ZIP assets and never falls back to GitHub-generated source archives.

Problem

The updater previously allowed a release source archive to become the installation URL when no suitable uploaded binary was found. Loose filename matching could also confuse neighboring engine versions such as UE5.8 and UE5.80, or let an unrelated plugin asset influence per-engine selection.

That turns release metadata mistakes into unsafe installations instead of a clear "no compatible update" result.

Solution

  • extract release-asset selection into a side-effect-free selector
  • consider only uploaded assets named Monolith-*.zip
  • when a release publishes engine-specific Monolith builds, require the exact current UE major/minor suffix
  • ignore foreign ZIPs and GitHub zipball_url source archives
  • return no candidate when the release does not contain a compatible Monolith binary
Release contents Selection result
One eligible Monolith binary ZIP Select that uploaded asset
Engine-specific Monolith ZIPs Select only the exact current UE version
Source archive, unrelated ZIPs, or wrong-engine binaries only Fail closed with no candidate

Verification

  • UE 5.8 protected Editor build passed and recompiled/relinked MonolithCore
  • Monolith.Updater.ReleaseSelection: 7/7 tests passed, with zero test warnings, zero errors, and process exit code 0
  • isolated UE 5.7 Editor build passed: 430/430 actions
  • differential static analysis introduced zero findings relative to the base revision
  • git diff --check passed
  • detailed evidence: Docs/testing/2026-07-26-updater-binary-release-selection.md

Compatibility and risk

Download, checksum, extraction, and self-replacement behavior are unchanged after a valid asset is selected. The intentional behavior change is that a malformed release now reports no compatible candidate instead of attempting to install a source archive or another binary.

This PR has no public API or configuration migration.

Visual evidence

Not applicable: this is updater selection and regression-test work with no visual or editor-facing UI change.

@kunkunGames
kunkunGames marked this pull request as ready for review July 25, 2026 01:23
@kunkunGames kunkunGames changed the title fix: reject source zipballs in the auto-updater fix(updater): reject GitHub source zipballs so only binary release assets can install Jul 25, 2026
@kunkunGames

Copy link
Copy Markdown
Author

Merge-order note for the six open PRs from me (#104, #106, #107, #108, #112, #113) — posting once here and linking from the others.

They conflict, but only in CHANGELOG.md. I ran a pairwise trial merge (git merge-tree --merge-base=tumourlove/master) across all 15 pairs. Every pair conflicts, and in every case the only conflicting file is CHANGELOG.md, because each PR inserts its entry at the same ## [Unreleased] anchor. One pair has a second conflict: #104 × #112 also conflict in Docs/specs/SPEC_MonolithCore.md, where both edit adjacent rows of the class table.

No source file conflicts in any pair.

Suggested order (each is independently mergeable; this order just minimises rebases):

# Title Why here
#104 fix(updater) reject source zipballs Independent; fixes a broken install for Blueprint-only users
#106 fix(blueprint) validate struct save_path Now also carries the FullyLoad removal — #105 is closed and folded in
#107 fix(material) validate creation destinations
#108 fix(niagara) validate destinations Also rewrites the shared SPEC_MonolithCore.md package-path routing/backlog line, so it should land after #106 and #107 for that list to read correctly
#112 feat(core) add monolith_find
#113 feat(index) structured-content FTS

I will do the rebases — you should not have to resolve CHANGELOG.md by hand. Merge in whatever order suits your batch and just say the word (or merge one and leave the rest; I watch for it), and I will rebase the remainder and re-verify each with a build plus its automation tests.

If you would rather not carry six separate changelog entries at all, I am equally happy to collapse the four fix(...) entries into a single bullet in whichever PR you merge last.

@tumourlove

Copy link
Copy Markdown
Owner

Confirmed the bug and this is going into the next release, v0.21.4. Keeping it out of v0.21.3 was deliberate: it is the only change in that batch touching the auto-update path, and that path needs a full end-to-end Windows rehearsal before it ships. Bundling it with seven compile-and-ship fixes would have put pressure on that gate, which is exactly how v0.14.7 shipped an install path that could never succeed and went unnoticed for fifteen release cycles.

Verified your diagnosis against master: the fallback at MonolithUpdateSubsystem.cpp:281-285 is real and reachable whenever a release carries no usable binary zip.

Your PR also prompted a change that matters more than the code fix. gh release create publishes the release and then uploads assets, so during that window /releases/latest returns a tag with zero or partial assets — the exact shape that trips the zipball fallback. Every deployed client from v0.14.7 to v0.21.2 has that code and cannot be reached by any fix to HEAD. As of v0.21.3 releases are published as a draft and only flipped live once a gate confirms all three assets are uploaded and every SHA marker matches a fresh hash. That protects everyone already out there, retroactively. Good find.

Two deviations when it lands, both worth flagging:

I am not using an exact filename match. The tag and the asset name are coupled by convention only — the script builds the name from -Version while the tag is typed by hand — so an exact match would turn any future drift into a permanent fail-closed no-update. Going with a prefix/suffix match instead: still kills the zipball, still rejects -UE5.70.zip.

The description says the change also removes the bPerEngineRelease mis-trigger. It does not; that flag is set separately at :229-232 and is untouched. Worth fixing, but as its own change.

I land contributor fixes as my own commits rather than merging the branch — shipped history stays single-author, and you will be credited in the v0.21.4 notes. Leaving this open until then.

@kunkunGames
kunkunGames force-pushed the agent/updater-binary-zip-fail-closed branch from 37a63cb to c68c1c1 Compare July 26, 2026 14:44
@kunkunGames kunkunGames changed the title fix(updater): reject GitHub source zipballs so only binary release assets can install fix(updater): accept only Monolith binary release assets Jul 26, 2026
@kunkunGames
kunkunGames force-pushed the agent/updater-binary-zip-fail-closed branch from c68c1c1 to 57ad2d4 Compare July 26, 2026 15:25
@kunkunGames

Copy link
Copy Markdown
Author

Both deviations you flagged are already in the branch — they landed in 57ad2d4, about six hours after your comment, so the head you read was the older one. No action needed from me; flagging it only so the version you land from is the current one.

Prefix/suffix, not exact match — this is what you described, and nothing is coupled to the hand-typed tag:

bool IsMonolithBinaryZipName(const FString& Name)
{
	return Name.StartsWith(TEXT("Monolith-"), ESearchCase::IgnoreCase)
		&& Name.EndsWith(TEXT(".zip"), ESearchCase::IgnoreCase);
}

bool MatchesEngineBinaryZipName(const FString& Name, const FString& EngineTag)
{
	return IsMonolithBinaryZipName(Name)
		&& Name.EndsWith(FString::Printf(TEXT("-%s.zip"), *EngineTag), ESearchCase::IgnoreCase);
}

The bPerEngineRelease mis-trigger is fixed — you were right that :229-232 was untouched, but that whole loop moved into the selector, and the prefix gate now runs before the flag:

if (!IsMonolithBinaryZipName(Name))
{
	continue;                      // unrelated asset leaves the loop here
}
if (Name.Contains(TEXT("-UE5."), ESearchCase::IgnoreCase))
{
	bPerEngineRelease = true;      // only reachable for Monolith-*.zip
}

So SomeOtherPlugin-UE5.8.zip can no longer flip a generic Monolith release into per-engine mode. Covered by Monolith.Updater.ReleaseSelection.EngineTaggedForeignAssetDoesNotSwitchMode.

One ordering detail that looks like a bug on a quick read but is deliberate: bPerEngineRelease is set before the browser_download_url check, so a Monolith-*-UE5.8.zip entry with no URL still forces per-engine mode and fails closed. That is the partial-upload window you described — a half-published release should abort rather than fall back to a generic zip.

The selector is a pure function over the release JSON with no HTTP or install side effects, so MonolithUpdateReleaseSelectorTest.cpp should port across unchanged if you want the coverage.

@kunkunGames kunkunGames changed the title fix(updater): accept only Monolith binary release assets 🛡️ Codex: accept only Monolith binary release assets Jul 27, 2026
@kunkunGames kunkunGames changed the title 🛡️ Codex: accept only Monolith binary release assets fix(updater): accept only Monolith binary release assets Jul 27, 2026
@tumourlove

Copy link
Copy Markdown
Owner

Status update: not in v0.22.0, and deliberately so. Still queued, and I want to be straight about why it keeps slipping rather than leaving you guessing.

The change itself is good. I reviewed it properly this time — the selector is correct, it is well scoped, it has no regressions against master, and removing the zipball_url fallback closes a path that can never produce a working install (a GitHub source archive has no Binaries/, so the swap leaves a structurally broken plugin directory).

Two things are holding it.

It requires an end-to-end rehearsal, and that is a hard rule here. Any change to the auto-update path has to be driven through a real Install click — download, hash verification, staged swap — against a real release, including from an old client built from a prior tree. That rule exists because v0.14.7 shipped an install path that could never once succeed on Windows and it took fifteen release cycles to notice: FPlatformMisc::GetSHA256Signature has no Windows implementation and its generic fallback is a fatal checkf, so every defensive guard around it was unreachable. "It compiles and the dialog appears" is not that rehearsal. The rehearsal is also awkward to stage, because the update endpoint is a compile-time constant and a draft release is invisible to /releases/latest, so it needs a throwaway repo.

And its urgency is genuinely low, which is the honest reason it gets cut when time runs short. The failure it targets — a client polling during the publish-then-upload window and installing a source zipball — is already closed server-side for every deployed client by publishing as a draft and flipping only after the assets are up and verified. That mitigation reaches clients no code change can, including all the ones already installed. Your PR protects clients installed at the version that ships it, against a window the draft flow has already shut. That makes it defence-in-depth against process error — someone publishing by hand, or a future maintainer dropping the gate — which is worth having, but not worth waiving a rehearsal for.

So the rule I wrote down before this release started, and followed: budget the rehearsal and ship it; the moment that budget is at risk, cut this rather than shorten the gate. The budget went, so it was cut.

Next release, with the rehearsal. Two things from your PR did land in v0.22.0 in the meantime: the release-body verification gate now catches pre-v2 SHA markers in any position rather than only at line start — it was line-anchored while the deployed parser it defends is unanchored, so an ordinary markdown bullet would have passed the gate and crashed old clients — and the offline-CLI build gate and freshness guard now actually run.

mindmedicine added a commit to mindmedicine/monolith that referenced this pull request Aug 20, 2026
Fixes tumourlove#83 and tumourlove#99 -- the same conceptual error twice, in two readers that
answered confidently about the wrong thing.

BOTH WERE REPRODUCED IN A LIVE EDITOR BEFORE ANY CODE WAS WRITTEN. That ordering
was deliberate: earlier the same day three gaps (tumourlove#101, tumourlove#104, tumourlove#87) turned out to
have real symptoms and WRONG STATED CAUSES, and two fixes had been funded on
descriptions nobody had reproduced.

tumourlove#83 -- get_module_script_inputs looped Cast<UNiagaraNodeInput> filtered to
Usage == Parameter, which on a stock Epic module matches only the ParameterMap
plumbing node. Epic exposes module inputs as ParameterMapGet Module.* PINS, not
Input nodes. Measured before: GravityForce returned input_count 2 -- the InputMap
and the Coordinate Space switch -- omitting Module.Gravity entirely, and carrying
NO warnings key at all. After: Module.Gravity by name, InputMap gone, the switch
keeping its full valid_options, count still 2. The NAMES were always the signal;
the count never was.

tumourlove#99 -- get_module_output_parameters was NOT A BUG, it was AN EMPTY BRANCH. The
MapSet path was literally `if (MapOut) { }` plus three comments ending "But the
formal output node approach above covers the standard case." It did not. Every
call returned output_count 1, outputs [OutputMap] -- nothing, dressed as an
answer. After: stock Collision returns 95 outputs, 50 addressable, OutputMap
absent, split by kind, with Output.Module.X resolved to the caller-addressable
Output.Collision.X.

THE FIX ALREADY EXISTED ONE FUNCTION AWAY. Gap tumourlove#31-F fixed this exact bug in
HandleGetDynamicInputInputs, and its own note points readers at
get_module_script_inputs -- which still had it. A documented fix, never applied
to the sibling. Both now share one traversal via a new
MonolithNiagaraParameterNames.h holding the pure name rules, with
HandleGetDynamicInputInputs and BuildStackWriterIndex refactored onto the same
rules rather than a third variant.

Unresolvable entries now WARN instead of vanishing. Verified on the output side:
delete a placed module's script and the reader returns "CANNOT REPORT: ... outputs
are UNKNOWN -- 'outputs' being empty does NOT mean the module writes nothing."
The input-side unresolved path was code-reviewed but could not be constructed;
it is NOT recorded as verified.

Also removed both surviving statements of the refuted I-37 -- there were TWO
sites, rename_script_parameter's schema description AND a runtime warning. That
claim shipped inside the tool's own help text, so a caller reading the action
rather than the docs still got the wrong rule.

KEPT DELIBERATELY, flagged by the author as outside his brief and ruled on: the
IsA-based detection makes BuildStackWriterIndex pick up UNiagaraNodeParameterMapFor
writes it previously missed, affecting four actions. A writer index that misses
writers is the same defect class being fixed here, so it stays. Measured
afterwards: audit_stack_wiring, trace_parameter_binding, get_available_parameters
and list_stack_writers all sane and mutually consistent -- BUT the new branch
never fired, because no ParameterMapFor content exists in reach and add_graph_node
cannot author one. Nothing broke; the widening's EFFECT is undetermined.

Tests: 2 new, Monolith.Niagara.ParameterNames, and they were RUN -- 2/2 pass,
alongside Monolith.RequiredAssetPath 7/7. They live in MonolithNiagara rather
than MonolithCore because MonolithCore cannot link them, and they cover the NAME
RULES ONLY, not the graph traversal that actually failed. The live validation is
the real coverage here and the note says so.

Logged, not fixed: Cast<UNiagaraNodeParameterMapGet/Set> does not compile under
MONOLITH_RELEASE_BUILD=1 (private header, no export macro) and
CollectStackParameterReaders already uses it unguarded -- a pre-existing latent
break in a release path we do not build.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants