feat(x2a-backend): make AAP sync timeout configurable via app-config

[hardengl]

Summary

Makes the AAP project sync timeout configurable via app-config.yaml so deployments can tune it without rebuilding the convertor image.

New config option:

x2a:
  credentials:
    aap:
      syncTimeoutSeconds: 300  # default: 120 (convertor default)

Changes:

• config.d.ts: Added syncTimeoutSeconds to the AAP credentials config type and schema
• JobResourceBuilder.ts: Passes AAP_SYNC_TIMEOUT_S env var to the convertor job via the project secret when syncTimeoutSeconds is set

Why: Cross-datacenter deployments (e.g. RDU2→TLV2) experience AAP receptor mesh latency that exceeds the convertor's default 120s timeout, causing a 71% publish failure rate. This was discovered during CI testing where all publish phases consistently timed out.

Companion PR

This requires the convertor-side change to read the AAP_SYNC_TIMEOUT_S env var:

• Increase AAP project sync timeout from 120s to 300s x2ansible/x2a-convertor#258

Both PRs are needed together:

1. Convertor PR feat(fab): update fab content #258 — reads AAP_SYNC_TIMEOUT_S env var (defaults to 120s if unset, backward-compatible)
2. This PR — passes the configured value from app-config.yaml to the convertor job

cc @pczarnik @eloy-coto

Made with Cursor

[github-actions]

This pull request adds a new top-level directory under workspaces/. Please follow Submitting a Pull Request for a New Workspace in CONTRIBUTING.md.

[sonarqubecloud]

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[rhdh-qodo-merge]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (9) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 22 rules

✅ Cross-repo context

  Not relevant to this PR: redhat-developer/rhdh

  Not relevant to this PR: redhat-developer/rhdh-chart

  Not relevant to this PR: redhat-developer/rhdh-operator

  Not relevant to this PR: redhat-developer/rhdh-local

1. outputDir missing appMode suffix 📘 Rule violation ≡ Correctness

Description

playwright.config.ts sets fixed outputFolder/outputDir names without an APP_MODE-derived
suffix, so legacy/NFS (or other modes) can overwrite each other’s artifacts. This violates the
requirement to suffix Playwright report/output directories with appMode.

Code

workspaces/x2a/playwright.config.ts[R49-53]

+  reporter: [
+    ['list'],
+    ['html', { open: 'never', outputFolder: 'e2e-test-report' }],
+    ['junit', { outputFile: 'playwright-results.xml' }],
+  ],

Relevance

⭐⭐ Medium

No evidence about appMode-suffixed Playwright output/report dirs; existing config uses fixed names
(PR 2194).

PR-#2194

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2600 requires Playwright reporter output folder and outputDir to be suffixed
with an appMode value derived from APP_MODE. The updated config hardcodes `outputFolder:
'e2e-test-report' and outputDir: 'test-results' without any -${appMode}` suffix.

Rule 2600: Playwright report and output directories must be suffixed with appMode
workspaces/x2a/playwright.config.ts[49-66]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Playwright reporter/output directories are not suffixed with an `APP_MODE`-derived value (e.g., `-legacy`), which can cause collisions between runs.

## Issue Context
Compliance requires the HTML report `outputFolder` and `outputDir` to include `-${appMode}` where `appMode = process.env.APP_MODE || 'legacy'`.

## Fix Focus Areas
- workspaces/x2a/playwright.config.ts[49-66]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Playwright start command hardcoded 📘 Rule violation ≡ Correctness

Description

playwright.config.ts hardcodes the webServer commands and does not derive them from APP_MODE,
preventing mode-specific start behavior. This violates the requirement to dynamically set the start
command based on APP_MODE.

Code

workspaces/x2a/playwright.config.ts[R26-29]

+  webServer: process.env.PLAYWRIGHT_URL
    ? []
    : [
        {

Relevance

⭐⭐ Medium

No evidence for APP_MODE-derived Playwright start commands; existing config hardcodes commands (PR
2194).

PR-#2194

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2602 requires Playwright to derive the start command from APP_MODE. The config
hardcodes command: 'yarn start app' and command: 'yarn start backend' without any
APP_MODE-based branching.

Rule 2602: Playwright config must derive start command from APP_MODE environment variable
workspaces/x2a/playwright.config.ts[26-41]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Playwright `webServer` start commands are hardcoded and not derived from `process.env.APP_MODE`.

## Issue Context
Compliance requires reading `APP_MODE` (default `legacy`) and selecting a mode-appropriate start command.

## Fix Focus Areas
- workspaces/x2a/playwright.config.ts[26-41]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. webServer missing cwd: __dirname 📘 Rule violation ≡ Correctness

Description

playwright.config.ts defines multiple webServer entries but omits cwd: __dirname in each
entry. This violates the requirement for multi-webServer configs to include cwd: __dirname per
entry.

Code

workspaces/x2a/playwright.config.ts[R26-29]

+  webServer: process.env.PLAYWRIGHT_URL
    ? []
    : [
        {

Relevance

⭐⭐ Medium

No evidence requiring cwd: __dirname per webServer entry; existing config omitted it (PR 2194).

PR-#2194

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2607 requires that when multiple web servers are configured, webServer must be an
array and each entry must include cwd: __dirname. The config has two webServer entries and
neither includes cwd.

Rule 2607: Multiple Playwright webServer entries must be an array with cwd: __dirname
workspaces/x2a/playwright.config.ts[26-41]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Playwright config uses multiple `webServer` entries but does not set `cwd: __dirname` in each entry.

## Issue Context
Compliance requires `webServer` to be an array and each element to include `cwd: __dirname`.

## Fix Focus Areas
- workspaces/x2a/playwright.config.ts[26-41]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. baseURL uses backend port 📘 Rule violation ≡ Correctness

Description

In CI (and when PLAYWRIGHT_URL is unset), workspaces/x2a/playwright.config.ts sets Playwright’s
use.baseURL to http://localhost:7007, which is the backend port rather than the required
frontend port. This violates the compliance rule that baseURL must target the frontend (with
backend URLs used only for readiness checks) and will cause UI flows like the auth helper that
navigates to '/' and waits for frontend elements (e.g., the “Enter” button) to hit the wrong
origin and likely fail, especially now that the PR enables running the full Playwright E2E suite via
yarn test:e2e.

Code

workspaces/x2a/playwright.config.ts[R55-56]

  use: {
    actionTimeout: 0,

Relevance

⭐⭐ Medium

Playwright baseURL=7007 in CI already merged without objection (PR 2194); no later fix history
found.

PR-#2194

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance ID 2672 requires Playwright baseURL to be the frontend port, but the cited Playwright
configuration sets use.baseURL to http://localhost:7007 when process.env.CI is true (and thus
when PLAYWRIGHT_URL is not provided). The auth fixture then navigates to '/' and waits for
UI-specific elements, which implies the base URL must serve the frontend; additionally, the
repository configuration indicates the frontend is on port 3000 while the backend is on 7007, and
package.json now runs the Playwright E2E suite via test:e2e, increasing the likelihood this
misconfiguration immediately breaks CI runs.

Rule 2672: Playwright project baseURL must be the frontend port, webServer readiness URL must be the backend port
workspaces/x2a/playwright.config.ts[55-63]
workspaces/x2a/playwright.config.ts[19-66]
workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[19-31]
workspaces/x2a/app-config.yaml[1-27]
workspaces/x2a/package.json[20-27]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Playwright’s `use.baseURL` defaults to the backend origin (`http://localhost:7007`) in CI when `PLAYWRIGHT_URL` is unset, even though compliance requires `baseURL` to point to the frontend port (e.g., `3000`) and backend URLs should only be used for readiness checks; this also breaks UI-oriented helpers (like the auth fixture) that navigate to `'/'` and expect rendered frontend elements.

## Issue Context
- Compliance (PR Compliance ID 2672) requires project `baseURL` to be the frontend port, while `webServer` readiness checks may use the backend port.
- The auth helper/fixture navigates to `'/'` and waits for frontend UI elements (e.g., an “Enter” button), so the base origin must be where the app UI is served.
- Repo configuration indicates frontend is on `3000` and backend is on `7007`.
- The PR now enables running the full Playwright E2E suite via `yarn test:e2e`, making this CI default more impactful.

## Fix Focus Areas
- workspaces/x2a/playwright.config.ts[26-66]
- workspaces/x2a/playwright.config.ts[55-63]
- workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[19-31]
- workspaces/x2a/app-config.yaml[1-27]
- workspaces/x2a/package.json[20-27]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. CSS locator used in e2e 📘 Rule violation ⚙ Maintainability

Description

E2E helpers rely on CSS/text selectors instead of role-based queries (or translated-text queries),
increasing fragility to DOM changes. This violates the selector strategy requirement for E2E tests.

Code

workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[R19-30]

+export async function performGuestLogin(page: Page) {
+  await page.goto('/');
+  await page.waitForLoadState('domcontentloaded', { timeout: 30000 });
+
+  const enterButton = page.locator('button:has-text("Enter")');
+  await enterButton.waitFor({ state: 'visible', timeout: 15000 });
+  await enterButton.click();
+
+  await page
+    .locator('nav')
+    .first()
+    .waitFor({ state: 'visible', timeout: 60000 });

Relevance

⭐⭐⭐ High

Team pushed to replace fragile CSS selectors with role/label locators in E2E helpers (PR 3249).

PR-#3249

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2616 disallows brittle selectors like CSS/structure-based locators in E2E tests.
The newly added auth fixture uses page.locator('button:has-text("Enter")') and
page.locator('nav') instead of role-based queries.

Rule 2616: E2E selectors must use role-based or translated-text queries instead of class names
workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[19-31]
workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[31-47]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
E2E helper code uses CSS/text locators (e.g., `page.locator('button:has-text(...)')`) instead of role-based selectors.

## Issue Context
Compliance requires role-based selectors (`getByRole`) or translated-text queries to reduce brittleness.

## Fix Focus Areas
- workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[19-30]
- workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[31-47]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Null timeout injected 🐞 Bug ☼ Reliability

Description

JobResourceBuilder injects AAP_SYNC_TIMEOUT_S whenever syncTimeoutSeconds is not undefined, so null
(or other unexpected runtime types) can be passed downstream as a string (e.g., "null"), leading to
unintended timeout behavior in the job environment. This is possible because KubeService forwards
raw x2a.credentials.aap config without validating syncTimeoutSeconds’ type/range.

Code

workspaces/x2a/plugins/x2a-backend/src/services/JobResourceBuilder.ts[R135-139]

+
+        // AAP sync timeout (seconds) — convertor reads AAP_SYNC_TIMEOUT_S env var
+        ...(syncTimeout !== undefined
+          ? { AAP_SYNC_TIMEOUT_S: String(syncTimeout) }
+          : {}),

Relevance

⭐⭐⭐ High

Team accepted strict type validation for config fields (PR #3536) and config value clamping (PR
#3097).

PR-#3536
PR-#3097

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The Secret injection uses a !== undefined check, which includes null, and the config-building
path copies the AAP config through without validating the new field’s type/value.

workspaces/x2a/plugins/x2a-backend/src/services/JobResourceBuilder.ts[96-141]
workspaces/x2a/plugins/x2a-backend/src/services/KubeService.ts[511-569]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`syncTimeoutSeconds` is conditionally injected into the project Secret when it is `!== undefined`, which unintentionally includes `null` and other invalid values. This can propagate malformed values into the job environment.

## Issue Context
The runtime config object is built by passing through `rawConfig.credentials.aap` largely as-is, so a mis-typed value can reach `JobResourceBuilder`.

## Fix Focus Areas
- workspaces/x2a/plugins/x2a-backend/src/services/JobResourceBuilder.ts[96-141]
- workspaces/x2a/plugins/x2a-backend/src/services/KubeService.ts[511-569]

## Suggested fix
- In `KubeService` (preferred), normalize/validate `rawConfig.credentials.aap?.syncTimeoutSeconds` before placing it into `x2aConfig`:
 - Treat `null` as unset.
 - Accept only finite numbers (optionally `Number.isInteger`) and enforce a sane lower bound (e.g., `> 0`).
 - If invalid, either (a) throw a boot-time config error (fail fast) or (b) omit the value and log a warning.
- In `JobResourceBuilder`, tighten the conditional to `syncTimeout != null` and (optionally) only inject when validated (e.g., `Number.isFinite(syncTimeout)`), so `null` doesn’t become `"null"`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Hardcoded UI strings in e2e 📘 Rule violation ⚙ Maintainability

Description

E2E tests assert against hardcoded user-facing UI strings instead of using translation keys loaded
from the plugin translation modules. This makes tests brittle across locales and violates the
translation-key selector requirement.

Code

workspaces/x2a/packages/app/e2e-tests/app.test.ts[R25-33]

+  if (devMode) {
+    await expect(
+      page.getByRole('heading', { name: 'Red Hat Catalog' }),
+    ).toBeVisible({ timeout: 10000 });
+  } else {
+    await expect(
+      page.getByRole('heading', { name: 'Welcome back!' }),
+    ).toBeVisible({ timeout: 10000 });
+  }

Relevance

⭐⭐ Medium

No repo history enforcing translation keys in E2E; i18n for UI strings accepted in UI code (PR
3144).

PR-#3144

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2596 requires E2E tests to use translation keys rather than hardcoded UI strings.
The modified test asserts headings by literal text (Red Hat Catalog, Welcome back!) instead of
using a translation lookup.

Rule 2596: E2E tests must use translation keys instead of hardcoded UI strings
workspaces/x2a/packages/app/e2e-tests/app.test.ts[25-33]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
E2E tests use hardcoded UI strings in assertions/selectors rather than translation keys.

## Issue Context
Compliance expects tests to load translations (e.g., via a `getTranslations` helper) and use `messages['key']` instead of literal strings.

## Fix Focus Areas
- workspaces/x2a/packages/app/e2e-tests/app.test.ts[25-33]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (4)

8. E2E helpers outside utils 📘 Rule violation ⚙ Maintainability

Description

Reusable E2E helper logic is introduced outside an e2e-tests/utils/ directory, which encourages
duplication and inconsistent helper usage. This violates the requirement to place reusable E2E logic
under utils/.

Code

workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[R1-53]

+/*
+ * Copyright Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Page } from '@playwright/test';
+
+export async function performGuestLogin(page: Page) {
+  await page.goto('/');
+  await page.waitForLoadState('domcontentloaded', { timeout: 30000 });
+
+  const enterButton = page.locator('button:has-text("Enter")');
+  await enterButton.waitFor({ state: 'visible', timeout: 15000 });
+  await enterButton.click();
+
+  await page
+    .locator('nav')
+    .first()
+    .waitFor({ state: 'visible', timeout: 60000 });
+}
+
+export async function performLogin(
+  page: Page,
+  _username?: string,
+  _password?: string,
+) {
+  await page.goto('/');
+  await page.waitForLoadState('domcontentloaded', { timeout: 30000 });
+
+  const nav = page.locator('nav').first();
+  const enterButton = page.locator('button:has-text("Enter")');
+
+  try {
+    await enterButton.waitFor({ state: 'visible', timeout: 15000 });
+    await enterButton.click();
+  } catch {
+    if (await nav.isVisible()) return;
+    throw new Error('Neither Enter button nor nav appeared within timeout');
+  }
+
+  await nav.waitFor({ state: 'visible', timeout: 60000 });
+}

Relevance

⭐⭐ Medium

No historical evidence about required e2e-tests/utils structure; x2a E2E scaffolding was minimal (PR
2194).

PR-#2194

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2622 requires reusable E2E helper logic to be placed under a utils/ directory
within the E2E test directory. This PR adds helper modules at e2e-tests/fixtures/auth.ts and
e2e-tests/pages/X2AnsiblePage.ts instead of e2e-tests/utils/....

Rule 2622: Reusable e2e test logic must be placed in a utils/ directory under the e2e test directory
workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[1-53]
workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[1-80]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New reusable E2E helper modules are added under `fixtures/` and `pages/` rather than under an `utils/` directory.

## Issue Context
Compliance expects reusable navigation/auth/panel logic to live in `e2e-tests/utils/` and be imported from there.

## Fix Focus Areas
- workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts[1-53]
- workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[1-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. toLowerCase() used in smoke 📘 Rule violation ≡ Correctness

Description

The E2E smoke test uses toLowerCase() for normalization instead of toLocaleLowerCase('en-US'),
which can behave differently under certain locales. This violates the locale-explicit case
conversion requirement.

Code

workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[R503-509]

+    const statusSection = page.locator('h2:has-text("Status")').locator('..');
+    const statusText =
+      (await statusSection.textContent().catch(() => '')) ?? '';
+    expect(
+      statusText.toLowerCase(),
+      `Expected initialized/success status, got: "${statusText}"`,
+    ).toMatch(/success|initialized|init/i);

Relevance

⭐⭐ Medium

No historical evidence found enforcing toLocaleLowerCase('en-US') over toLowerCase in tests.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1733 requires using toLocaleLowerCase('en-US') instead of toLowerCase() for
English-intended comparisons/normalization. The smoke test calls statusText.toLowerCase() and
rowText.toLowerCase() in new assertions.

Rule 1733: Use locale-aware case conversion with explicit locale for strings
workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[503-509]
workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[795-799]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Case normalization uses `toLowerCase()` instead of locale-explicit `toLocaleLowerCase('en-US')`.

## Issue Context
Compliance requires explicit locale for deterministic case conversion on English-intended text.

## Fix Focus Areas
- workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[503-509]
- workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[795-799]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. document used without window 📘 Rule violation ⚙ Maintainability

Description

Code executed in the browser context references document as a bare global instead of
window.document. This violates the requirement to explicitly access browser globals via window.

Code

workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[R198-206]

+            .evaluate(() => {
+              const buttons = Array.from(document.querySelectorAll('button'));
+              const auth = buttons.find(
+                b =>
+                  b.textContent?.includes('Authorize') &&
+                  !b.textContent?.includes('Cancel'),
+              );
+              auth?.click();
+            })

Relevance

⭐⭐ Medium

No historical evidence found requiring window.document in Playwright evaluate callbacks.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1769 requires browser globals to be accessed via window. The added
popup.evaluate callback uses document.querySelectorAll(...) as a bare identifier.

Rule 1769: Browser globals must be explicitly accessed via window
workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[198-206]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Browser globals are referenced without `window.` (e.g., `document` instead of `window.document`) inside code evaluated in the page context.

## Issue Context
Compliance requires explicit `window.` access to browser globals.

## Fix Focus Areas
- workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts[198-206]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. E2E GitHub token unguarded 🐞 Bug ☼ Reliability

Description

Some of the new Playwright E2E tests use getGitHubToken() which can return an empty string and then
proceed to call GitHub APIs and/or start X2A runs with that token, causing authentication failures
that are not skipped or failed fast with a clear prerequisite error. This makes E2E runs brittle
compared to smoke.test.ts which explicitly requires GITHUB_TOKEN.

Code

workspaces/x2a/packages/app/e2e-tests/analyze-bad-path.test.ts[R45-83]

+function getGitHubToken(): string {
+  return (
+    process.env.RHDH_ORCHESTRATOR_GITHUB_TOKEN || process.env.GITHUB_TOKEN || ''
+  );
+}
+
+async function getGuestToken(baseURL: string): Promise<string> {
+  if (guestToken) return guestToken;
+  const ctx = await request.newContext({ baseURL, ignoreHTTPSErrors: true });
+  const resp = await ctx.post('/api/auth/guest/refresh');
+  const data = await resp.json();
+  await ctx.dispose();
+  guestToken = data?.backstageIdentity?.token ?? '';
+  return guestToken;
+}
+
+async function apiHeaders(baseURL: string) {
+  const token = await getGuestToken(baseURL);
+  return {
+    Authorization: `Bearer ${token}`,
+    'Content-Type': 'application/json',
+  };
+}
+
+// --- GitHub API helpers ---
+
+async function githubApi(
+  method: string,
+  path: string,
+  body?: unknown,
+): Promise<{ status: number; data: unknown }> {
+  const ghToken = getGitHubToken();
+  const ctx = await request.newContext({
+    baseURL: 'https://api.github.com',
+    extraHTTPHeaders: {
+      Authorization: `token ${ghToken}`,
+      Accept: 'application/vnd.github.v3+json',
+    },
+  });

Relevance

⭐⭐ Medium

No similar historical review on E2E GitHub token guarding; team likes fail-fast error handling
elsewhere (PR 2585).

PR-#2585

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added tests build GitHub requests with Authorization: token ${ghToken} where ghToken can be
'', and other tests pass this value as repo auth tokens; smoke.test.ts demonstrates an existing
fail-fast approach that these new tests do not follow.

workspaces/x2a/packages/app/e2e-tests/analyze-bad-path.test.ts[45-101]
workspaces/x2a/packages/app/e2e-tests/export-all-files.test.ts[65-134]
workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[32-41]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Several E2E tests proceed with an empty GitHub token, which will predictably fail when the test attempts GitHub API operations (branch/file mutations) or repository-authenticated X2A runs.

## Issue Context
There is already a stricter pattern in `smoke.test.ts` (`requireGitHubToken`) that throws early when `GITHUB_TOKEN` is missing.

## Fix Focus Areas
- workspaces/x2a/packages/app/e2e-tests/analyze-bad-path.test.ts[45-101]
- workspaces/x2a/packages/app/e2e-tests/export-all-files.test.ts[65-134]
- workspaces/x2a/packages/app/e2e-tests/smoke.test.ts[32-41]

## Suggested fix
- Introduce a shared helper (e.g., `fixtures/github.ts`) that either:
 - `throw`s with a clear message when required env vars are missing, or
 - uses `test.skip(!token, '...')` for live tests.
- Update tests using `getGitHubToken()` to use the shared helper so behavior is consistent across the suite.
- (Optional) If using both `RHDH_ORCHESTRATOR_GITHUB_TOKEN` and `GITHUB_TOKEN`, ensure the helper checks both and documents precedence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[rhdh-qodo-merge]

PR Summary by Qodo

Make AAP sync timeout configurable and add X2A Playwright E2E suite

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add configurable AAP project-sync timeout and pass it to convertor via env var.
• Introduce Playwright E2E tests covering X2A wizard navigation and pipeline phases.
• Tune Playwright config and package scripts for CI reporting and execution.

Diagram

graph TD
  A["app-config.yaml"] --> B(["x2a-backend config"]) --> C(["JobResourceBuilder"]) --> D["K8s Secret"] --> E(["Convertor job"]) --> F["AAP"]
  G(["Playwright E2E"]) --> H(["Backstage UI/APIs"]) --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize Playwright auth/polling helpers

• ➕ Reduces repeated guest-token, headers, and polling code across many specs
• ➕ Makes retry/timeout behavior consistent and easier to tune in CI
• ➕ Improves maintainability as endpoints evolve
• ➖ Requires follow-up refactor of newly-added tests
• ➖ Can hide intent if abstractions become too generic

2. Model AAP sync timeout outside credentials block

• ➕ Avoids mixing operational tuning with credential configuration
• ➕ Creates room for future operational knobs (per-phase/per-target settings)
• ➖ More config churn and possible migration overhead
• ➖ Current change is minimal and backwards-compatible

3. Make sync timeout a per-run parameter

• ➕ Allows overriding behavior without redeploying
• ➕ Supports one-off long-running conversions
• ➖ Requires API/UI plumbing and validation
• ➖ Harder to enforce safe defaults and consistent operations

Recommendation: The current approach (optional config → env var injection) is pragmatic and low-risk; keep it. Consider a follow-up to extract common Playwright utilities (auth, headers, polling, project lifecycle) into shared fixtures/helpers to reduce duplication and stabilize CI behavior.

Files changed (18) +4950 / -21

Enhancement (1) +7 / -0

JobResourceBuilder.tsInject AAP_SYNC_TIMEOUT_S env var into convertor job when configured +7/-0

Inject AAP_SYNC_TIMEOUT_S env var into convertor job when configured

• Reads syncTimeoutSeconds from config and conditionally adds AAP_SYNC_TIMEOUT_S to the secret data passed into the convertor job environment.

workspaces/x2a/plugins/x2a-backend/src/services/JobResourceBuilder.ts

Tests (14) +4915 / -12

analyze-bad-path.test.tsAdd analyze-phase invalid module path E2E regression test +564/-0

Add analyze-phase invalid module path E2E regression test

• Adds an end-to-end test that verifies analyze fails when a module source path disappears, using GitHub API branch/file manipulation and X2A API polling.

workspaces/x2a/packages/app/e2e-tests/analyze-bad-path.test.ts

app.test.tsReplace noop test with welcome-page smoke assertion +12/-12

Replace noop test with welcome-page smoke assertion

• Performs guest login and verifies the expected landing page heading depending on dev vs hosted mode.

workspaces/x2a/packages/app/e2e-tests/app.test.ts

conversion-flow.test.tsAdd conversion wizard navigation/E2E coverage +134/-0

Add conversion wizard navigation/E2E coverage

• Introduces tests for navigating to X2A and loading the scaffolder template; full wizard happy-path assertions are currently skipped due to known scaffolder rendering issues.

workspaces/x2a/packages/app/e2e-tests/conversion-flow.test.ts

edit-project.test.tsAdd edit-project API E2E test +520/-0

Add edit-project API E2E test

• Creates a project via API and verifies editable fields (name/ownedBy/description) can be updated through the project PATCH endpoint.

workspaces/x2a/packages/app/e2e-tests/edit-project.test.ts

export-all-files.test.tsAdd publish/export completeness + AAP sync regression gate +469/-0

Add publish/export completeness + AAP sync regression gate

• Validates publish pushes all tracked files to the target repo and completes with success status, acting as a regression gate for AAP sync timing/race issues.

workspaces/x2a/packages/app/e2e-tests/export-all-files.test.ts

auth.tsAdd reusable Playwright login helpers +53/-0

Add reusable Playwright login helpers

• Adds helpers for guest/login flows that click Enter when needed and wait for navigation UI to be visible.

workspaces/x2a/packages/app/e2e-tests/fixtures/auth.ts

navigation.test.tsAdd X2A navigation regression tests +70/-0

Add X2A navigation regression tests

• Covers direct URL and sidebar navigation and asserts URL correctness; includes additional skipped wizard step navigation tests.

workspaces/x2a/packages/app/e2e-tests/navigation.test.ts

X2AnsiblePage.tsIntroduce X2A Playwright page object +383/-0

Introduce X2A Playwright page object

• Adds a page object encapsulating common UI actions: navigation, scaffolder wizard interactions, GitHub auth dialog handling, and module phase execution helpers.

workspaces/x2a/packages/app/e2e-tests/pages/X2AnsiblePage.ts

phase-duration-attempts.test.tsAdd telemetry-based phase duration/attempt count E2E test +449/-0

Add telemetry-based phase duration/attempt count E2E test

• Validates phase duration and attempt count exposure, skipping gracefully when required response fields are not present on the deployed backend.

workspaces/x2a/packages/app/e2e-tests/phase-duration-attempts.test.ts

pipeline-phases.test.tsAdd pipeline phases E2E test (API init + UI phases) +411/-0

Add pipeline phases E2E test (API init + UI phases)

• Creates a project and triggers init via API, then drives analyze/migrate/publish via the module UI while polling backend state for phase completion.

workspaces/x2a/packages/app/e2e-tests/pipeline-phases.test.ts

project-rules.test.tsAdd project rules CRUD E2E test +324/-0

Add project rules CRUD E2E test

• Exercises /api/x2a/rules endpoints for CRUD, validation, and association behavior using guest-auth API calls.

workspaces/x2a/packages/app/e2e-tests/project-rules.test.ts

resync-migration.test.tsAdd migration resync E2E test +265/-0

Add migration resync E2E test

• Validates the refresh=true run path re-reads the migration plan and updates the module list accordingly.

workspaces/x2a/packages/app/e2e-tests/resync-migration.test.ts

smoke.test.tsAdd end-to-end pipeline smoke test with resilient auth/polling +839/-0

Add end-to-end pipeline smoke test with resilient auth/polling

• Adds a comprehensive smoke suite that runs a full pipeline against real deployments, including token refresh and robust polling/timeouts.

workspaces/x2a/packages/app/e2e-tests/smoke.test.ts

source-dir-resolution.test.tsAdd source_dir='.' resolution regression test +422/-0

Add source_dir='.' resolution regression test

• Covers repo-root cookbook/module discovery and validates path resolution behavior for export/plan context when sourcePath is '.'.

workspaces/x2a/packages/app/e2e-tests/source-dir-resolution.test.ts

Other (3) +28 / -9

package.jsonEnable and extend Playwright E2E scripts +4/-1

Enable and extend Playwright E2E scripts

• Replaces the placeholder e2e script and adds UI/headed/debug variants for running Playwright locally and in CI.

workspaces/x2a/package.json

playwright.config.tsUpdate Playwright configuration for CI and external base URL +11/-8

Update Playwright configuration for CI and external base URL

• Switches webServer behavior to use PLAYWRIGHT_URL, adds list+HTML+JUnit reporters, configures workers, ignores HTTPS errors, and moves outputs to test-results.

workspaces/x2a/playwright.config.ts

config.d.tsAdd AAP syncTimeoutSeconds config typing and docs +13/-0

Add AAP syncTimeoutSeconds config typing and docs

• Extends X2A AAP credentials config to include optional syncTimeoutSeconds with documentation about cross-datacenter latency and default behavior.

workspaces/x2a/plugins/x2a-backend/config.d.ts