fix(e2e): lock the shared GitHub session, and stop a lane silently booting legacy

docs/changelog.md

@@ -2,7 +2,19 @@
 
 All notable changes to this project will be documented in this file.
 
-## [2.1.11] - Current
+## [2.1.12] - Current
+
+### Added
+
+- **A lane now says which shell it runs, and where that was decided** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): `deploy()` logs `[nfs] <namespace>: new frontend system ON|off, from <mechanism>` — from `deploy()` rather than `configure()`, because the worker fixture calls `configure()` with no arguments for every project before any spec runs, so a lane opting in through `configure({ useNewFrontendSystem: true })` would print `off` first and `ON` second, and the first line is the one a reader greps. Three mechanisms can enable NFS — the `-app-next` project name, `USE_NEW_FRONTEND_SYSTEM=true`, and `configure({ useNewFrontendSystem: true })` — and none of them is visible from a single file, so answering "is this lane NFS?" meant reading three. A namespace ending in `-app-next` that was explicitly configured with `useNewFrontendSystem: false` also warns, because it deploys the legacy shell under a name that reads as an NFS lane in every report. Only that direction is checked: a lane that enables NFS without the suffix is legitimate and common.
+- **`SidebarTabs` accepts both shells' scaffolder label** ([RHIDP-16458](https://redhat.atlassian.net/browse/RHIDP-16458)): the union hardcoded `"Self-service"`, the legacy shell's title. Under app-next the same page is `"Create"`, so a spec running there could not type-check without a cast. Both are members now. This does not decide which label a lane should use — that is [RHIDP-16462](https://redhat.atlassian.net/browse/RHIDP-16462), and three different workarounds are in flight for it.
+
+### Fixed
+
+- **A workspace could silently disable the new frontend system it asked for** ([RHIDP-16457](https://redhat.atlassian.net/browse/RHIDP-16457)): the NFS secret layer is merged *before* the workspace's own `tests/config/rhdh-secrets.yaml`, so a workspace setting `APP_CONFIG_app_packageName` or `ENABLE_STANDARD_MODULE_FEDERATION` for its own reasons overrode it and the lane booted the legacy shell. Nothing failed — the legacy suite re-ran and passed, and the one thing the lane existed to prove was never exercised. `deploy()` now checks the merged secret still carries both markers and throws naming the key, the value found, and the file that overrode it. The merge order itself is unchanged: a workspace must be able to override defaults, which is precisely why the outcome needs checking rather than assuming.
+- **The GitHub session file was shared with no locking** ([RHIDP-16459](https://redhat.atlassian.net/browse/RHIDP-16459)): the path was a bare relative `authState_<user>.json`, resolved against `process.cwd()` — which the worker fixture sets to the same workspace directory for every project. So every lane and every worker shared one file with no lock: a reader landing mid-write failed on truncated JSON, as a flake that looked nothing like the plugin under test, and every added lane adds a writer. Access is now serialised across the whole run, the write goes through a temp file and a rename (removed even when it fails), the path is absolute, and an unreadable or empty session falls through to a full login instead of throwing. Deliberately still **one file per user, not per project**: scoping it per project is the obvious fix and is the wrong one, because `logintoGithub` derives its 2FA code from a single shared TOTP secret, so lanes logging in inside the same 30-second window submit the identical code and GitHub rejects the second.
+
+## [2.1.11]
 
 ### Fixed
 

docs/guide/deployment/rhdh-deployment.md

@@ -89,10 +89,43 @@ await rhdh.deploy();
 
 What gets merged (same order as other config: package defaults → auth → NFS defaults → your workspace files; later wins; secrets are merged then **envsubst** runs once on the result):
 
-1. **Secrets** — `APP_CONFIG_app_packageName: app-next` and `ENABLE_STANDARD_MODULE_FEDERATION: "true"` (your `rhdh-secrets.yaml` still wins on conflicts and can use `$VAR` substitution).
-2. **Dynamic plugins** — Default OCI refs for `red-hat-developer-hub-backstage-plugin-app-auth` and `...-app-integrations` from package YAML; override pins in **`tests/config/dynamic-plugins.yaml`** (same as other plugins).
+1. **Secrets** — `APP_CONFIG_app_packageName: app-next` and `ENABLE_STANDARD_MODULE_FEDERATION: "true"`. Your `rhdh-secrets.yaml` merges last and wins on every other key, and can use `$VAR` substitution — but **not on these two**: overriding either would leave the lane running the legacy shell while calling itself NFS, so `deploy()` throws instead. See below.
+2. **Dynamic plugins** — Default OCI refs for `red-hat-developer-hub-backstage-plugin-app-auth` and `...-app-integrations` from package YAML; override pins in **`tests/config/dynamic-plugins.yaml`** (same as other plugins). Re-pinning is fine; **disabling** either degrades the lane the same way the secrets above would, and that one is not guarded — if an NFS lane behaves like the legacy shell, check this list first.
 3. **Helm** — Package `config/new-frontend-system/value_file.yaml`, then your `value_file.yaml`, then optional `tests/config/value_file-app-next.yaml` when that file exists.
 
+#### Knowing which shell actually ran
+
+`deploy()` logs the answer once, because three mechanisms can decide it and none of them
+is visible from a single file:
+
+```
+[nfs] tech-radar-app-next: new frontend system ON, from the -app-next project name
+[nfs] github: new frontend system ON, from configure({ useNewFrontendSystem: true })
+[nfs] rbac: new frontend system off, from nothing — the default is the legacy shell
+```
+
+Grep `[nfs]` when a lane behaves like the wrong shell. It is logged from `deploy()`
+rather than `configure()` on purpose: the worker fixture calls `configure()` with no
+arguments for every project before any spec runs, so a lane opting in through
+`configure({ useNewFrontendSystem: true })` would otherwise print `off` first.
+
+A namespace ending in `-app-next` that is explicitly configured with
+`useNewFrontendSystem: false` warns — it deploys the legacy shell under a name that
+reads as an NFS lane in every report. The reverse is not checked: enabling NFS without
+the suffix is normal.
+
+If the merged secret would not actually enable NFS, `deploy()` throws rather than
+deploying:
+
+```
+[nfs] "bulk-import-app-next" is configured for the new frontend system, but the
+secret about to be applied would not enable it:
+  APP_CONFIG_app_packageName is "app" (expected "app-next")
+```
+
+That is a post-condition on your own config, not a bug in the harness: remove the
+override from `tests/config/rhdh-secrets.yaml`, or set it to the value named.
+
 Workspace-specific **app-config** (titles, plugin routes, etc.) remains your responsibility.
 
 ### Example: Full Configuration
@@ -150,14 +183,15 @@ await rhdh.deploy({ timeout: null });
 `deploy()` automatically skips if the deployment already succeeded in the current test run (e.g., after a worker restart due to test failure). This prevents expensive re-deployments.
 
 This method:
-1. Merges configuration files (common → auth → optional NFS defaults → project overrides) for app-config, secrets, and dynamic plugins
-2. Substitutes environment variables in the merged secrets (`envsubst`)
-3. [Injects plugin metadata](/guide/configuration/config-files#plugin-metadata-injection) into dynamic plugins config
-4. Applies ConfigMaps (app-config, dynamic-plugins)
-5. Applies Secrets
-6. Installs RHDH via Helm or Operator
-7. Waits for the deployment to be ready
-8. Sets `RHDH_BASE_URL` environment variable
+1. Reports which frontend shell this lane will run, and what decided it — see [New frontend system](#new-frontend-system-usenewfrontendsystem)
+2. Merges configuration files (common → auth → optional NFS defaults → project overrides) for app-config, secrets, and dynamic plugins
+3. Substitutes environment variables in the merged secrets (`envsubst`)
+4. [Injects plugin metadata](/guide/configuration/config-files#plugin-metadata-injection) into dynamic plugins config
+5. Applies ConfigMaps (app-config, dynamic-plugins)
+6. Applies Secrets — **throws** first if the lane asked for the new frontend system and the merged secret would not enable it
+7. Installs RHDH via Helm or Operator
+8. Waits for the deployment to be ready
+9. Sets `RHDH_BASE_URL` environment variable
 
 #### Base URL format
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@red-hat-developer-hub/e2e-test-utils",
-  "version": "2.1.11",
+  "version": "2.1.12",
   "description": "Test utilities for RHDH E2E tests",
   "license": "Apache-2.0",
   "repository": {

src/deployment/rhdh/deployment.ts

@@ -1,5 +1,10 @@
 import { KubernetesClientHelper } from "../../utils/kubernetes-client.js";
 import { WorkspacePaths } from "../../utils/workspace-paths.js";
+import {
+  assertNfsIntentMatches,
+  assertNfsMarkersSurvived,
+  describeNfsSource,
+} from "./nfs-guard.js";
 import { $ } from "../../utils/bash.js";
 import yaml from "js-yaml";
 import os from "os";
@@ -33,6 +38,8 @@ export class RHDHDeployment {
   public k8sClient = new KubernetesClientHelper();
   public rhdhUrl: string;
   public deploymentConfig: DeploymentConfig;
+  /** `configure({ useNewFrontendSystem })` as the caller passed it, for the report. */
+  private _explicitFrontendSystemChoice: boolean | undefined;
 
   constructor(namespace: string) {
     this.deploymentConfig = this._buildDeploymentConfig({ namespace });
@@ -49,6 +56,7 @@ export class RHDHDeployment {
     const executed = await runOnce(
       `deploy-${this.deploymentConfig.namespace}`,
       async () => {
+        this._reportFrontendSystem();
         this._log("Starting RHDH deployment...");
         this._log("RHDH Base URL: " + this.rhdhUrl);
         console.table(this.deploymentConfig);
@@ -120,6 +128,16 @@ export class RHDHDeployment {
 
     const secretPayload = substituted as Record<string, unknown>;
 
+    if (this.deploymentConfig.useNewFrontendSystem) {
+      // The workspace's own secrets file merges last, so the NFS markers can be
+      // overwritten here and nothing downstream would notice: the lane would boot
+      // the legacy shell and pass.
+      assertNfsMarkersSurvived(
+        (secretPayload as { stringData?: Record<string, unknown> }).stringData,
+        this.deploymentConfig.namespace,
+      );
+    }
+
     await this.k8sClient.applySecretFromObject(
       "rhdh-secrets",
       secretPayload as { stringData?: Record<string, string> },
@@ -497,6 +515,9 @@ export class RHDHDeployment {
       "helm";
 
     const namespace = input.namespace ?? this.deploymentConfig.namespace;
+    // Kept so the deploy-time report can name the mechanism; a resolved boolean
+    // cannot distinguish the three sources.
+    this._explicitFrontendSystemChoice = input.useNewFrontendSystem;
     const useNewFrontendSystem =
       input.useNewFrontendSystem ??
       (namespace.endsWith("-app-next") ||
@@ -540,6 +561,32 @@ export class RHDHDeployment {
     );
   }
 
+  /**
+   * Says which shell this lane will run, and where that was decided.
+   *
+   * Three mechanisms can turn NFS on and none of them is visible from a single
+   * file, so "is this lane NFS?" is otherwise answered by reading a project name,
+   * an environment variable and a `configure()` call together.
+   *
+   * Reported from `deploy()` rather than `configure()`: the worker fixture calls
+   * `configure()` with no arguments for every project before any spec runs, so a lane
+   * that opts in with `configure({ useNewFrontendSystem: true })` — which is how
+   * `github` and `homepage` do it — would print `off` first and `ON` second, and the
+   * first line is the one a reader greps.
+   */
+  private _reportFrontendSystem(): void {
+    const { namespace, useNewFrontendSystem } = this.deploymentConfig;
+    const explicitChoice = this._explicitFrontendSystemChoice;
+    // Logged before the check so the resolved state is on record even when the
+    // next line ends the run.
+    console.log(
+      `[nfs] ${namespace}: new frontend system ` +
+        `${useNewFrontendSystem ? "ON" : "off"}, from ` +
+        `${describeNfsSource(namespace, explicitChoice)}`,
+    );
+    assertNfsIntentMatches(namespace, explicitChoice);
+  }
+
   private _buildBaseUrl(): string {
     const prefix =
       this.deploymentConfig.method === "helm"

src/deployment/rhdh/nfs-guard.test.ts

@@ -0,0 +1,168 @@
+import { describe, it } from "node:test";
+import assert from "node:assert";
+import {
+  NFS_SECRET_MARKERS,
+  assertNfsMarkersSurvived,
+  assertNfsIntentMatches,
+  describeNfsIntentConflict,
+  describeNfsSource,
+  findDroppedNfsMarkers,
+} from "./nfs-guard.js";
+
+const [PACKAGE_NAME_KEY] = NFS_SECRET_MARKERS[0];
+const [MF_KEY] = NFS_SECRET_MARKERS[1];
+
+/** A merged secret in which both markers survived, plus unrelated workspace data. */
+const intact = (): Record<string, unknown> => ({
+  ...Object.fromEntries(NFS_SECRET_MARKERS),
+  ["SOME_TOKEN"]: "x",
+});
+
+describe("nfs secret markers", () => {
+  it("finds nothing dropped when both markers survive the merge", () => {
+    assert.deepStrictEqual(findDroppedNfsMarkers(intact()), []);
+  });
+
+  it("reports a marker the workspace overwrote", () => {
+    // tests/config/rhdh-secrets.yaml merges after the NFS defaults, so this is
+    // what a workspace setting the key for its own reasons produces.
+    const dropped = findDroppedNfsMarkers({
+      ...intact(),
+      [PACKAGE_NAME_KEY]: "app",
+    });
+    assert.deepStrictEqual(dropped, [
+      { key: PACKAGE_NAME_KEY, expected: "app-next", actual: "app" },
+    ]);
+  });
+
+  it("reports a marker that is absent, distinctly from one that is wrong", () => {
+    const rest = intact();
+    delete rest[MF_KEY];
+    const dropped = findDroppedNfsMarkers(rest);
+    assert.strictEqual(dropped.length, 1);
+    assert.strictEqual(dropped[0].actual, undefined);
+  });
+
+  it("accepts an unquoted YAML true rather than raising a false alarm", () => {
+    // A typing mistake in the workspace's YAML, not a different value — the intent is
+    // plainly the same. Comparing without coercing would fail a working lane.
+    const dropped = findDroppedNfsMarkers({ ...intact(), [MF_KEY]: true });
+    assert.deepStrictEqual(dropped, []);
+  });
+
+  it("rewrites a boolean marker to the string the API server requires", () => {
+    // stringData goes straight onto the V1Secret body, so a JSON boolean is rejected
+    // with "cannot unmarshal bool into Go struct field ... of type string". Tolerating
+    // it in the comparison alone would green-light a config that cannot apply.
+    const payload = { ...intact(), [MF_KEY]: true };
+    assertNfsMarkersSurvived(payload, "ws-app-next");
+    assert.strictEqual(payload[MF_KEY], "true");
+  });
+
+  it("counts a missing stringData block as both markers dropped", () => {
+    assert.strictEqual(findDroppedNfsMarkers(undefined).length, 2);
+  });
+
+  it("throws naming the namespace, the key, and where the override comes from", () => {
+    assert.throws(
+      () =>
+        assertNfsMarkersSurvived(
+          { ...intact(), [PACKAGE_NAME_KEY]: "app" },
+          "bulk-import-app-next",
+        ),
+      (err: Error) => {
+        assert.match(err.message, /bulk-import-app-next/);
+        assert.match(err.message, /APP_CONFIG_app_packageName is "app"/);
+        assert.match(err.message, /rhdh-secrets\.yaml is merged after/);
+        return true;
+      },
+    );
+  });
+
+  it("does not throw when the markers are intact", () => {
+    assert.doesNotThrow(() =>
+      assertNfsMarkersSurvived(intact(), "ws-app-next"),
+    );
+  });
+});
+
+describe("nfs intent conflict", () => {
+  it("flags an -app-next namespace explicitly opted out of NFS", () => {
+    const msg = describeNfsIntentConflict("quay-app-next", false);
+    assert.match(String(msg), /named -app-next/);
+  });
+
+  it("says nothing when the name and the intent agree", () => {
+    assert.strictEqual(
+      describeNfsIntentConflict("quay-app-next", true),
+      undefined,
+    );
+    assert.strictEqual(describeNfsIntentConflict("quay", false), undefined);
+  });
+
+  it("says nothing when no explicit choice was made", () => {
+    // The default path resolves to NFS from the name, so there is no conflict.
+    assert.strictEqual(
+      describeNfsIntentConflict("quay-app-next", undefined),
+      undefined,
+    );
+  });
+
+  it("fails the deploy on the conflict, rather than only naming it", () => {
+    // A warning on a run that exits 0 is the failure this module exists to stop:
+    // the legacy suite re-runs, everything passes, and nothing proved NFS works.
+    assert.throws(
+      () => assertNfsIntentMatches("quay-app-next", false),
+      /named -app-next/,
+    );
+  });
+
+  it("lets every non-conflicting combination through", () => {
+    assert.doesNotThrow(() => assertNfsIntentMatches("quay-app-next", true));
+    assert.doesNotThrow(() =>
+      assertNfsIntentMatches("quay-app-next", undefined),
+    );
+    assert.doesNotThrow(() => assertNfsIntentMatches("quay", false));
+    assert.doesNotThrow(() => assertNfsIntentMatches("github", true));
+  });
+
+  it("does not flag the reverse, which is legitimate", () => {
+    // github and homepage enable NFS through configure() without renaming, and a
+    // global USE_NEW_FRONTEND_SYSTEM may turn it on everywhere.
+    assert.strictEqual(describeNfsIntentConflict("github", true), undefined);
+  });
+});
+
+describe("nfs source", () => {
+  it("names an explicit choice over the project name", () => {
+    assert.match(describeNfsSource("ws-app-next", true), /configure\(/);
+  });
+
+  it("names the project name when nothing was passed", () => {
+    assert.match(describeNfsSource("ws-app-next", undefined), /-app-next/);
+  });
+
+  it("names the environment variable when it is what decided", () => {
+    const prev = process.env.USE_NEW_FRONTEND_SYSTEM;
+    process.env.USE_NEW_FRONTEND_SYSTEM = "true";
+    try {
+      assert.match(
+        describeNfsSource("ws", undefined),
+        /USE_NEW_FRONTEND_SYSTEM/,
+      );
+    } finally {
+      if (prev === undefined) delete process.env.USE_NEW_FRONTEND_SYSTEM;
+      else process.env.USE_NEW_FRONTEND_SYSTEM = prev;
+    }
+  });
+
+  it("says so when nothing enabled it", () => {
+    const prev = process.env.USE_NEW_FRONTEND_SYSTEM;
+    delete process.env.USE_NEW_FRONTEND_SYSTEM;
+    try {
+      assert.match(describeNfsSource("ws", undefined), /legacy shell/);
+    } finally {
+      if (prev !== undefined) process.env.USE_NEW_FRONTEND_SYSTEM = prev;
+    }
+  });
+});