test(ci): land Windows packaged desktop E2E and registered browser quarantine - #134
Closed
slashdevcorpse wants to merge 2 commits into
Closed
test(ci): land Windows packaged desktop E2E and registered browser quarantine#134slashdevcorpse wants to merge 2 commits into
slashdevcorpse wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
slashdevcorpse has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughThis PR adds packaged desktop Electron E2E coverage, identity-aware process cleanup, renderer readiness probing, a registered browser quarantine system, and expanded Linux/Windows CI quality gates with workflow contract validation. ChangesCross-platform E2E and CI quality gates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant DesktopHarness
participant Electron
participant FakeCodex
participant ProcessTree
CI->>DesktopHarness: Launch packaged desktop fixture
DesktopHarness->>Electron: Start isolated app with network guard
Electron->>FakeCodex: Send app-server protocol requests
FakeCodex-->>Electron: Stream responses and notifications
DesktopHarness->>ProcessTree: Close app and verify process identities
ProcessTree-->>DesktopHarness: Terminate verified process tree
DesktopHarness-->>CI: Publish test results and diagnostics
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
slashdevcorpse has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/e2e/fixtures/network-guard.cjs (1)
38-54: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffShare loopback detection with
apps/desktop/e2e/fixtures/electron-bootstrap.cjs.
normalizeHost/isLoopbackHosthere are duplicated almost verbatim inapps/desktop/e2e/fixtures/electron-bootstrap.cjs(lines 30-46), and they already differ slightly (default host arg"localhost"here vs""there). Since both are the security boundary that decides which destinations are permitted, divergence could open a hole in one layer but not the other. Consider extracting these into a small shared.cjshelper imported by both guards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/e2e/fixtures/network-guard.cjs` around lines 38 - 54, The loopback host logic is duplicated between normalizeHost/isLoopbackHost and electron-bootstrap.cjs, allowing the security checks to diverge. Extract both functions into a shared CommonJS helper, import and reuse them from both guards, and preserve identical normalization and loopback-detection behavior, including one consistent default-host value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/desktop/e2e/fixtures/network-guard.cjs`:
- Around line 38-54: The loopback host logic is duplicated between
normalizeHost/isLoopbackHost and electron-bootstrap.cjs, allowing the security
checks to diverge. Extract both functions into a shared CommonJS helper, import
and reuse them from both guards, and preserve identical normalization and
loopback-detection behavior, including one consistent default-host value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 64fa9170-04e4-460c-8b4e-68b1a17a2668
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
.gitattributes.github/quarantine.yml.github/workflows/ci.yml.gitignoreapps/desktop/e2e/desktop.e2e.tsapps/desktop/e2e/desktop.fixture.test.tsapps/desktop/e2e/desktop.fixture.tsapps/desktop/e2e/fixtures/diagnostic-redaction.cjsapps/desktop/e2e/fixtures/electron-bootstrap.cjsapps/desktop/e2e/fixtures/fake-codex.tsapps/desktop/e2e/fixtures/network-guard.cjsapps/desktop/e2e/global-setup.tsapps/desktop/e2e/playwright.config.tsapps/desktop/e2e/support/processTree.test.tsapps/desktop/e2e/support/processTree.tsapps/desktop/e2e/tsconfig.jsonapps/desktop/package.jsonapps/desktop/scripts/smoke-test-windows-job.windows.integration.test.mjsapps/desktop/src/preload.tsapps/server/src/provider/windowsJobLauncherSourceContract.test.tsapps/web/BROWSER_TEST_QUARANTINE.mdapps/web/package.jsonapps/web/src/components/ChatMarkdown.test.tsxapps/web/src/components/ChatView.browser.tsxapps/web/src/components/Sidebar.import.test.tsapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/vite-env.d.tsapps/web/src/wsNativeApi.test.tsapps/web/src/wsNativeApi.tsapps/web/src/wsTransport.tsapps/web/vitest.browser.geometry.config.tsapps/web/vitest.browser.quarantine.tsapps/web/vitest.browser.stable.config.tspackage.jsonpackages/contracts/package.jsonpackages/contracts/src/ipc.tspackages/shared/package.jsonscripts/benchmark-windows-command-discovery.test.tsscripts/lib/quarantine-registry.test.tsscripts/lib/quarantine-registry.tsscripts/lib/release-worktree-cleanliness.test.tsscripts/lib/workflow-contracts.test.tsscripts/lib/workflow-contracts.tsscripts/package.jsonscripts/quarantine-registry.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/quarantine-registry.ts">
<violation number="1" location="scripts/quarantine-registry.ts:36">
P3: Conflicting `--platform` flags silently select the last platform, so a malformed CI/manual invocation can skip the quarantined suite it appeared to request. Reject a second `--platform` value like `--baseline-ref` does.</violation>
</file>
<file name="apps/web/src/wsNativeApi.ts">
<violation number="1" location="apps/web/src/wsNativeApi.ts:99">
P2: A hung readiness RPC can outlive the advertised 30-second retry window (and the Playwright evaluation), because the deadline is checked only after each default-60-second request settles. Bound/abort each probe request using its remaining deadline so this E2E gate fails or retries within its contract.</violation>
</file>
<file name="scripts/lib/quarantine-registry.ts">
<violation number="1" location="scripts/lib/quarantine-registry.ts:230">
P2: A symlinked registered path can escape the repository despite the containment validation, because `resolve` does not canonicalize symlink targets. Canonicalize existing files with `realpathSync` before applying `isWithinRepository`, while retaining missing-path handling.</violation>
<violation number="2" location="scripts/lib/quarantine-registry.ts:302">
P2: Stable browser runs silently omit any test whose full name contains a line terminator, even when it has no quarantine marker. Use a dot-all equivalent when matching the complete test name.</violation>
</file>
<file name="scripts/lib/workflow-contracts.ts">
<violation number="1" location="scripts/lib/workflow-contracts.ts:1281">
P2: Packaged E2E can rebuild artifacts without contract failure when a workflow uses `bun run build`. Include the root build script in this check so the consumer remains artifact-only.</violation>
<violation number="2" location="scripts/lib/workflow-contracts.ts:1322">
P2: An additional upload-artifact action pinned to another full SHA can bypass the failure-only/redacted diagnostics contract. Count all `actions/upload-artifact@` steps before enforcing the single approved upload.</violation>
</file>
<file name="apps/desktop/e2e/desktop.fixture.ts">
<violation number="1" location="apps/desktop/e2e/desktop.fixture.ts:1259">
P2: Suggest wrapping each line in a try/catch to skip malformed lines instead of failing all remaining validation. A single partial write during concurrent log access can silently suppress all teardown assertions that follow the parse failure.</violation>
</file>
<file name="apps/desktop/e2e/fixtures/diagnostic-redaction.cjs">
<violation number="1" location="apps/desktop/e2e/fixtures/diagnostic-redaction.cjs:10">
P2: SENSITIVE_QUERY_VALUE_PATTERN omits common credential/session-bearing query parameters (`client_secret`, `code`, `state`, `refresh_token`, `id_token`, `csrf_token`, `api_token`, `session`) that could appear in E2E diagnostic URLs. The standalone `token` alternative only matches `?token=abc` exactly — longer names ending in `_token` leak through. Add the missing entries to the alternation so the redaction covers the full set of OAuth and session-bearing query params that diagnostics might encounter.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| // feature session can serve both orchestration and provider RPCs. The generation guards | ||
| // keep every request on that same feature session without changing production retries. | ||
| await requestOnSameSession(() => api.orchestration.getShellSnapshot()); | ||
| const refreshed = await requestOnSameSession(() => api.server.refreshProviders()); |
There was a problem hiding this comment.
P2: A hung readiness RPC can outlive the advertised 30-second retry window (and the Playwright evaluation), because the deadline is checked only after each default-60-second request settles. Bound/abort each probe request using its remaining deadline so this E2E gate fails or retries within its contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/wsNativeApi.ts, line 99:
<comment>A hung readiness RPC can outlive the advertised 30-second retry window (and the Playwright evaluation), because the deadline is checked only after each default-60-second request settles. Bound/abort each probe request using its remaining deadline so this E2E gate fails or retries within its contract.</comment>
<file context>
@@ -51,6 +51,84 @@ import { resolveWsHttpUrl } from "./lib/wsHttpUrl";
+ // feature session can serve both orchestration and provider RPCs. The generation guards
+ // keep every request on that same feature session without changing production retries.
+ await requestOnSameSession(() => api.orchestration.getShellSnapshot());
+ const refreshed = await requestOnSameSession(() => api.server.refreshProviders());
+ await requestOnSameSession(() => api.orchestration.getShellSnapshot());
+ const includesCodex = refreshed.providers.some(
</file context>
| if (options.validateSources !== false) { | ||
| const registeredMarkers = new Map(entries.map((entry) => [entry.marker, entry] as const)); | ||
| for (const entry of entries) { | ||
| const absolutePath = resolve(repositoryRoot, entry.path); |
There was a problem hiding this comment.
P2: A symlinked registered path can escape the repository despite the containment validation, because resolve does not canonicalize symlink targets. Canonicalize existing files with realpathSync before applying isWithinRepository, while retaining missing-path handling.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/quarantine-registry.ts, line 230:
<comment>A symlinked registered path can escape the repository despite the containment validation, because `resolve` does not canonicalize symlink targets. Canonicalize existing files with `realpathSync` before applying `isWithinRepository`, while retaining missing-path handling.</comment>
<file context>
@@ -0,0 +1,358 @@
+ if (options.validateSources !== false) {
+ const registeredMarkers = new Map(entries.map((entry) => [entry.marker, entry] as const));
+ for (const entry of entries) {
+ const absolutePath = resolve(repositoryRoot, entry.path);
+ if (!isWithinRepository(repositoryRoot, absolutePath)) {
+ errors.push(`Quarantine entry \`${entry.id}\` escapes the repository root.`);
</file context>
| const markers = quarantineMarkersForPlatform(registry, platform).map(escapeRegExp); | ||
| if (markers.length === 0) return mode === "stable" ? /.*/ : /$a/; | ||
| const markerPattern = `(?:${markers.join("|")})`; | ||
| return mode === "stable" ? new RegExp(`^(?!.*${markerPattern}).*$`) : new RegExp(markerPattern); |
There was a problem hiding this comment.
P2: Stable browser runs silently omit any test whose full name contains a line terminator, even when it has no quarantine marker. Use a dot-all equivalent when matching the complete test name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/quarantine-registry.ts, line 302:
<comment>Stable browser runs silently omit any test whose full name contains a line terminator, even when it has no quarantine marker. Use a dot-all equivalent when matching the complete test name.</comment>
<file context>
@@ -0,0 +1,358 @@
+ const markers = quarantineMarkersForPlatform(registry, platform).map(escapeRegExp);
+ if (markers.length === 0) return mode === "stable" ? /.*/ : /$a/;
+ const markerPattern = `(?:${markers.join("|")})`;
+ return mode === "stable" ? new RegExp(`^(?!.*${markerPattern}).*$`) : new RegExp(markerPattern);
+}
+
</file context>
| } | ||
| } | ||
|
|
||
| const diagnostics = actionSteps(job, UPLOAD_ARTIFACT_ACTION); |
There was a problem hiding this comment.
P2: An additional upload-artifact action pinned to another full SHA can bypass the failure-only/redacted diagnostics contract. Count all actions/upload-artifact@ steps before enforcing the single approved upload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/workflow-contracts.ts, line 1320:
<comment>An additional upload-artifact action pinned to another full SHA can bypass the failure-only/redacted diagnostics contract. Count all `actions/upload-artifact@` steps before enforcing the single approved upload.</comment>
<file context>
@@ -820,31 +1064,547 @@ function allowedWritePermission(path: string, location: string, scope: string):
+ }
+ }
+
+ const diagnostics = actionSteps(job, UPLOAD_ARTIFACT_ACTION);
+ if (diagnostics.length !== 1) {
+ errors.push(
</file context>
| if ( | ||
| runSteps.some((step) => | ||
| executableShellLines(step.rawCommand).some((line) => | ||
| /(?:^|[\s"'&|;()<>])(?:bun\s+run\s+build:desktop|turbo\s+(?:run\s+)?build)(?=$|[\s"'&|;()<>])/u.test( |
There was a problem hiding this comment.
P2: Packaged E2E can rebuild artifacts without contract failure when a workflow uses bun run build. Include the root build script in this check so the consumer remains artifact-only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/workflow-contracts.ts, line 1279:
<comment>Packaged E2E can rebuild artifacts without contract failure when a workflow uses `bun run build`. Include the root build script in this check so the consumer remains artifact-only.</comment>
<file context>
@@ -820,31 +1064,547 @@ function allowedWritePermission(path: string, location: string, scope: string):
+ if (
+ runSteps.some((step) =>
+ executableShellLines(step.rawCommand).some((line) =>
+ /(?:^|[\s"'&|;()<>])(?:bun\s+run\s+build:desktop|turbo\s+(?:run\s+)?build)(?=$|[\s"'&|;()<>])/u.test(
+ line,
+ ),
</file context>
Suggested change
| /(?:^|[\s"'&|;()<>])(?:bun\s+run\s+build:desktop|turbo\s+(?:run\s+)?build)(?=$|[\s"'&|;()<>])/u.test( | |
| /(?:^|[\s"'&|;()<>])(?:bun\s+run\s+build(?::desktop)?|turbo\s+(?:run\s+)?build)(?=$|[\s"'&|;()<>])/u.test( |
| return raw | ||
| .split(/\r?\n/u) | ||
| .filter(Boolean) | ||
| .map((line) => JSON.parse(line) as JsonRecord); |
There was a problem hiding this comment.
P2: Suggest wrapping each line in a try/catch to skip malformed lines instead of failing all remaining validation. A single partial write during concurrent log access can silently suppress all teardown assertions that follow the parse failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/e2e/desktop.fixture.ts, line 1259:
<comment>Suggest wrapping each line in a try/catch to skip malformed lines instead of failing all remaining validation. A single partial write during concurrent log access can silently suppress all teardown assertions that follow the parse failure.</comment>
<file context>
@@ -0,0 +1,1286 @@
+ return raw
+ .split(/\r?\n/u)
+ .filter(Boolean)
+ .map((line) => JSON.parse(line) as JsonRecord);
+ }
+
</file context>
Comment on lines
+10
to
+11
| const SENSITIVE_QUERY_VALUE_PATTERN = | ||
| /([?&](?:access_token|api_key|auth_token|authorization|token)=)[^&#\s"'<>]*/giu; |
There was a problem hiding this comment.
P2: SENSITIVE_QUERY_VALUE_PATTERN omits common credential/session-bearing query parameters (client_secret, code, state, refresh_token, id_token, csrf_token, api_token, session) that could appear in E2E diagnostic URLs. The standalone token alternative only matches ?token=abc exactly — longer names ending in _token leak through. Add the missing entries to the alternation so the redaction covers the full set of OAuth and session-bearing query params that diagnostics might encounter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/e2e/fixtures/diagnostic-redaction.cjs, line 10:
<comment>SENSITIVE_QUERY_VALUE_PATTERN omits common credential/session-bearing query parameters (`client_secret`, `code`, `state`, `refresh_token`, `id_token`, `csrf_token`, `api_token`, `session`) that could appear in E2E diagnostic URLs. The standalone `token` alternative only matches `?token=abc` exactly — longer names ending in `_token` leak through. Add the missing entries to the alternation so the redaction covers the full set of OAuth and session-bearing query params that diagnostics might encounter.</comment>
<file context>
@@ -0,0 +1,65 @@
+const Path = require("node:path");
+
+const URL_IN_TEXT_PATTERN = /\b(?:https?|wss?|super-synara):\/\/[^\s"'<>]+/giu;
+const SENSITIVE_QUERY_VALUE_PATTERN =
+ /([?&](?:access_token|api_key|auth_token|authorization|token)=)[^&#\s"'<>]*/giu;
+
</file context>
Suggested change
| const SENSITIVE_QUERY_VALUE_PATTERN = | |
| /([?&](?:access_token|api_key|auth_token|authorization|token)=)[^&#\s"'<>]*/giu; | |
| const SENSITIVE_QUERY_VALUE_PATTERN = | |
| /([?&](?:access_token|api_key|api_token|auth_token|authorization|client_secret|code|csrf_token|id_token|refresh_token|session|state|token)=)[^&#\s"'<>]*/giu; |
| if (arg === "--platform") { | ||
| const value = args[index + 1]; | ||
| if (!QUARANTINE_PLATFORMS.includes(value as QuarantinePlatform)) usage(); | ||
| platform = value as QuarantinePlatform; |
There was a problem hiding this comment.
P3: Conflicting --platform flags silently select the last platform, so a malformed CI/manual invocation can skip the quarantined suite it appeared to request. Reject a second --platform value like --baseline-ref does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/quarantine-registry.ts, line 36:
<comment>Conflicting `--platform` flags silently select the last platform, so a malformed CI/manual invocation can skip the quarantined suite it appeared to request. Reject a second `--platform` value like `--baseline-ref` does.</comment>
<file context>
@@ -0,0 +1,172 @@
+ if (arg === "--platform") {
+ const value = args[index + 1];
+ if (!QUARANTINE_PLATFORMS.includes(value as QuarantinePlatform)) usage();
+ platform = value as QuarantinePlatform;
+ index += 1;
+ } else if (arg === "--baseline-ref") {
</file context>
Owner
Author
|
Closing unmerged and keeping #133 as the canonical deferred Windows-first test/CI goal. Exact head 4170d7b is behind main, unit_windows failed, and review reported eight substantive P2/P3 findings plus duplicated loopback security logic. This broad 45-file CI/E2E change is not release-critical or exact-head merge-qualified, so none of it is entering 0.5.5-super.9. |
8 tasks
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #133.
Supersedes the closed, unmerged prototype in #130 with a single current-main commit.
Summary
Scope
This PR is limited to test, fixture, package, and workflow infrastructure. The small source-path additions are inert E2E harness hooks gated by the fixture-only sentinel; they do not change normal product behavior.
Product browser lifecycle work remains tracked in #123, installed artifact qualification in #11, production process ownership in #7, and maintenance-lock coverage in #113.
Verification
Hosted exact-head CI and the repeat Windows packaged-E2E run are authoritative.
Summary by cubic
Adds packaged Windows desktop E2E and a registry-driven browser-test quarantine, and hardens CI on Linux and Windows. Improves reliability with isolated E2E state, strict network guards, redacted diagnostics, and exact, typed quarantine markers.
New Features
[quarantine:<id>]markers via.github/quarantine.yml; Vitest reads the registry to build stable/quarantine suites per platform; CI validates the registry and posts a platform-scoped summary.@playwright/test: offline journeys with a fake Codex, guarded network I/O, redacted artifacts, renderer readiness probe (window.__synaraE2e) with stability checks, a preload sentinel, and process-tree cleanup.Dependencies
@playwright/testtoapps/desktop.Written for commit 4170d7b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores