fix(desktop): report a tolerated platform only once the feed is publi…

…shed Round-2 review follow-ups on the linux-aarch64 release leg: - create-desktop-update-manifest.mjs: emit the ::warning::no updater artifact annotation only after the feed has been written. It fired at selection time, so a run that later threw on a different leg published an annotation claiming an incomplete feed went out while no feed existed at all. - create-desktop-update-manifest.mjs: scope repeat-accumulation to --allow-missing-platform, the only genuinely multi-valued option. Accumulating for every option comma-joined a duplicated single-valued flag straight into the signed feed (version 0.1.0,0.1.0, which no updater client parses as semver) or wrote a file named f,f so no feed existed, both at exit status 0. Single-valued options keep the ordinary last-wins override. - test-release.js: pin tolerated-and-present, the normal case on the only production caller (the OSS re-mirror path), so dropping the matches.length === 0 conjunct cannot silently omit linux-aarch64 from the mirror feed; plus the refused-after-tolerating and duplicated-flag cases. - desktop-isolation.test.js: read the changed-files filter alternatives out of the anchored grep -Eq alternation instead of substring-matching the whole run script, and pin that the lane the filter gates still runs test-release.js. Co-authored-by: Qwen-Coder <[email protected]> Patrol-Run: qwen-pr-closeout/jmujyzhfka1 Co-authored-by: Qwen-Coder <[email protected]>
yiliang114 · Sep 28, 2026 · Sep 27, 2026 · Sep 27, 2026 · Sep 27, 2026 · Sep 27, 2026
commit 217ceea5f692de11e281ecff5c9c02eb8d13f14c
diff --git a/.github/scripts/create-desktop-update-manifest.mjs b/.github/scripts/create-desktop-update-manifest.mjs
@@ -15,6 +15,10 @@ const allowMissingPlatforms = new Set(
     .map((platform) => platform.trim())
     .filter(Boolean),
 );
+// Tolerated platforms are collected here and reported only once the feed has
+// actually been written, so a run that throws on some other leg publishes no
+// annotation claiming an incomplete feed went out.
+const droppedPlatforms = [];
 const platforms = {};
 const platformArtifacts = [
   [
@@ -67,17 +71,26 @@ const manifest = {
 };
 fs.writeFileSync(options.output, `${JSON.stringify(manifest, null, 2)}\n`);
 
+// Only now is "publishing the feed without it" true: every leg that was not
+// tolerated has been selected and signed, and the feed is on disk. Emitting
+// this inside selectArtifact instead would put the annotation on runs that
+// later throw and write nothing, sending oncall hunting for a truncated feed
+// that never existed.
+for (const platform of droppedPlatforms) {
+  // stdout, not stderr: GitHub parses workflow commands from stdout only,
+  // and stderr has to stay reserved for the thrown error the tests match on.
+  // Without this a tolerant run is byte-identical in the log to a complete
+  // one, and the dropped key is only discoverable by diffing the published
+  // feed against the previous mirror.
+  console.log(
+    `::warning::no updater artifact for ${platform}; publishing the feed without it (--allow-missing-platform)`,
+  );
+}
+
 function selectArtifact(assets, pattern, platform) {
   const matches = assets.filter((asset) => pattern.test(asset));
   if (matches.length === 0 && allowMissingPlatforms.has(platform)) {
-    // stdout, not stderr: GitHub parses workflow commands from stdout only,
-    // and stderr has to stay reserved for the thrown error the tests match on.
-    // Without this a tolerant run is byte-identical in the log to a complete
-    // one, and the dropped key is only discoverable by diffing the published
-    // feed against the previous mirror.
-    console.log(
-      `::warning::no updater artifact for ${platform}; publishing the feed without it (--allow-missing-platform)`,
-    );
+    droppedPlatforms.push(platform);
     return null;
   }
   if (matches.length !== 1) {
@@ -95,16 +108,25 @@ function releaseBaseUrl(options) {
 }
 
 function parseArguments(args) {
+  // Only this option is genuinely multi-valued. Accumulating repeats for every
+  // option would comma-join a duplicated single-valued flag straight into the
+  // published, signed feed -- `--version a --version a` writes a version no
+  // updater client can parse, and `--output f --output f` writes a file named
+  // `f,f` so no feed exists at all, both at exit status 0.
+  const multiValueOptions = new Set(['allow-missing-platform']);
   const values = {};
   for (let index = 0; index < args.length; index += 2) {
     const name = args[index]?.replace(/^--/, '');
     const value = args[index + 1];
     if (!name || value === undefined) throw new Error('Invalid arguments.');
-    // Accumulate repeats: a bash-array caller spells a multi-valued option as
-    // repeated flags, and plain assignment would keep only the last one — the
-    // dropped value then surfaces as a missing build leg during a release run.
+    // Accumulate repeats of the multi-valued option only: a bash-array caller
+    // spells it as repeated flags, and plain assignment would keep only the
+    // last one — the dropped value then surfaces as a missing build leg during
+    // a release run. Single-valued options keep the ordinary last-wins.
     values[name] =
-      values[name] === undefined ? value : `${values[name]},${value}`;
+      multiValueOptions.has(name) && values[name] !== undefined
+        ? `${values[name]},${value}`
+        : value;
   }
   for (const required of ['assets', 'repository', 'tag', 'version', 'output']) {
     if (!values[required]) throw new Error(`Missing --${required}`);

diff --git a/packages/desktop/scripts/test-release.js b/packages/desktop/scripts/test-release.js
@@ -1451,6 +1451,56 @@ function testUpdateManifest(directory) {
       ],
       { encoding: 'utf8' },
     );
+
+  // Tolerated-and-present, which is the normal case on the only production
+  // caller: sync-desktop-to-oss.yml passes --allow-missing-platform
+  // linux-aarch64 on every SOURCE=release re-mirror, and from this release
+  // onward that artifact is in the downloaded assets. Without this run the
+  // `matches.length === 0 &&` conjunct is unpinned -- dropping it turns the
+  // escape hatch into a blanket opt-out that omits the arm64 leg from the
+  // mirror feed (the first updater endpoint) while the asset sits in the same
+  // bucket, exits 0, and prints a warning that is false. That is #12806 again.
+  const toleratedButPresent = runManifest(
+    '--allow-missing-platform',
+    'linux-aarch64',
+  );
+  assert.equal(toleratedButPresent.status, 0, toleratedButPresent.stderr);
+  assert.deepEqual(
+    Object.keys(JSON.parse(fs.readFileSync(output, 'utf8')).platforms).sort(),
+    [
+      'darwin-aarch64',
+      'darwin-x86_64',
+      'linux-aarch64',
+      'linux-x86_64',
+      'windows-x86_64',
+    ],
+    'a tolerated platform whose artifact did upload must still be published',
+  );
+  assert.doesNotMatch(
+    toleratedButPresent.stdout,
+    /::warning::/,
+    'a tolerated platform that did upload must not be reported missing',
+  );
+
+  // Accumulation belongs to --allow-missing-platform alone. A duplicated
+  // single-valued flag has to override: comma-joining it would put
+  // `"version": "0.1.0,0.1.0"` in the signed feed, which the updater client
+  // cannot parse as semver, and for --output it would write a file named
+  // `<f>,<f>` so no feed exists at all -- both with the step exiting 0.
+  const duplicatedVersion = runManifest('--version', '0.1.0');
+  assert.equal(duplicatedVersion.status, 0, duplicatedVersion.stderr);
+  assert.equal(
+    JSON.parse(fs.readFileSync(output, 'utf8')).version,
+    '0.1.0',
+    'a repeated --version must override, not comma-join into the feed',
+  );
+  const duplicatedOutput = runManifest('--output', output);
+  assert.equal(
+    duplicatedOutput.status,
+    0,
+    `a repeated --output must override, not write a comma-joined filename: ${duplicatedOutput.stderr}`,
+  );
+
   fs.rmSync(path.join(assets, artifacts[4]));
   fs.rmSync(path.join(assets, `${artifacts[4]}.sig`));
   const missingLeg = runManifest();
@@ -1504,6 +1554,28 @@ function testUpdateManifest(directory) {
       ['darwin-x86_64', 'linux-x86_64', 'windows-x86_64'],
     );
   }
+
+  // darwin-aarch64 is tolerated but linux-aarch64 is not, so this run throws
+  // and writes no feed. It must not also publish an annotation claiming an
+  // incomplete feed went out: GitHub parses workflow commands from stdout, so
+  // a warning emitted at selection time turns a red run into a claim that
+  // sends oncall looking for a truncated desktop-latest.json that never
+  // existed, while the real cause is the other leg named in stderr.
+  const refusedAfterTolerating = runManifest(
+    '--allow-missing-platform',
+    'darwin-aarch64',
+  );
+  assert.notEqual(refusedAfterTolerating.status, 0);
+  assert.match(
+    refusedAfterTolerating.stderr,
+    /Expected one updater artifact for linux-aarch64, found 0/,
+  );
+  assert.doesNotMatch(
+    refusedAfterTolerating.stdout,
+    /::warning::/,
+    'a run that refused to publish must not also report a tolerated platform',
+  );
+
   for (const artifact of [artifacts[0], artifacts[4]]) {
     fs.writeFileSync(path.join(assets, artifact), artifact);
     fs.writeFileSync(

diff --git a/scripts/tests/desktop-isolation.test.js b/scripts/tests/desktop-isolation.test.js
@@ -27,7 +27,8 @@
 //    `packages/desktop/scripts/test-release.js` is the only test either of them
 //    has and it runs in this job, so a filter naming one script and not the
 //    other lets a PR that edits only the unlisted one skip its own test and
-//    still report green.
+//    still report green. The filter is only worth pinning together with the
+//    lane it gates, so the same test asserts that lane still runs the script.
 //
 // The npm/pnpm mirror of the negation list is already pinned by
 // `package-scripts.test.js` ("mirrors the npm workspace boundaries in
@@ -218,14 +219,44 @@ describe('desktop_shell CI job — the crate path agrees with itself', () => {
   });
 
   it('lists both updater-feed scripts, so either one triggers the lane that tests them', () => {
+    // Read the alternatives out of the `grep -Eq '^(...)'` call instead of
+    // searching the whole run script. A substring match cannot tell an
+    // alternative of the anchored ERE from the same text in one of that step's
+    // comment lines, and it does not pin the `-E` mode the pattern's `\.`, `|`
+    // and `^(…)` depend on: under `grep -Fq` they are literals, the group
+    // never matches, every PR gets changed=false, and the job reports success
+    // having compiled nothing.
+    const pattern = /grep -Eq '\^\((.*)\)'/u.exec(filterRun)?.[1];
+    expect(
+      pattern,
+      "the changed-files filter is no longer an anchored `grep -Eq '^(...)'` alternation, so its `\\.` escapes and `|` separators are literals and no changed file ever matches",
+    ).toBeDefined();
+    const alternatives = pattern.split('|');
     for (const script of [
       'create-desktop-update-manifest',
       'create-electron-bridge-manifest',
     ]) {
       expect(
-        filterRun,
-        `the changed-files filter does not list ${script}.mjs, so a PR touching only that script skips 'Run desktop release tests' and reports green having tested nothing`,
+        alternatives,
+        `the changed-files filter does not list ${script}.mjs as an alternative, so a PR touching only that script skips 'Run desktop release tests' and reports green having tested nothing`,
       ).toContain(`\\.github/scripts/${script}\\.mjs`);
     }
+
+    // The filter is only the first link: the mechanism that closes #12806 is
+    // filter matches -> desktop_shell runs -> `Run desktop release tests`
+    // executes test-release.js -> the matrix/feed parity assert fires. Pin the
+    // last two links as well, or the filter can gate a lane that tests nothing.
+    // `toContain` on the changed-files conjunct, not equality on the whole
+    // `if:` -- that gate also carries `runner.os == 'Linux'`, which the
+    // windows-2022 leg of the job matrix legitimately does not satisfy.
+    const lane = stepNamed('Run desktop release tests');
+    expect(
+      String(lane?.run ?? ''),
+      'desktop_shell no longer runs test-release.js, so the changed-files filter gates a lane that tests nothing',
+    ).toContain('node scripts/test-release.js');
+    expect(
+      String(lane?.if ?? ''),
+      'the test-release.js lane is no longer gated by the filter this test pins, so the two can drift apart silently',
+    ).toContain("steps.filter.outputs.changed == 'true'");
   });
 });