From 5ab74f399d6f9dad0c667c237ea740e93855695f Mon Sep 17 00:00:00 2001 From: doodlum <15017472+doodlum@users.noreply.github.com> Date: Sun, 27 Sep 2026 16:37:36 +0100 Subject: [PATCH 1/2] fix(collections): skip the member a skip names, not one sharing its file name markCollectionMemberSkipped matched a skipped reference against the session members one by one, accepting a tag, file hash or logical file name on the first member it met. At install start every untouched optional member is defaulted to skipped through it, so an optional that shared a file hash or a logical file name ("Main File") with a required member listed before it marked the required member ignored instead. The flag is durable: the required member was never downloaded on this or any later resume, the collection reported complete, and the optional the user never chose was installed. Try each identity marker across every member before the next, weaker one, so the tagged member wins and the hash and file name stay fallbacks. Co-Authored-By: Claude Opus 5.5 --- .../collections/util/InstallDriver.test.ts | 31 ++++++++++ src/renderer/src/util/collectionSkip.test.ts | 48 +++++++++++++++ src/renderer/src/util/collectionSkip.ts | 59 +++++++++++-------- 3 files changed, 115 insertions(+), 23 deletions(-) diff --git a/src/renderer/src/extensions/collections/util/InstallDriver.test.ts b/src/renderer/src/extensions/collections/util/InstallDriver.test.ts index ffb54802ad..f27b71991b 100644 --- a/src/renderer/src/extensions/collections/util/InstallDriver.test.ts +++ b/src/renderer/src/extensions/collections/util/InstallDriver.test.ts @@ -212,6 +212,37 @@ describe("InstallDriver optional default-skip", () => { expect(rules.find((r) => r.reference.tag === "req-a")?.ignored).toBeUndefined(); }); + // Nexus file names repeat across unrelated mods ("Main File"), so a required member listed before + // an optional can share its logical file name. Defaulting the optional must skip the optional, + // not the required member, which would then never be installed, with the collection reported + // complete. + test("defaults the optional, not a required member with the same file name", async ({ + makeDriver, + }) => { + const sharedReq = makeRule({ + type: "requires", + reference: makeReference({ tag: "req-main", logicalFileName: "Main File" }), + }); + const sharedOpt = makeRule({ + type: "recommends", + reference: makeReference({ tag: "opt-main", logicalFileName: "Main File" }), + }); + const fixture = buildCollectionFixture("col-shared", [sharedReq, sharedOpt]); + const h = makeDriver({ + mods: { [GAME_ID]: { [fixture.collection.id]: fixture.collection } }, + downloads: { [fixture.download.id]: fixture.download }, + profiles: { [profile.id]: profile }, + }); + await startWith(h, fixture); + + const session = h.getState().session.collections.activeSession; + expect(session?.mods[modRuleId(sharedOpt)].status).toBe("ignored"); + expect(session?.mods[modRuleId(sharedReq)].status).not.toBe("ignored"); + const rules = h.getState().persistent.mods[GAME_ID]["col-shared"].rules ?? []; + expect(rules.find((r) => r.reference.tag === "opt-main")?.ignored).toBe(true); + expect(rules.find((r) => r.reference.tag === "req-main")?.ignored).toBeUndefined(); + }); + test("does not re-default an optional the user already selected (ignored:false)", async ({ makeDriver, }) => { diff --git a/src/renderer/src/util/collectionSkip.test.ts b/src/renderer/src/util/collectionSkip.test.ts index e34557b051..93c4038905 100644 --- a/src/renderer/src/util/collectionSkip.test.ts +++ b/src/renderer/src/util/collectionSkip.test.ts @@ -141,6 +141,54 @@ describe("markCollectionMemberSkipped - automatic skip (mod reference)", () => { expect(flagged).toEqual(["tag-x"]); }); + // the member the tag names wins over one met earlier that only shares a weaker marker, in the + // session scan and in the live-rule scan alike + test.for([ + ["file hash", { fileMD5: "shared-md5" }], + ["logical file name", { logicalFileName: "Main File" }], + ] as const)( + "ignores the tagged member, not an earlier one with the same %s", + ([, shared], { makeApi }) => { + const earlier = makeRule({ + type: "requires", + reference: makeReference({ tag: "tag-earlier", ...shared }), + }); + const skipped = makeRule({ + type: "recommends", + reference: makeReference({ tag: "tag-skipped", ...shared }), + }); + const h = makeApi({ + mods: { + [GAME_ID]: { + [COLLECTION_ID]: makeMod({ id: COLLECTION_ID, rules: [earlier, skipped] }), + }, + }, + session: makeInstallState({ + activeSession: makeSession({ + sessionId: SESSION_ID, + collectionId: COLLECTION_ID, + gameId: GAME_ID, + mods: { + [modRuleId(earlier)]: makeModInstallInfo({ rule: earlier, status: "pending" }), + [modRuleId(skipped)]: makeModInstallInfo({ rule: skipped, status: "pending" }), + }, + }), + }), + }); + + const matched = markCollectionMemberSkipped(h.api, { reference: skipped.reference }); + + expect(matched).toBe(true); + expect(statusOf(h, skipped)).toBe("ignored"); + expect(statusOf(h, earlier)).toBe("pending"); + const rules = h.getState().persistent.mods[GAME_ID][COLLECTION_ID].rules ?? []; + const flagged = rules + .filter((rule) => rule.ignored === true) + .map((rule) => rule.reference.tag); + expect(flagged).toEqual(["tag-skipped"]); + }, + ); + // a session can track a member the collection's current rules no longer carry (the rules were // replaced mid-install); the skip settles the session entry without re-adding the old rule test("settles a member whose rule left the collection without re-adding it", ({ makeApi }) => { diff --git a/src/renderer/src/util/collectionSkip.ts b/src/renderer/src/util/collectionSkip.ts index 711b9a3fd9..a1c28c4f7e 100644 --- a/src/renderer/src/util/collectionSkip.ts +++ b/src/renderer/src/util/collectionSkip.ts @@ -61,28 +61,43 @@ function fuzzyChainMatch(identifiers: ISkippedDownloadIdentifiers, ref: IModRefe } /** - * Match the skipped dependency's reference against a collection rule. This mirrors the previous - * `collection-mod-skipped` handler exactly (tag is most reliable, then file hash, then logical - * file name) so the automatic/premium skip path is behaviourally unchanged. + * The identity markers a skipped dependency's reference is matched on, most reliable first: the + * tag names one member, a file hash can be shared by two members installing the same archive, and + * a logical file name ("Main File") by any number of unrelated ones. */ -function matchesReference(reference: IModReference, ruleRef: IModReference): boolean { - if (reference.tag && ruleRef.tag === reference.tag) { - return true; - } - if (reference.fileMD5 && ruleRef.fileMD5 === reference.fileMD5) { - return true; - } - if (reference.logicalFileName && ruleRef.logicalFileName === reference.logicalFileName) { - return true; - } - return false; -} +const REFERENCE_MARKERS = ["tag", "fileMD5", "logicalFileName"] as const; -function matchesSkip(skip: ICollectionSkip, ref: IModReference): boolean { - if ("reference" in skip) { - return matchesReference(skip.reference, ref); +/** + * Find the item a skip names. A reference skip tries each marker against every item before the + * next, weaker one: accepting any marker on the first item met would ignore whichever member + * comes first with the skipped member's file hash or logical file name, instead of the member + * the tag names. + */ +function findSkipped( + skip: ICollectionSkip, + items: T[], + referenceOf: (item: T) => IModReference | undefined, +): T | undefined { + if (!("reference" in skip)) { + return items.find((item) => { + const ref = referenceOf(item); + return ( + ref != null && + (testRefByIdentifiers(skip.identifiers, ref) || fuzzyChainMatch(skip.identifiers, ref)) + ); + }); + } + for (const marker of REFERENCE_MARKERS) { + const value = skip.reference[marker]; + if (!value) { + continue; + } + const found = items.find((item) => referenceOf(item)?.[marker] === value); + if (found !== undefined) { + return found; + } } - return testRefByIdentifiers(skip.identifiers, ref) || fuzzyChainMatch(skip.identifiers, ref); + return undefined; } /** @@ -117,9 +132,7 @@ export function markCollectionMemberSkipped(api: IExtensionApi, skip: ICollectio // The session is keyed by each member's rule as it was when the install started, so the entry // is matched on its own snapshot rather than on the live rule. const [sessionRuleId] = - Object.entries(session.mods).find(([, info]) => - info.rule?.reference != null ? matchesSkip(skip, info.rule.reference) : false, - ) ?? []; + findSkipped(skip, Object.entries(session.mods), ([, info]) => info.rule?.reference) ?? []; // The fallback scan relies on members having distinct identities; if two share the matched // identifier (e.g. the same logicalFileName, or two fuzzy rules on one modId), the wrong member @@ -127,7 +140,7 @@ export function markCollectionMemberSkipped(api: IExtensionApi, skip: ICollectio const rule = (sessionRuleId !== undefined ? rules.find((iter) => modRuleId(iter) === sessionRuleId) - : undefined) ?? rules.find((iter) => matchesSkip(skip, iter.reference)); + : undefined) ?? findSkipped(skip, rules, (iter) => iter.reference); // Only a rule the session tracks can be settled. A live rule it never saw (the collection // gained it after the install started) still carries the decision durably. const liveRuleId = rule !== undefined ? modRuleId(rule) : undefined; From 1c0bc4a9748597b8bf5165a399138c9ec1567b83 Mon Sep 17 00:00:00 2001 From: doodlum <15017472+doodlum@users.noreply.github.com> Date: Sun, 27 Sep 2026 18:45:07 +0100 Subject: [PATCH 2/2] fix(collections): name a skipped member by its rule, not its markers A deterministic collection tags a fuzzy member by its mod page and install spec, so a required and an optional file from one page share a tag, and the tag-first match from the previous commit still ignored the required member when the optional was defaulted to skipped. The skip sites that have the member's rule now pass its session key (modRuleId) and markCollectionMemberSkipped uses it directly: the optional default in InstallDriver.start, handleDownloadSkipped and the instructions Skip recovery in InstallManager. The marker scan remains for a skip with no rule id, and a skip that carries a tag no longer falls back to the logical file name. A member's own sub-dependency is not a collection member, so its skip no longer ignores whichever member shares its markers. makeDeterministicRef builds references tagged the way deterministic collections tag them, so identity tests meet the real collision. Co-Authored-By: Claude Opus 5.5 --- .../collections/util/InstallDriver.test.ts | 43 ++++++++++++ .../collections/util/InstallDriver.ts | 6 +- .../mod_management/InstallManager.ts | 33 +++++++-- src/renderer/src/test-utils/builders.ts | 9 +++ src/renderer/src/util/collectionSkip.test.ts | 70 +++++++++++++++++++ src/renderer/src/util/collectionSkip.ts | 39 +++++++---- 6 files changed, 180 insertions(+), 20 deletions(-) diff --git a/src/renderer/src/extensions/collections/util/InstallDriver.test.ts b/src/renderer/src/extensions/collections/util/InstallDriver.test.ts index f27b71991b..518c0481ae 100644 --- a/src/renderer/src/extensions/collections/util/InstallDriver.test.ts +++ b/src/renderer/src/extensions/collections/util/InstallDriver.test.ts @@ -16,6 +16,7 @@ import { describe, expect, vi } from "vitest"; import { makeCollectionModInfo, + makeDeterministicRef, makeDownload, makeFileListItem, makeInstallerChoices, @@ -243,6 +244,48 @@ describe("InstallDriver optional default-skip", () => { expect(rules.find((r) => r.reference.tag === "req-main")?.ignored).toBeUndefined(); }); + // A deterministic collection tags a fuzzy member by its mod page and install spec, so a required + // and an optional file from one page share a tag. Defaulting the optional must still skip it, + // not the required member. + test("defaults the optional, not a required member sharing its deterministic tag", async ({ + makeDriver, + }) => { + const page = { repository: "nexus", gameId: GAME_ID, modId: "1234", fileId: "1" }; + const sharedReq = makeRule({ + type: "requires", + reference: makeDeterministicRef({ + repo: page, + versionMatch: ">=1.0.0+prefer", + logicalFileName: "Main File", + }), + }); + const sharedOpt = makeRule({ + type: "recommends", + reference: makeDeterministicRef({ + repo: { ...page, fileId: "2" }, + versionMatch: ">=1.0.0+prefer", + logicalFileName: "Optional patch", + }), + }); + // the fixture models the real rule: both members carry the same tag + expect(sharedReq.reference.tag).toBeDefined(); + expect(sharedOpt.reference.tag).toBe(sharedReq.reference.tag); + const fixture = buildCollectionFixture("col-det", [sharedReq, sharedOpt]); + const h = makeDriver({ + mods: { [GAME_ID]: { [fixture.collection.id]: fixture.collection } }, + downloads: { [fixture.download.id]: fixture.download }, + profiles: { [profile.id]: profile }, + }); + await startWith(h, fixture); + + const session = h.getState().session.collections.activeSession; + expect(session?.mods[modRuleId(sharedOpt)].status).toBe("ignored"); + expect(session?.mods[modRuleId(sharedReq)].status).not.toBe("ignored"); + const rules = h.getState().persistent.mods[GAME_ID]["col-det"].rules ?? []; + expect(rules.find((r) => r.type === "recommends")?.ignored).toBe(true); + expect(rules.find((r) => r.type === "requires")?.ignored).toBeUndefined(); + }); + test("does not re-default an optional the user already selected (ignored:false)", async ({ makeDriver, }) => { diff --git a/src/renderer/src/extensions/collections/util/InstallDriver.ts b/src/renderer/src/extensions/collections/util/InstallDriver.ts index b4e2e2a333..2a3ac848b9 100644 --- a/src/renderer/src/extensions/collections/util/InstallDriver.ts +++ b/src/renderer/src/extensions/collections/util/InstallDriver.ts @@ -12,6 +12,7 @@ import type { IState } from "../../../types/IState"; import { generateCollectionSessionId, isTerminalMemberStatus, + modRuleId, } from "../../../util/collectionInstallSession"; import { getCollectionActiveSession, @@ -997,7 +998,10 @@ class InstallDriver { // rule flag + session status) now that the session exists. for (const rule of optional) { if (rule.ignored === undefined) { - markCollectionMemberSkipped(this.mApi, { reference: rule.reference }); + markCollectionMemberSkipped(this.mApi, { + reference: rule.reference, + ruleId: modRuleId(rule), + }); } } diff --git a/src/renderer/src/extensions/mod_management/InstallManager.ts b/src/renderer/src/extensions/mod_management/InstallManager.ts index 1abd635dcf..4318532334 100644 --- a/src/renderer/src/extensions/mod_management/InstallManager.ts +++ b/src/renderer/src/extensions/mod_management/InstallManager.ts @@ -857,6 +857,22 @@ class InstallManager { this.maybeAdvancePhase(collectionId, api); } + /** + * Whether a dependency is a member of the collection being installed: it carries the member's + * session key, or it belongs to the collection itself rather than to one of its members. A + * member's own dependencies are installed with the member as their source and are not members. + */ + private isCollectionMemberDependency( + api: IExtensionApi, + sourceModId: string, + dep: IDependency, + ): boolean { + return ( + dep.sessionRuleId !== undefined || + getCollectionActiveSession(api.getState())?.collectionId === sourceModId + ); + } + private handleDownloadSkipped(api: IExtensionApi, sourceModId: string, dep: IDependency) { if (!sourceModId || !dep) { return; @@ -882,8 +898,11 @@ class InstallManager { } // Mark the skipped member ignored directly against the active session (collections is core - // now, so no event round-trip through the InstallDriver is needed). - markCollectionMemberSkipped(api, { reference: dep.reference }); + // now, so no event round-trip through the InstallDriver is needed). A sub-dependency of a + // member is not a member itself, so its skip ignores nothing. + if (this.isCollectionMemberDependency(api, sourceModId, dep)) { + markCollectionMemberSkipped(api, { reference: dep.reference, ruleId: dep.sessionRuleId }); + } // See if we can advance the phase this.maybeAdvancePhase(sourceModId, api); @@ -2720,8 +2739,14 @@ class InstallManager { } else { this.mDependencyRetryCount.delete(installKey); if (recovery.action === "skip") { - // same settle the free-user skip performs: durable ignore + session "ignored" - markCollectionMemberSkipped(api, { reference: dep.reference }); + // same settle the free-user skip performs: durable ignore + session "ignored". + // A sub-dependency of a member is not a member itself, so its skip ignores nothing. + if (this.isCollectionMemberDependency(api, sourceModId, dep)) { + markCollectionMemberSkipped(api, { + reference: dep.reference, + ruleId: dep.sessionRuleId, + }); + } } else if (recovery.action === "fail") { // Retries exhausted: settle the member as failed (terminal) so the collection can // still complete and the member is not re-prompted. writeCollectionSession no-ops diff --git a/src/renderer/src/test-utils/builders.ts b/src/renderer/src/test-utils/builders.ts index feac92ed87..b3325eb7e8 100644 --- a/src/renderer/src/test-utils/builders.ts +++ b/src/renderer/src/test-utils/builders.ts @@ -36,6 +36,7 @@ import type { ICollectionMod, ICollectionModRule, } from "../extensions/collections/types/ICollection"; +import { deterministicReferenceTag } from "../extensions/collections/util/deterministicReferenceTag"; import type InstallDriver from "../extensions/collections/util/InstallDriver"; import { stateReducer as downloadStateReducer } from "../extensions/download_management/reducers/state"; import { downloadPathForGame } from "../extensions/download_management/selectors"; @@ -135,6 +136,14 @@ export function makeFuzzyRef(overrides: Partial = {}): IModRefere return makeExactRef({ versionMatch: "*", ...overrides }); } +// A reference tagged the way a deterministic collection tags its members (transformCollection): +// the tag is derived from the reference, so two fuzzy files from one mod page share it, as they do +// in real collections. Tests of member identity must use this rather than a hand-picked tag. +export function makeDeterministicRef(overrides: Partial = {}): IModReference { + const reference = makeExactRef({ tag: undefined, ...overrides }); + return { ...reference, tag: deterministicReferenceTag(reference) }; +} + export function makeRule(overrides: Partial = {}): IModRule { return { type: "requires", diff --git a/src/renderer/src/util/collectionSkip.test.ts b/src/renderer/src/util/collectionSkip.test.ts index 93c4038905..541c0f7c8b 100644 --- a/src/renderer/src/util/collectionSkip.test.ts +++ b/src/renderer/src/util/collectionSkip.test.ts @@ -10,6 +10,7 @@ import { describe, expect } from "vitest"; import type { IModRule } from "../extensions/mod_management/types/IMod"; import { + makeDeterministicRef, makeInstallState, makeMod, makeModInstallInfo, @@ -189,6 +190,75 @@ describe("markCollectionMemberSkipped - automatic skip (mod reference)", () => { }, ); + // two fuzzy files from one mod page share a deterministic tag; the skip site's rule id tells + // them apart where no marker can + test("ignores the member its rule id names when another shares its deterministic tag", ({ + makeApi, + }) => { + const page = { repository: "nexus", gameId: GAME_ID, modId: "1234", fileId: "1" }; + const required = makeRule({ + type: "requires", + reference: makeDeterministicRef({ repo: page, versionMatch: ">=1.0.0+prefer" }), + }); + const optional = makeRule({ + type: "recommends", + reference: makeDeterministicRef({ + repo: { ...page, fileId: "2" }, + versionMatch: ">=1.0.0+prefer", + }), + }); + expect(optional.reference.tag).toBe(required.reference.tag); + const h = makeApi({ + mods: { + [GAME_ID]: { + [COLLECTION_ID]: makeMod({ id: COLLECTION_ID, rules: [required, optional] }), + }, + }, + session: makeInstallState({ + activeSession: makeSession({ + sessionId: SESSION_ID, + collectionId: COLLECTION_ID, + gameId: GAME_ID, + mods: { + [modRuleId(required)]: makeModInstallInfo({ rule: required, status: "pending" }), + [modRuleId(optional)]: makeModInstallInfo({ rule: optional, status: "pending" }), + }, + }), + }), + }); + + const matched = markCollectionMemberSkipped(h.api, { + reference: optional.reference, + ruleId: modRuleId(optional), + }); + + expect(matched).toBe(true); + expect(statusOf(h, optional)).toBe("ignored"); + expect(statusOf(h, required)).toBe("pending"); + const rules = h.getState().persistent.mods[GAME_ID][COLLECTION_ID].rules ?? []; + expect(rules.filter((rule) => rule.ignored === true).map((rule) => rule.type)).toEqual([ + "recommends", + ]); + }); + + // a tagged skip that names no member (a sub-dependency, say) must not land on a member that + // only shares its logical file name + test("does not fall back to the file name for a skip that carries a tag", ({ makeApi }) => { + const rule = makeRule({ + type: "requires", + reference: makeReference({ tag: "mod-a", logicalFileName: "Main File" }), + }); + const h = makeApi(ruleOverrides(rule)); + + const matched = markCollectionMemberSkipped(h.api, { + reference: makeReference({ tag: "not-a-member", logicalFileName: "Main File" }), + }); + + expect(matched).toBe(false); + expect(statusOf(h, rule)).toBe("pending"); + expect(durableIgnored(h)).toBeUndefined(); + }); + // a session can track a member the collection's current rules no longer carry (the rules were // replaced mid-install); the skip settles the session entry without re-adding the old rule test("settles a member whose rule left the collection without re-adding it", ({ makeApi }) => { diff --git a/src/renderer/src/util/collectionSkip.ts b/src/renderer/src/util/collectionSkip.ts index a1c28c4f7e..a35d99cea0 100644 --- a/src/renderer/src/util/collectionSkip.ts +++ b/src/renderer/src/util/collectionSkip.ts @@ -28,8 +28,9 @@ export type ISkippedDownloadIdentifiers = Omit fileName.toLowerCase().replace(/[^a-z]+/gi, ""); @@ -61,17 +62,19 @@ function fuzzyChainMatch(identifiers: ISkippedDownloadIdentifiers, ref: IModRefe } /** - * The identity markers a skipped dependency's reference is matched on, most reliable first: the - * tag names one member, a file hash can be shared by two members installing the same archive, and - * a logical file name ("Main File") by any number of unrelated ones. + * The identity markers a skip without a rule id is matched on, strongest first. None of them is + * unique: a deterministic tag is shared by fuzzy files from one mod page, a file hash by members + * installing the same archive, and a logical file name ("Main File") by unrelated files. So a skip + * carrying a tag never falls back to the file name, and a skip that can name its member's rule + * (`ruleId`) is not matched on markers at all. */ -const REFERENCE_MARKERS = ["tag", "fileMD5", "logicalFileName"] as const; +const markersFor = (reference: IModReference) => + reference.tag ? (["tag", "fileMD5"] as const) : (["fileMD5", "logicalFileName"] as const); /** - * Find the item a skip names. A reference skip tries each marker against every item before the - * next, weaker one: accepting any marker on the first item met would ignore whichever member - * comes first with the skipped member's file hash or logical file name, instead of the member - * the tag names. + * Find the item a skip names by its markers. Each marker is tried against every item before the + * next, weaker one, so an earlier member that only shares a weaker marker is never taken for the + * one the stronger marker names. */ function findSkipped( skip: ICollectionSkip, @@ -87,7 +90,7 @@ function findSkipped( ); }); } - for (const marker of REFERENCE_MARKERS) { + for (const marker of markersFor(skip.reference)) { const value = skip.reference[marker]; if (!value) { continue; @@ -131,12 +134,18 @@ export function markCollectionMemberSkipped(api: IExtensionApi, skip: ICollectio ); // The session is keyed by each member's rule as it was when the install started, so the entry // is matched on its own snapshot rather than on the live rule. + const namedRuleId = + "ruleId" in skip && skip.ruleId !== undefined && session.mods[skip.ruleId] !== undefined + ? skip.ruleId + : undefined; const [sessionRuleId] = - findSkipped(skip, Object.entries(session.mods), ([, info]) => info.rule?.reference) ?? []; + namedRuleId !== undefined + ? [namedRuleId] + : (findSkipped(skip, Object.entries(session.mods), ([, info]) => info.rule?.reference) ?? []); - // The fallback scan relies on members having distinct identities; if two share the matched - // identifier (e.g. the same logicalFileName, or two fuzzy rules on one modId), the wrong member - // could be ignored. + // The marker scan relies on members having distinct identities; if two share the matched + // marker (two fuzzy files from one mod page share a deterministic tag), the wrong member could + // be ignored. Skip sites that have the member's rule pass its ruleId to avoid the scan. const rule = (sessionRuleId !== undefined ? rules.find((iter) => modRuleId(iter) === sessionRuleId)