Skip to content

Fix: early exit on JS setup failures#1931

Merged
Saga4 merged 2 commits intomainfrom
fix/js-setup-early-exit
Apr 1, 2026
Merged

Fix: early exit on JS setup failures#1931
Saga4 merged 2 commits intomainfrom
fix/js-setup-early-exit

Conversation

@mohammedahmed18
Copy link
Copy Markdown
Contributor

@mohammedahmed18 mohammedahmed18 commented Mar 31, 2026

image

Summary

  • setup_test_config now returns bool — optimizer aborts when JS requirements (Node, npm, test framework) are missing instead of silently continuing
  • Adds SetupError dataclass to languages/base.py for structured error reporting
  • Fixes warning logger to print error.message instead of the SetupError repr
  • Adds error-level log at the optimizer call site so users see why optimization was aborted
  • Updates all test assertions to work with SetupError objects

Test plan

  • All 11 tests in test_javascript_requirements.py pass
  • prek (ruff check + ruff format) passes

🤖 Generated with Claude Code

setup_test_config now returns bool so the optimizer aborts when JS
requirements (Node, npm, test framework) are missing instead of
silently continuing. Adds SetupError dataclass, fixes warning log to
print messages, and updates tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 31, 2026

Claude finished @mohammedahmed18's task in 3m 39s —— View job


PR Review Summary

Tasks

  • Triage PR size (LARGE: ~76 production lines added)
  • Run lint/typecheck (prek + mypy)
  • Resolve stale threads (none found)
  • Code review
  • Duplicate detection
  • Optimization PRs handled

Prek Checks

prek run --from-ref origin/mainpassed (ruff check + ruff format clean)

Mypy has pre-existing errors in optimizer.py and javascript/support.py unrelated to this PR. One new mypy error introduced by this PR in javascript/optimizer.py:54:

Returning Any from function declared to return "list[SetupError]"  [no-any-return]

This is because js_support.verify_requirements(...) is called via LanguageSupport protocol which doesn't declare verify_requirements, so it resolves to Any. Not blocking, but worth noting.


Code Review

Logic is correct overall. The early-exit behavior is properly wired through the call chain and all language implementations are updated consistently.

Issue 1 — Misleading docstring (minor) [codeflash/languages/javascript/optimizer.py]

The existing docstring line "Logs warnings if requirements are not met but does not abort." is now stale — the function has changed so that callers can abort based on its return value. The added Returns: line doesn't remove this contradiction:

"""Verify JavaScript/TypeScript requirements before optimization.

Checks that Node.js, npm, and the test framework are available.
Logs warnings if requirements are not met but does not abort.  ← stale

Returns: List of setup errors if requirements are not met, empty list otherwise.
"""

Fix this →

Issue 2 — should_abort=False is never used (design) [codeflash/languages/base.py, codeflash/languages/javascript/support.py]

Every SetupError created in this PR uses should_abort=True. The should_abort field has no False usage anywhere, making the abort check any(e.should_abort for e in setup_errors) equivalent to bool(setup_errors). Either:

  • Document a concrete planned use case for should_abort=False (non-fatal warnings that continue optimization), or
  • Simplify: remove the field and treat any returned error as abort-worthy

Not a blocker, but the field adds cognitive overhead without current benefit.


Duplicate Detection

No duplicates detected. The SetupError type is defined once in base.py and used from there.


Test Coverage

New TestSetupTestConfig class with 3 tests covers the new early-exit paths. Existing 11 tests updated correctly to use error.message instead of raw string comparison.


Optimization PRs

PR #1932 (JavaSupport.setup_test_config — 171% speedup) — adds a _config_cache dict to avoid repeated detect_java_project file I/O calls. Optimization is correct and credible. CI is mostly passing (one Snyk limit failure is pre-existing/unrelated, several checks still pending). Leaving open to merge once all CI checks complete.


@mohammedahmed18 mohammedahmed18 changed the title feat: early exit on JS setup failures Fix: early exit on JS setup failures Mar 31, 2026
@codeflash-ai
Copy link
Copy Markdown
Contributor

codeflash-ai bot commented Mar 31, 2026

⚡️ Codeflash found optimizations for this PR

📄 171% (1.71x) speedup for JavaSupport.setup_test_config in codeflash/languages/java/support.py

⏱️ Runtime : 24.8 milliseconds 9.15 milliseconds (best of 14 runs)

A dependent PR with the suggested changes has been created. Please review:

If you approve, it will be merged into this PR (branch fix/js-setup-early-exit).

Static Badge

@Saga4 Saga4 merged commit 3c38a80 into main Apr 1, 2026
28 of 31 checks passed
@Saga4 Saga4 deleted the fix/js-setup-early-exit branch April 1, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants