test: harden cross-platform CI and packaged desktop E2E - #130
Closed
slashdevcorpse wants to merge 10 commits into
Closed
test: harden cross-platform CI and packaged desktop E2E#130slashdevcorpse wants to merge 10 commits into
slashdevcorpse wants to merge 10 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 registry-driven browser quarantine management, restructures CI across Linux and Windows, introduces packaged Electron desktop E2E coverage with isolated fixtures and process cleanup, preserves adopted webview URLs during loading, and adjusts test runtime configuration. ChangesQuarantine registry and CI
Desktop E2E harness
Adopted webview handling
Test stability and platform fixtures
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/lib/quarantine-registry.test.ts (1)
50-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of the happy paths; a few validation branches remain untested.
Duplicate id, sorted-id-order violations, path escaping the repo root, non-existent/non-file paths, and a marker missing from its declared file are all handled in
validateQuarantineRegistrybut have no corresponding test case here.🤖 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 `@scripts/lib/quarantine-registry.test.ts` around lines 50 - 157, Extend the “quarantine registry” tests to cover the remaining validateQuarantineRegistry validation branches: duplicate ids, ids not in sorted order, paths escaping repositoryRoot, missing or directory paths, and markers absent from their declared source file. For each case, construct a minimal invalid registry fixture and assert the corresponding validation error.apps/web/vitest.browser.quarantine.ts (1)
23-25: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid rescanning the repository on browser-config load
browserQuarantineTestNamePattern()is used by both browser configs, so this walksapps/,packages/, andscripts/twice during browser-suite startup. CI already validates the registry separately, sovalidateSources: falsehere would keep the registry/schema checks without the redundant traversal.🤖 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/web/vitest.browser.quarantine.ts` around lines 23 - 25, Update the validateQuarantineRegistry call in the browser configuration load to pass validateSources: false alongside repositoryRoot, preserving registry and schema validation while avoiding the redundant repository scan.
🤖 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.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 657-678: Update the quality aggregate gate to explicitly include
the macos_arm64 job if macOS is intended to be required for merging: add it to
needs and require its result to equal success alongside the existing
dependencies. If macos_arm64 is intentionally non-blocking, leave the workflow
unchanged and document or confirm that intent.
In `@scripts/lib/quarantine-registry.ts`:
- Around line 145-240: Refactor the validation loop around the entry field
checks to record the error count before validating each entry, then append the
constructed entry only when no new errors were added. Remove the duplicated
conjunctive predicate guarding entries.push, while preserving all existing
validations, including rejecting future lastFlaked dates. Use the existing
parsed field values and error collection to construct the valid entry.
---
Nitpick comments:
In `@apps/web/vitest.browser.quarantine.ts`:
- Around line 23-25: Update the validateQuarantineRegistry call in the browser
configuration load to pass validateSources: false alongside repositoryRoot,
preserving registry and schema validation while avoiding the redundant
repository scan.
In `@scripts/lib/quarantine-registry.test.ts`:
- Around line 50-157: Extend the “quarantine registry” tests to cover the
remaining validateQuarantineRegistry validation branches: duplicate ids, ids not
in sorted order, paths escaping repositoryRoot, missing or directory paths, and
markers absent from their declared source file. For each case, construct a
minimal invalid registry fixture and assert the corresponding validation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 016a58ed-7a74-4274-8f8d-ac843395e42d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
.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/src/browserManager.test.tsapps/desktop/src/browserManager.tsapps/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
Comment on lines
+657
to
+678
| quality: | ||
| name: quality | ||
| if: always() | ||
| needs: | ||
| - quality_linux | ||
| - quality_windows | ||
| - unit | ||
| - browser_windows | ||
| - e2e_linux | ||
| - e2e_windows | ||
| runs-on: ubuntu-24.04 | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - name: Require every quality dependency to succeed | ||
| run: | | ||
| test "${{ needs.quality_linux.result }}" = success | ||
| test "${{ needs.quality_windows.result }}" = success | ||
| test "${{ needs.unit.result }}" = success | ||
| test "${{ needs.browser_windows.result }}" = success | ||
| test "${{ needs.e2e_linux.result }}" = success | ||
| test "${{ needs.e2e_windows.result }}" = success | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Confirm macos_arm64 is intentionally excluded from the merge gate.
The aggregate gates quality_linux, quality_windows, unit, browser_windows, e2e_linux, and e2e_windows. windows_x64 is transitively enforced because e2e_windows needs it (a failed producer skips the consumer, which the = success check rejects). However, macos_arm64 has no downstream consumer and is absent from needs, so its failures (native build / packaged smoke) would not block merge. If macOS is meant to be a required lane, add it to the aggregate; otherwise confirm the intent.
🤖 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 @.github/workflows/ci.yml around lines 657 - 678, Update the quality
aggregate gate to explicitly include the macos_arm64 job if macOS is intended to
be required for merging: add it to needs and require its result to equal success
alongside the existing dependencies. If macos_arm64 is intentionally
non-blocking, leave the workflow unchanged and document or confirm that intent.
Comment on lines
+145
to
+240
| for (const [index, value] of document.entries.entries()) { | ||
| const label = `Quarantine entry ${index + 1}`; | ||
| if (!isRecord(value)) { | ||
| errors.push(`${label} must be a mapping.`); | ||
| continue; | ||
| } | ||
| for (const key of Object.keys(value)) { | ||
| if (!ENTRY_KEYS.has(key)) errors.push(`${label} has unsupported key \`${key}\`.`); | ||
| } | ||
|
|
||
| const id = value.id; | ||
| const path = value.path; | ||
| const marker = value.marker; | ||
| const suite = value.suite; | ||
| const platforms = value.platform; | ||
| const reason = value.reason; | ||
| const owner = value.owner; | ||
| const lastFlaked = value.lastFlaked; | ||
| const cases = value.cases; | ||
|
|
||
| if (typeof id !== "string" || !ID_PATTERN.test(id)) errors.push(`${label} id is invalid.`); | ||
| if (typeof id === "string") { | ||
| if (ids.has(id)) errors.push(`${label} duplicates id \`${id}\`.`); | ||
| ids.add(id); | ||
| } | ||
| if ( | ||
| typeof path !== "string" || | ||
| path.length === 0 || | ||
| path.includes("\\") || | ||
| path.startsWith("/") | ||
| ) { | ||
| errors.push(`${label} path must be a repository-relative POSIX path.`); | ||
| } | ||
| if (typeof marker !== "string" || typeof id !== "string" || marker !== `[quarantine:${id}]`) { | ||
| errors.push(`${label} marker must exactly match its id.`); | ||
| } | ||
| if (suite !== "browser-geometry") errors.push(`${label} suite is unsupported.`); | ||
| if (!Array.isArray(platforms) || platforms.length === 0 || !platforms.every(isPlatform)) { | ||
| errors.push(`${label} platform must contain supported platforms.`); | ||
| } else { | ||
| if (new Set(platforms).size !== platforms.length) errors.push(`${label} repeats a platform.`); | ||
| const sorted = [...platforms].sort( | ||
| (left, right) => PLATFORM_ORDER.get(left)! - PLATFORM_ORDER.get(right)!, | ||
| ); | ||
| if (sorted.some((platform, platformIndex) => platform !== platforms[platformIndex])) { | ||
| errors.push(`${label} platforms must use linux, windows order.`); | ||
| } | ||
| } | ||
| if (typeof reason !== "string" || reason.trim().length < 12) { | ||
| errors.push(`${label} reason must explain the quarantine.`); | ||
| } | ||
| if (typeof owner !== "string" || owner.trim().length === 0) { | ||
| errors.push(`${label} owner must be non-empty.`); | ||
| } | ||
| const lastFlakedDate = typeof lastFlaked === "string" ? parseIsoDate(lastFlaked) : null; | ||
| if (!lastFlakedDate) errors.push(`${label} lastFlaked must be a real YYYY-MM-DD date.`); | ||
| else if (lastFlakedDate > today) errors.push(`${label} lastFlaked cannot be in the future.`); | ||
| if (!Number.isInteger(cases) || (cases as number) < 1) { | ||
| errors.push(`${label} cases must be a positive integer.`); | ||
| } | ||
|
|
||
| if ( | ||
| typeof id === "string" && | ||
| ID_PATTERN.test(id) && | ||
| typeof path === "string" && | ||
| path.length > 0 && | ||
| !path.includes("\\") && | ||
| !path.startsWith("/") && | ||
| typeof marker === "string" && | ||
| marker === `[quarantine:${id}]` && | ||
| suite === "browser-geometry" && | ||
| Array.isArray(platforms) && | ||
| platforms.length > 0 && | ||
| platforms.every(isPlatform) && | ||
| typeof reason === "string" && | ||
| reason.trim().length >= 12 && | ||
| typeof owner === "string" && | ||
| owner.trim().length > 0 && | ||
| typeof lastFlaked === "string" && | ||
| lastFlakedDate && | ||
| Number.isInteger(cases) && | ||
| (cases as number) > 0 | ||
| ) { | ||
| entries.push({ | ||
| id, | ||
| path, | ||
| marker, | ||
| suite, | ||
| platform: platforms, | ||
| reason, | ||
| owner, | ||
| lastFlaked, | ||
| cases: cases as number, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate validation predicates for id/path/marker/suite/platform/reason/owner/lastFlaked/cases.
Lines 165-204 and 206-227 encode the same set of field checks twice — once to accumulate error messages, once (inverted/conjunctive) to gate the entries.push. They already drifted: the first pass additionally rejects a future lastFlaked (Line 201), but the second pass's gate only checks that the date parsed, not that it isn't in the future (Line 224). It's currently harmless only because the function nulls the whole registry whenever any error exists, but that's an easy invariant to break in a future edit.
Track whether the current entry produced any new errors and build the entry from that, instead of restating every predicate.
As per coding guidelines, **/*.{ts,tsx}: "avoid duplicating logic across files and prefer reusable modules over local shortcuts" — the same principle applies to duplicated logic within a file.
♻️ Proposed refactor to remove the duplicate predicate block
for (const [index, value] of document.entries.entries()) {
const label = `Quarantine entry ${index + 1}`;
if (!isRecord(value)) {
errors.push(`${label} must be a mapping.`);
continue;
}
for (const key of Object.keys(value)) {
if (!ENTRY_KEYS.has(key)) errors.push(`${label} has unsupported key \`${key}\`.`);
}
+ const errorsBeforeEntry = errors.length;
const id = value.id;
const path = value.path;
const marker = value.marker;
const suite = value.suite;
const platforms = value.platform;
const reason = value.reason;
const owner = value.owner;
const lastFlaked = value.lastFlaked;
const cases = value.cases;
if (typeof id !== "string" || !ID_PATTERN.test(id)) errors.push(`${label} id is invalid.`);
if (typeof id === "string") {
if (ids.has(id)) errors.push(`${label} duplicates id \`${id}\`.`);
ids.add(id);
}
// ...unchanged per-field checks (path, marker, suite, platform, reason, owner, lastFlaked, cases)...
- if (
- typeof id === "string" &&
- ID_PATTERN.test(id) &&
- typeof path === "string" &&
- path.length > 0 &&
- !path.includes("\\") &&
- !path.startsWith("/") &&
- typeof marker === "string" &&
- marker === `[quarantine:${id}]` &&
- suite === "browser-geometry" &&
- Array.isArray(platforms) &&
- platforms.length > 0 &&
- platforms.every(isPlatform) &&
- typeof reason === "string" &&
- reason.trim().length >= 12 &&
- typeof owner === "string" &&
- owner.trim().length > 0 &&
- typeof lastFlaked === "string" &&
- lastFlakedDate &&
- Number.isInteger(cases) &&
- (cases as number) > 0
- ) {
+ if (errors.length === errorsBeforeEntry) {
entries.push({
- id,
- path,
- marker,
- suite,
- platform: platforms,
- reason,
- owner,
- lastFlaked,
+ id: id as string,
+ path: path as string,
+ marker: marker as string,
+ suite: suite as QuarantineEntry["suite"],
+ platform: platforms as readonly QuarantinePlatform[],
+ reason: reason as string,
+ owner: owner as string,
+ lastFlaked: lastFlaked as string,
cases: cases as number,
});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const [index, value] of document.entries.entries()) { | |
| const label = `Quarantine entry ${index + 1}`; | |
| if (!isRecord(value)) { | |
| errors.push(`${label} must be a mapping.`); | |
| continue; | |
| } | |
| for (const key of Object.keys(value)) { | |
| if (!ENTRY_KEYS.has(key)) errors.push(`${label} has unsupported key \`${key}\`.`); | |
| } | |
| const id = value.id; | |
| const path = value.path; | |
| const marker = value.marker; | |
| const suite = value.suite; | |
| const platforms = value.platform; | |
| const reason = value.reason; | |
| const owner = value.owner; | |
| const lastFlaked = value.lastFlaked; | |
| const cases = value.cases; | |
| if (typeof id !== "string" || !ID_PATTERN.test(id)) errors.push(`${label} id is invalid.`); | |
| if (typeof id === "string") { | |
| if (ids.has(id)) errors.push(`${label} duplicates id \`${id}\`.`); | |
| ids.add(id); | |
| } | |
| if ( | |
| typeof path !== "string" || | |
| path.length === 0 || | |
| path.includes("\\") || | |
| path.startsWith("/") | |
| ) { | |
| errors.push(`${label} path must be a repository-relative POSIX path.`); | |
| } | |
| if (typeof marker !== "string" || typeof id !== "string" || marker !== `[quarantine:${id}]`) { | |
| errors.push(`${label} marker must exactly match its id.`); | |
| } | |
| if (suite !== "browser-geometry") errors.push(`${label} suite is unsupported.`); | |
| if (!Array.isArray(platforms) || platforms.length === 0 || !platforms.every(isPlatform)) { | |
| errors.push(`${label} platform must contain supported platforms.`); | |
| } else { | |
| if (new Set(platforms).size !== platforms.length) errors.push(`${label} repeats a platform.`); | |
| const sorted = [...platforms].sort( | |
| (left, right) => PLATFORM_ORDER.get(left)! - PLATFORM_ORDER.get(right)!, | |
| ); | |
| if (sorted.some((platform, platformIndex) => platform !== platforms[platformIndex])) { | |
| errors.push(`${label} platforms must use linux, windows order.`); | |
| } | |
| } | |
| if (typeof reason !== "string" || reason.trim().length < 12) { | |
| errors.push(`${label} reason must explain the quarantine.`); | |
| } | |
| if (typeof owner !== "string" || owner.trim().length === 0) { | |
| errors.push(`${label} owner must be non-empty.`); | |
| } | |
| const lastFlakedDate = typeof lastFlaked === "string" ? parseIsoDate(lastFlaked) : null; | |
| if (!lastFlakedDate) errors.push(`${label} lastFlaked must be a real YYYY-MM-DD date.`); | |
| else if (lastFlakedDate > today) errors.push(`${label} lastFlaked cannot be in the future.`); | |
| if (!Number.isInteger(cases) || (cases as number) < 1) { | |
| errors.push(`${label} cases must be a positive integer.`); | |
| } | |
| if ( | |
| typeof id === "string" && | |
| ID_PATTERN.test(id) && | |
| typeof path === "string" && | |
| path.length > 0 && | |
| !path.includes("\\") && | |
| !path.startsWith("/") && | |
| typeof marker === "string" && | |
| marker === `[quarantine:${id}]` && | |
| suite === "browser-geometry" && | |
| Array.isArray(platforms) && | |
| platforms.length > 0 && | |
| platforms.every(isPlatform) && | |
| typeof reason === "string" && | |
| reason.trim().length >= 12 && | |
| typeof owner === "string" && | |
| owner.trim().length > 0 && | |
| typeof lastFlaked === "string" && | |
| lastFlakedDate && | |
| Number.isInteger(cases) && | |
| (cases as number) > 0 | |
| ) { | |
| entries.push({ | |
| id, | |
| path, | |
| marker, | |
| suite, | |
| platform: platforms, | |
| reason, | |
| owner, | |
| lastFlaked, | |
| cases: cases as number, | |
| }); | |
| } | |
| } | |
| for (const [index, value] of document.entries.entries()) { | |
| const label = `Quarantine entry ${index + 1}`; | |
| if (!isRecord(value)) { | |
| errors.push(`${label} must be a mapping.`); | |
| continue; | |
| } | |
| for (const key of Object.keys(value)) { | |
| if (!ENTRY_KEYS.has(key)) errors.push(`${label} has unsupported key \`${key}\`.`); | |
| } | |
| const errorsBeforeEntry = errors.length; | |
| const id = value.id; | |
| const path = value.path; | |
| const marker = value.marker; | |
| const suite = value.suite; | |
| const platforms = value.platform; | |
| const reason = value.reason; | |
| const owner = value.owner; | |
| const lastFlaked = value.lastFlaked; | |
| const cases = value.cases; | |
| if (typeof id !== "string" || !ID_PATTERN.test(id)) errors.push(`${label} id is invalid.`); | |
| if (typeof id === "string") { | |
| if (ids.has(id)) errors.push(`${label} duplicates id \`${id}\`.`); | |
| ids.add(id); | |
| } | |
| if ( | |
| typeof path !== "string" || | |
| path.length === 0 || | |
| path.includes("\\") || | |
| path.startsWith("/") | |
| ) { | |
| errors.push(`${label} path must be a repository-relative POSIX path.`); | |
| } | |
| if (typeof marker !== "string" || typeof id !== "string" || marker !== `[quarantine:${id}]`) { | |
| errors.push(`${label} marker must exactly match its id.`); | |
| } | |
| if (suite !== "browser-geometry") errors.push(`${label} suite is unsupported.`); | |
| if (!Array.isArray(platforms) || platforms.length === 0 || !platforms.every(isPlatform)) { | |
| errors.push(`${label} platform must contain supported platforms.`); | |
| } else { | |
| if (new Set(platforms).size !== platforms.length) errors.push(`${label} repeats a platform.`); | |
| const sorted = [...platforms].sort( | |
| (left, right) => PLATFORM_ORDER.get(left)! - PLATFORM_ORDER.get(right)!, | |
| ); | |
| if (sorted.some((platform, platformIndex) => platform !== platforms[platformIndex])) { | |
| errors.push(`${label} platforms must use linux, windows order.`); | |
| } | |
| } | |
| if (typeof reason !== "string" || reason.trim().length < 12) { | |
| errors.push(`${label} reason must explain the quarantine.`); | |
| } | |
| if (typeof owner !== "string" || owner.trim().length === 0) { | |
| errors.push(`${label} owner must be non-empty.`); | |
| } | |
| const lastFlakedDate = typeof lastFlaked === "string" ? parseIsoDate(lastFlaked) : null; | |
| if (!lastFlakedDate) errors.push(`${label} lastFlaked must be a real YYYY-MM-DD date.`); | |
| else if (lastFlakedDate > today) errors.push(`${label} lastFlaked cannot be in the future.`); | |
| if (!Number.isInteger(cases) || (cases as number) < 1) { | |
| errors.push(`${label} cases must be a positive integer.`); | |
| } | |
| if (errors.length === errorsBeforeEntry) { | |
| entries.push({ | |
| id: id as string, | |
| path: path as string, | |
| marker: marker as string, | |
| suite: suite as QuarantineEntry["suite"], | |
| platform: platforms as readonly QuarantinePlatform[], | |
| reason: reason as string, | |
| owner: owner as string, | |
| lastFlaked: lastFlaked as string, | |
| cases: cases as number, | |
| }); | |
| } | |
| } |
🤖 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 `@scripts/lib/quarantine-registry.ts` around lines 145 - 240, Refactor the
validation loop around the entry field checks to record the error count before
validating each entry, then append the constructed entry only when no new errors
were added. Remove the duplicated conjunctive predicate guarding entries.push,
while preserving all existing validations, including rejecting future lastFlaked
dates. Use the existing parsed field values and error collection to construct
the valid entry.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 47 files
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/lib/workflow-contracts.ts">
<violation number="1" location="scripts/lib/workflow-contracts.ts:1667">
P2: An additional failure-masked command can bypass the nonblocking-test policy: `node scripts/quarantine-registry.ts run --platform linux && bun run ...` passes this prefix check, while failure from the appended command is ignored. Restrict this allowance to the two exact registered quarantine commands.</violation>
<violation number="2" location="scripts/lib/workflow-contracts.ts:1728">
P2: Runner validation now accepts `${{ matrix.runner }}` outside the unit job. A new CI job can therefore evade the approved-runner allowlist (or fail at runtime without a matrix) while the contract verifier succeeds; scope this exception to `jobs.unit.runs-on`.</violation>
</file>
<file name="apps/desktop/e2e/fixtures/electron-bootstrap.cjs">
<violation number="1" location="apps/desktop/e2e/fixtures/electron-bootstrap.cjs:67">
P2: The onCompleted handler throws if a completed request has an invalid URL, while the two sibling handlers safely catch that case. Wrap the URL parse in a try-catch consistent with the pattern used in onBeforeRequest and onErrorOccurred.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| for (const runner of runners) { | ||
| if (typeof runner !== "string" || !APPROVED_RUNNERS.has(runner)) { | ||
| const isCiUnitMatrixRunner = | ||
| policy.path === ".github/workflows/ci.yml" && runner === MATRIX_RUNNER_EXPRESSION; |
There was a problem hiding this comment.
P2: Runner validation now accepts ${{ matrix.runner }} outside the unit job. A new CI job can therefore evade the approved-runner allowlist (or fail at runtime without a matrix) while the contract verifier succeeds; scope this exception to jobs.unit.runs-on.
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 1728:
<comment>Runner validation now accepts `${{ matrix.runner }}` outside the unit job. A new CI job can therefore evade the approved-runner allowlist (or fail at runtime without a matrix) while the contract verifier succeeds; scope this exception to `jobs.unit.runs-on`.</comment>
<file context>
@@ -948,7 +1724,9 @@ function validateAllowedWorkflow(
for (const runner of runners) {
- if (typeof runner !== "string" || !APPROVED_RUNNERS.has(runner)) {
+ const isCiUnitMatrixRunner =
+ policy.path === ".github/workflows/ci.yml" && runner === MATRIX_RUNNER_EXPRESSION;
+ if (typeof runner !== "string" || (!APPROVED_RUNNERS.has(runner) && !isCiUnitMatrixRunner)) {
errors.push(`${policy.path} references unsupported runner ${String(runner)}.`);
</file context>
| const allowed = | ||
| step["continue-on-error"] === true && | ||
| typeof step.run === "string" && | ||
| normalizeShellCommand(step.run).startsWith("node scripts/quarantine-registry.ts run "); |
There was a problem hiding this comment.
P2: An additional failure-masked command can bypass the nonblocking-test policy: node scripts/quarantine-registry.ts run --platform linux && bun run ... passes this prefix check, while failure from the appended command is ignored. Restrict this allowance to the two exact registered quarantine commands.
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 1667:
<comment>An additional failure-masked command can bypass the nonblocking-test policy: `node scripts/quarantine-registry.ts run --platform linux && bun run ...` passes this prefix check, while failure from the appended command is ignored. Restrict this allowance to the two exact registered quarantine commands.</comment>
<file context>
@@ -896,6 +1656,22 @@ function validateCiArchitecture(workflow: UnknownRecord, errors: string[]): void
+ const allowed =
+ step["continue-on-error"] === true &&
+ typeof step.run === "string" &&
+ normalizeShellCommand(step.run).startsWith("node scripts/quarantine-registry.ts run ");
+ if (!allowed && step["continue-on-error"] !== false) {
+ errors.push(
</file context>
Suggested change
| normalizeShellCommand(step.run).startsWith("node scripts/quarantine-registry.ts run "); | |
| [ | |
| "node scripts/quarantine-registry.ts run --platform linux", | |
| "node scripts/quarantine-registry.ts run --platform windows", | |
| ].includes(normalizeShellCommand(step.run)); |
| callback({ cancel: true }); | ||
| }); | ||
| guardedSession.webRequest.onCompleted({ urls: ["<all_urls>"] }, (details) => { | ||
| const parsed = new URL(details.url); |
There was a problem hiding this comment.
P2: The onCompleted handler throws if a completed request has an invalid URL, while the two sibling handlers safely catch that case. Wrap the URL parse in a try-catch consistent with the pattern used in onBeforeRequest and onErrorOccurred.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/e2e/fixtures/electron-bootstrap.cjs, line 67:
<comment>The onCompleted handler throws if a completed request has an invalid URL, while the two sibling handlers safely catch that case. Wrap the URL parse in a try-catch consistent with the pattern used in onBeforeRequest and onErrorOccurred.</comment>
<file context>
@@ -0,0 +1,148 @@
+ callback({ cancel: true });
+ });
+ guardedSession.webRequest.onCompleted({ urls: ["<all_urls>"] }, (details) => {
+ const parsed = new URL(details.url);
+ if (NETWORK_PROTOCOLS.has(parsed.protocol) && isLoopbackHost(parsed.hostname)) {
+ recordNetworkEvent({
</file context>
Suggested change
| const parsed = new URL(details.url); | |
| let parsed; | |
| try { | |
| parsed = new URL(details.url); | |
| } catch { | |
| return; | |
| } |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
slashdevcorpse
marked this pull request as draft
July 22, 2026 15:32
8 tasks
Owner
Author
|
Closing this stale-base prototype without merge so it cannot race the Windows .9 release. The exact head is not qualified (required quality failed; Windows E2E 5/7; Linux E2E 0/7; Windows/macOS unit failures; five unresolved threads). Product browser lifecycle work remains in #123, installed NSIS qualification in #11, production process ownership in #7, and the reusable Windows-first test/CI scope is tracked in #133. The .9 installer timeout is outside this diff. |
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.
Summary
Scope boundary
This intentionally does not bring back legacy provider lifecycle or broad process-ownership changes. Production behavior changes are limited to the adopted-webview target preservation fix; reconnect waiting is isolated to the desktop E2E harness.
Verification
TURBO_CONCURRENCY=1 bun turbo teston Windows: 9/9 package tasks greenbun run build:desktop: passedbun run test:e2e: 7/7 passed twice on the packaged Windows artifact (6.9m and 6.4m)git diff --check: passedHosted CI remains the authoritative
fmt,lint,typecheck, cross-platform unit, browser, packaged E2E, release smoke, dependency review, and CodeQL gate.Summary by cubic
Hardened CI and E2E: added cross-platform unit and browser lanes with a reviewed quarantine gate, plus packaged Electron journeys on Linux and Windows. Also fixed a desktop navigation edge case to prevent transient target loss during adopted webview loads.
New Features
.github/quarantine.ymlwith validation and summaries; Vitest configs select tests via registered[quarantine:<id>]markers.window.__synaraE2eprobe that requires a stable, open transport session and waits through reconnect windows.@playwright/test.Bug Fixes
Written for commit 84cdefe. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Quality Improvements