From 5fe4090280c61d4cb1a4b50b862a16a99b880a67 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Wed, 2 Sep 2026 15:09:43 -0300 Subject: [PATCH 1/2] chore(install-dynamic-plugins): document why three guards are unreachable (RHIDP-16761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit installer-oci.ts had three branches that no test could ever reach, and they read as a coverage gap to anyone looking at the report: :45 splitOciPackage if (!imagePart || !pluginPath) return null :140 isAlreadyInstalled if (pullPolicy !== PullPolicy.ALWAYS) return false :147 isAlreadyInstalled if (!parts) return false They are unreachable, verified rather than assumed. installOciPlugin is not part of the package's public API — index.ts exports only the CLI module — so its sole caller is installer.ts, downstream of merger.ts calling ociPluginKey, whose OCI_REGEX already rejects a leading or trailing `!`. Those forms are in oci-key.test.ts's invalidCases, which is why :45 and :147 cannot fire. :140 cannot fire because PullPolicy has exactly two members and IF_NOT_PRESENT already returned three lines above. Three reviewers on RHIDP-16222 proposed deleting them. Keeping them is the better trade: deleting :147 in particular downgrades a clean skip into a TypeError on parts.imagePart the moment the upstream validation is relaxed, and the guards cost nothing at runtime. So they stay, and each carries an istanbul ignore naming the invariant that makes it unreachable — which is the part that matters. If someone loosens OCI_REGEX or adds a third PullPolicy, the comment tells them the pragma is now wrong. The `/* istanbul ignore next -- reason */` form follows the one existing use in this repo, augment-backend's HttpEmbedder.ts. Measured, full suite with --no-cache: before 95.08% stmts / 90% branch, uncovered lines 45, 140, 147 after 100% stmts / 100% branch / 100% funcs / 100% lines No behaviour change: the guards still run, and the same 258 tests pass. tsc, prettier and lint clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../packages/install-dynamic-plugins/src/installer-oci.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts b/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts index 5ba49c2d4e7..76c52f320d3 100644 --- a/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts +++ b/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts @@ -42,6 +42,9 @@ function splitOciPackage( if (bang === -1) return null; const imagePart = pkg.slice(0, bang); const pluginPath = pkg.slice(bang + 1); + /* istanbul ignore next -- unreachable while OCI_REGEX rejects a leading or + trailing `!` upstream (see oci-key.test.ts `invalidCases`); kept so this + function stays total if that validation is ever relaxed. */ if (!imagePart || !pluginPath) return null; return { imagePart, pluginPath }; } @@ -137,6 +140,9 @@ async function isAlreadyInstalled( return true; } + /* istanbul ignore next -- unreachable while PullPolicy has exactly two + members and IF_NOT_PRESENT already returned above; kept so a third policy + defaults to re-installing rather than silently skipping. */ if (pullPolicy !== PullPolicy.ALWAYS) return false; const digestFile = path.join(destination, pathInstalled, IMAGE_HASH_FILE); @@ -144,6 +150,8 @@ async function isAlreadyInstalled( const localDigest = (await fs.readFile(digestFile, 'utf8')).trim(); const parts = splitOciPackage(pkg); + /* istanbul ignore next -- unreachable for the same reason as the guard in + splitOciPackage: `pkg` reached installOciPlugin through ociPluginKey. */ if (!parts) return false; const remoteDigest = await imageCache.getDigest(parts.imagePart); if (localDigest !== remoteDigest) return false; From 0c9c17cd60104a57c3abc8fac21822a159bbf246 Mon Sep 17 00:00:00 2001 From: Gustavo Lira e Silva Date: Wed, 2 Sep 2026 19:22:41 -0300 Subject: [PATCH 2/2] test(install-dynamic-plugins): cover the unrecognised pullPolicy path (RHIDP-16761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit. One of the three guards it called unreachable is reachable from a user's config, so the claim was wrong and the istanbul ignore was hiding a real branch rather than documenting a dead one. `isAlreadyInstalled`'s `if (pullPolicy !== PullPolicy.ALWAYS) return false` was justified on the grounds that PullPolicy has exactly two members and IF_NOT_PRESENT already returned above. That reasoning only holds if the value is actually one of the two. It is not checked: installer.ts reads the config with `parseYaml(rawContent) as DynamicPluginsConfig`, a type assertion with no runtime validation, and `effectivePullPolicy` returns `plugin.pullPolicy` whenever it is truthy. So `pullPolicy: Never` — or `always`, lower-cased — travels from dynamic-plugins.yaml straight into this comparison. The behaviour on that path is the right one: fall through and re-install, rather than silently skip and strand the plugin at whatever version is on disk. It just had no test. It does now, and removing the guard fails exactly that test and nothing else. The pragma on that line is gone, replaced by a comment saying why the branch exists. The line is now covered for real instead of excluded from the report. The other two remain ignored, with better reasons than the first commit gave. The original comments cited OCI_REGEX, which only explains why a trailing `!` is rejected. The reason a package with no `!` at all never reaches splitOciPackage with an empty side is that the merger appends the resolved path first — `!plugin.package.includes('!')` in mergePlugin, and resolveInherit for the {{inherit}} case. Both mechanisms are now named, so a reader can check whether the pragma still holds after changing either. installer-oci.ts stays at 100% on all four metrics, but one of those branches is now covered by a test rather than excluded by a pragma. 259 tests / 19 suites. tsc, prettier and lint clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/installer-oci.test.ts | 26 +++++++++++++++++++ .../src/installer-oci.ts | 17 +++++++----- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.test.ts b/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.test.ts index 2e4c8d807d9..0b57c9e3205 100644 --- a/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.test.ts +++ b/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.test.ts @@ -248,6 +248,32 @@ describe('installOciPlugin — floating tags (RHDHBUGS-1077)', () => { expect(result.pluginPath).toBeNull(); expect(calls.getDigest).toEqual([]); }); + + it('re-installs rather than skipping when the configured pullPolicy is not a recognised value', async () => { + // `dynamic-plugins.yaml` is read with `parseYaml(...) as DynamicPluginsConfig` + // (installer.ts) — a type assertion, not a runtime check — so a typo like + // `pullPolicy: Never` reaches here as an unrecognised string. It must fall + // through to a re-install; silently skipping would strand the plugin at + // whatever version happened to be on disk. + const installed = await seedInstalled('digest-aaaa'); + const tarball = await makeLayerTarball(PLUGIN_PATH, '{"name":"my-plugin"}'); + const { cache, calls } = recordingImageCache({ + digest: 'digest-aaaa', + tarball, + }); + + const result = await installOciPlugin( + ociPlugin({ pullPolicy: 'Never' as PullPolicy }), + destination, + cache, + installed, + ); + + expect(result.pluginPath).toBe(PLUGIN_PATH); + expect(calls.getTarball).toHaveLength(1); + // Never consults the registry for a digest it would not know how to act on. + expect(calls.getDigest).toEqual(['oci://registry.io/org/plugin:latest']); + }); }); describe('installOciPlugin — package spec handling', () => { diff --git a/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts b/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts index 76c52f320d3..87ab418d0e8 100644 --- a/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts +++ b/workspaces/install-dynamic-plugins/packages/install-dynamic-plugins/src/installer-oci.ts @@ -42,9 +42,11 @@ function splitOciPackage( if (bang === -1) return null; const imagePart = pkg.slice(0, bang); const pluginPath = pkg.slice(bang + 1); - /* istanbul ignore next -- unreachable while OCI_REGEX rejects a leading or - trailing `!` upstream (see oci-key.test.ts `invalidCases`); kept so this - function stays total if that validation is ever relaxed. */ + /* istanbul ignore next -- unreachable on the install path: OCI_REGEX rejects + a leading or trailing `!` (oci-key.test.ts `invalidCases`), and a package + with no `!` at all has one appended by the merger before it gets here + (merger.ts, `!plugin.package.includes('!')` and resolveInherit). Kept so + this function stays total if either changes. */ if (!imagePart || !pluginPath) return null; return { imagePart, pluginPath }; } @@ -140,9 +142,9 @@ async function isAlreadyInstalled( return true; } - /* istanbul ignore next -- unreachable while PullPolicy has exactly two - members and IF_NOT_PRESENT already returned above; kept so a third policy - defaults to re-installing rather than silently skipping. */ + // Not unreachable: `dynamic-plugins.yaml` is parsed with a type assertion, + // not a runtime check, so a typo'd `pullPolicy` arrives here as an + // unrecognised string and must fall through to a re-install. if (pullPolicy !== PullPolicy.ALWAYS) return false; const digestFile = path.join(destination, pathInstalled, IMAGE_HASH_FILE); @@ -151,7 +153,8 @@ async function isAlreadyInstalled( const localDigest = (await fs.readFile(digestFile, 'utf8')).trim(); const parts = splitOciPackage(pkg); /* istanbul ignore next -- unreachable for the same reason as the guard in - splitOciPackage: `pkg` reached installOciPlugin through ociPluginKey. */ + splitOciPackage: every `pkg` that reaches here carries a `!`, either + from the user or appended by the merger. */ if (!parts) return false; const remoteDigest = await imageCache.getDigest(parts.imagePart); if (localDigest !== remoteDigest) return false;