Unblock CI on main

[benceruleanlu]

Add .fail to a known failing test to unblock CI on main

Reasoning

The behavior changed and this test's expectation was never updated. We automatically install or upgrade packages when needed, and so this test was trying to find the modal in the troubleshooting page where it said install Python packages, but the packages were already installed.

This is a temporary fix

Summary by CodeRabbit

• Tests
  • Marked a virtual-environment troubleshooting test as "fixme" (temporarily disabled) to reflect current expected behavior; test steps and assertions unchanged.

[coderabbitai]

📝 Walkthrough

Walkthrough

A single integration test in the troubleshooting virtual-environment suite was changed from a normal test to a fixme-marked test, preserving the original test body and control flow while altering its execution status.

Changes

Cohort / File(s)	Summary
Test Expectation Update tests/integration/post-install/troubleshootingVenv.spec.ts	Replaced test(...) with test.fixme(...) for the "Can fix venv" test, leaving the test body, assertions, timing, and waits unchanged.

Poem

🐰 I hopped to the test with a carrot and grin,
Marked a little failing where once was a win,
Troubleshooting burrows where code-flowers dwell,
I nudge it to rest with a soft little bell,
Tomorrow we’ll try — for now, dream and dwell.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Unblock CI on main' is vague and does not clearly convey the specific change made in the PR. While it relates to the objective, it lacks specificity about what the actual code change is (marking a test as fixme/fail).	Consider a more specific title such as 'Mark venv troubleshooting test as fixme to unblock CI' or 'Skip failing venv test temporarily' to better describe the actual code change being made.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/venv-troubleshooting-upgrade

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR changes the error handling behavior for virtual environment requirements validation. When requirements are missing or incomplete and don't match known upgrade patterns, the code now returns 'error' instead of 'package-upgrade'.

Changes:

• Modified return value from 'package-upgrade' to 'error' for non-OK requirements that don't match known upgrade patterns
• Updated log message and level (info → warn) to reflect the new error classification

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This change modifies the behavior in comfyInstallation.ts where 'package-upgrade' is treated as OK with a warning (lines 168-170). After this change, cases that previously returned 'package-upgrade' will now return 'error', which will be treated as a genuine error (lines 172-173) instead of OK with a warning.

This is a breaking behavioral change that affects the validation flow. Previously, unknown missing packages were tolerated as a warning; now they will be treated as errors. Consider whether this is the intended behavior and ensure it's properly documented and that any downstream consumers are updated accordingly.

[Copilot]

The log message "Requirements are missing or out of date. Treating as error." is potentially misleading. This code path is reached when requirements are NOT OK AND do not match known upgrade patterns. It's not necessarily about being "out of date" - it could be truly missing packages or unexpected package changes.

Consider clarifying the log message to be more precise, such as "Requirements check failed with unknown or unexpected package changes. Treating as error." This would better distinguish it from the known upgrade cases that are handled at lines 966-968.

Suggested change

	log.warn('Requirements are missing or out of date. Treating as error.', {
	log.warn('Requirements check failed with unknown or unexpected package changes. Treating as error.', {

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 024652d33f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/virtualEnvironment.ts (1)

883-889: ⚠️ Potential issue | 🟠 Major

Update JSDoc and fix call site logic to distinguish 'package-upgrade' from 'error'.

The JSDoc still references the outdated 'manager-upgrade' value instead of the current 'package-upgrade' return type. Additionally, the call site at line 324 incorrectly treats 'package-upgrade' (known safe upgrades) the same as 'error' (unexpected missing packages) by calling manualInstall() for both. Only 'error' should trigger a manual install; 'package-upgrade' should allow the application to proceed (similar to the comfyInstallation.ts:167 call site behavior).

Update the JSDoc to accurately reflect the three return values and their semantics, and fix the logic at line 326 to handle 'package-upgrade' and 'error' separately:

Suggested changes

1. Lines 887-889 — Update JSDoc:

-   * `@returns` `'OK'` if pip install does not detect any missing packages,
-   * `'manager-upgrade'` if `uv` and `toml` are missing,
-   * or `'error'` when any other combination of packages are missing.
+   * `@returns` `'OK'` if all required packages are installed and up to date,
+   * `'package-upgrade'` if missing packages match a known safe upgrade pattern,
+   * or `'error'` for any other missing or unexpected package combination.

2. Lines 326-331 — Handle 'package-upgrade' separately from 'error':

         if (requirementsStatus === 'OK') {
           log.info('Skipping requirements installation - all requirements already installed');
+        } else if (requirementsStatus === 'package-upgrade') {
+          log.info('Known package upgrade detected - proceeding with installation');
-        } else {
+        } else if (requirementsStatus === 'error') {
           log.info('Starting manual install - venv missing requirements');
           await this.manualInstall(callbacks);
         }

[benceruleanlu]

Addressed review feedback: removed allow‑list gating to preserve auto‑update for any mismatch, reset now installs requirements, tests updated, docs/logs aligned.

[luke-mino-altherr]

Can you add a blurb in the PR / merge commit body to explain:

1. why we think this test is failing
2. explaining we're merging this as a temporary measure to unblock CI

I'm also curious when this started popping up and whether its a real regression...