test(regression): tag global-variables-crud as @stable + add Credential value hiding test - #245
Conversation
…al value hiding test
There was a problem hiding this comment.
Pull request overview
This PR promotes the Global Variables CRUD regression coverage to @stable, adds a credential secrecy regression test, and documents the related UI/UX scenario.
Changes:
- Added
@stabletags to existing global variable create/delete tests. - Added a new test asserting a credential sentinel value is not visible after save.
- Updated QA checklist and added documentation for the Global Variables CRUD scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
tests/tests-automations/regression/ui-ux/global-variables-crud.spec.ts |
Adds stable tags and a new credential value hiding test. |
QA-CHECKLIST.md |
Marks related Global Variables checklist items as validated. |
docs/ui-ux/global-variables-crud.md |
Documents the covered Global Variables CRUD tests and exclusions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Credential is the default type in the modal — no need to switch tabs | ||
| await page | ||
| .getByPlaceholder("Enter a name for the variable...") | ||
| .fill(varName); | ||
| await page | ||
| .getByPlaceholder("Enter a value for the variable...") | ||
| .fill(sentinelValue); |
There was a problem hiding this comment.
Caught a real bug — fixed in fc12387. Confirmed via tests/tests-automations/regression/ui-ux/globalVariables.spec.ts:43-66 that the modal opens on the Generic tab by default: line 51 asserts "Generic" type label is visible immediately after Add New, and line 63 explicitly clicks credential-tab before saving the Credential variable.
So the previous test was silently creating a Generic variable and asserting its value wasn't leaked — which passes trivially because Generic variables also do not echo their value back into the list.
Fix: inserted await page.getByTestId("credential-tab").click() between the name fill and value fill, matching the existing flow. After the fix:
--workers=1 --retries=0: 3/3 PASS (26.1 s)- Force-fail on the sentinel
toHaveCount(0)→toHaveCount(42): failed exactly on the assertion line, reverted, repassed. - Zero backend errors in the audit run.
|
|
||
| 1. Run setup | ||
| 2. Click "Add New Variable" via JS evaluate | ||
| 3. Credential is the default type — no need to switch tabs |
There was a problem hiding this comment.
Valid — fixed in fc12387 in lockstep with the spec change. The Step-by-step section for Test 3 now has an explicit step "Click credential-tab — the modal opens on the Generic tab by default; this switches it to Credential before save." The misleading "Credential is the default type — no need to switch tabs" line is gone.
| - [-] Create global variable of type "Generic" | ||
| - [x] Delete global variable | ||
| - [x] Create global variable of type "Generic" | ||
| - [x] Credential variable value is hidden from the variable list |
There was a problem hiding this comment.
Declining this one — the Copilot suggestion contradicts the convention in this repo.
CLAUDE.md §QA-CHECKLIST.md (lines 172-177) explicitly states that the Coverage Summary table and the Phase 0 — Validated block (counts + bulleted list) are auto-generated and must never be edited manually. The update-coverage-summary.yml workflow regenerates both on every push to main that touches QA-CHECKLIST.md, the regeneration scripts, or any .spec.ts. The regenerators are idempotent — running them locally either produces no diff (if main is already in sync) or a transient diff that is immediately overwritten on the next push to main, which adds churn without value.
So flipping the bullets to [x] is the only edit needed in this PR; the table and Phase 0 block will be in sync as soon as this merges.
|
|
||
| ## Step by step *(required)* | ||
|
|
||
| **Both tests share the same setup:** |
There was a problem hiding this comment.
Valid, fixed in fc12387. Heading is now "All three tests share the same setup:" — the file has had 3 tests in scope since Test 3 was added; the "Both" carryover was stale.
| // is the value being echoed back into the list, a toast, or any preview. | ||
| await expect( | ||
| page.getByText(sentinelValue, { exact: true }), | ||
| ).toHaveCount(0, { timeout: 5000 }); |
There was a problem hiding this comment.
Valid — fixed in fc12387. Dropped exact: true so getByText(sentinelValue) now does substring matching. A leak embedded in a longer message ("Saved: SECRET-SENTINEL-..." or any labeled preview) now fails the assertion, which matches the guarantee stated by the test name ("hidden from the variable list").
The inline comment was also expanded to explain the substring behaviour and clarify the deliberate gap (input-value attributes still don't count as visible text, since <input type="password" value="…"> is the expected mechanism for masking the editable input). Force-fail probe at toHaveCount(0) → toHaveCount(42) confirmed the assertion fails on the exact line; reverted, repassed.
| 4. Fill name `credential-{timestamp}`, fill value with a distinctive sentinel `SECRET-SENTINEL-{timestamp}` | ||
| 5. Click "Save Variable" | ||
| 6. Sanity: assert variable name is visible in the list | ||
| 7. Critical: assert `getByText(sentinelValue, { exact: true })` has `count() === 0` — value must not surface as visible text anywhere on the page |
There was a problem hiding this comment.
Valid, fixed in fc12387 in lockstep with the spec. The validation criterion now reads:
Credential value (sentinel) has substring-text count 0 anywhere on the page after save (Test 3) —
getByText(sentinelValue)withoutexact: true, so embedded occurrences inside longer messages also fail the test
The Test 3 Step-by-step also calls out the substring behaviour explicitly ("getByText(sentinelValue) (substring match, no exact)") so the doc no longer overstates what the assertion catches.
…ntinel check Addresses Copilot review on PR #245. Two real bugs in the new Test 3: 1. The test never clicked `credential-tab` before saving, so it was silently creating a Generic variable and asserting that the value wasn't leaked. The assertion passed trivially because Generic variables also don't echo their value back into the list. The existing globalVariables.spec.ts:63 confirms Generic is the default tab — Credential requires an explicit click. Fixed by inserting `getByTestId('credential-tab').click()` between name fill and value fill, matching the existing flow. 2. The sentinel assertion used `getByText(sentinelValue, { exact: true })`, which only catches the value when rendered as a standalone exact-match. A leak embedded inside a toast or labeled preview like 'Saved: SECRET-SENTINEL-...' would have passed. Dropped `exact: true` so substring matches also fail the assertion — that matches the guarantee described in the test name ("hidden from the variable list"). Doc updated to reflect both changes (Step-by-step gains the credential-tab click step; validation criterion now describes substring matching). The heading 'Both tests share the same setup' was also stale — fixed to 'All three tests'. 7-step validation pipeline passed: - typecheck clean - lint: 0 errors, 31 pre-existing warnings (no new ones introduced) - 3/3 PASS at --workers=1 --retries=0 (26.1 s) - Force-fail on `toHaveCount(0)` -> `toHaveCount(42)` failed on the expected line, reverted, re-passed - Trace coherent, zero backend errors
Summary
tests/tests-automations/regression/ui-ux/global-variables-crud.spec.tsas@stableafter running the full 7-step validation pipeline (typecheck, lint, anti-pattern checklist, run --retries=0, force-fail per test, --trace=on, backend error audit — all green).getByText(sentinel).toHaveCount(0)). Defends against regressions that would leak API keys / secrets into the list, a toast, or any preview.docs/ui-ux/global-variables-crud.mdto document Test 3 and remove "Credential-type variable creation" from the "does not cover" section.QA-CHECKLIST.mdbullets in §4.3:Create global variable,Delete global variable,Create global variable of type "Generic"flipped[-] → [x]; added a new[x]bullet forCredential variable value is hidden from the variable list.Test plan
npm run typecheck— cleannpx eslint <spec>— 0 errors (warnings are pre-existing patterns already accepted in@stablespecs likewebhook-component-regression,tool-mode)npx playwright test <spec> --workers=1 --retries=0— 3 passed (24.7s) on the validation run, 3 passed (28.3s) on the--trace=onrun🚨 Backend Erroroccurrences in the run outputNotes
The Credential test asserts only that the saved value never surfaces as visible text. It does not interact with the eye/show-value toggle nor assert that the input uses
type="password"— browser-level masking and toggle UX are out of scope and documented as such in the spec doc.