feat: Add StorageFactory and VariableFactory with service registration

[dkaushik94]

Added StorageFactory and VariableFactory. Registered StorageService and VariableService to the LFX service discovery routine to return service objects, and not None. Internal method calls still call noop, but avoid disgraceful errors such as AttributeError(s).

Summary by CodeRabbit

Release Notes

• New Features

  • Storage and variable services are now available and registered by default, enabling file persistence and environment variable management without additional configuration.

• Tests

  • Added comprehensive unit tests to verify service factories and their core functionality.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a3bd77a5-835d-4089-bfba-e1eb6f94e043

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

Two new service factory classes, StorageServiceFactory and VariableServiceFactory, are added for the bare lfx package. Both are registered via explicit register_factory calls inside the updated initialize_services(). ServiceType enum entries are reordered in schema.py. A new test module verifies factory metadata, service resolution, file I/O, and custom factory override behavior.

Changes

Default Storage and Variable Factories

Layer / File(s)	Summary
StorageServiceFactory and VariableServiceFactory implementations src/lfx/src/lfx/services/schema.py, src/lfx/src/lfx/services/storage/factory.py, src/lfx/src/lfx/services/variable/factory.py	StorageServiceFactory sets LocalStorageService as default with a dependency on SETTINGS_SERVICE and constructs it with session_service=None. VariableServiceFactory declares no dependencies and returns a fresh VariableService. ServiceType enum entries are reordered without value changes.
Factory registration in initialize_services() src/lfx/src/lfx/services/initialize.py	Imports and registers StorageServiceFactory and VariableServiceFactory via explicit register_factory calls alongside SettingsServiceFactory, with updated comments explaining these services do not self-register at import time.
Unit tests for default factories src/lfx/tests/unit/services/test_default_factories.py	Adds a fresh_service_manager fixture and four tests: factory metadata assertions, non-None service resolution, async file round-trip via save_file/get_file against a temp directory, and custom factory override asserting the resolved service is not LocalStorageService.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (8 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding StorageFactory and VariableFactory classes and registering the corresponding services. It is concise, specific, and directly reflects the primary objective of the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Test Coverage For New Implementations	✅ Passed	PR includes comprehensive test file (test_default_factories.py) with 4 meaningful test functions covering factory metadata, service instantiation, file I/O, and override mechanisms. Tests follow na...
Test Quality And Coverage	✅ Passed	Tests properly verify factories register, return non-None services, use correct async patterns, validate integration, and test overridability. While not comprehensive for all service methods, they...
Test File Naming And Structure	✅ Passed	Test file test_default_factories.py follows proper naming (test_*.py), pytest structure, has descriptive function names, proper setup/teardown with fixture, logical organization, and covers positiv...
Excessive Mock Usage Warning	✅ Passed	Tests use zero mocks from mock library; all tests instantiate and exercise real objects (factories, services, file I/O) with appropriate fixture setup for test isolation.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch le-1464-register-services-lfx

---

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.}

[github-actions]

✅ Test Coverage Advisor

No source changes detected without accompanying tests. Thanks for keeping coverage up! 🎉

Advisory check only — never blocks merge.

[github-actions]

Frontend Unit Test Coverage Report

Coverage Summary

Lines	Statements	Branches	Functions
	43.39% (57786/133158)	69.06% (7842/11355)	41.64% (1299/3119)

Unit Test Results

Tests	Skipped	Failures	Errors	Time
4945	0 💤	0 ❌	0 🔥	15m 3s ⏱️

[codecov]

Codecov Report

❌ Patch coverage is 95.08197% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.85%. Comparing base (2b7b113) to head (f00016e).

Files with missing lines	Patch %	Lines
src/lfx/src/lfx/schema/image.py	80.00%	3 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##           release-1.11.0   #13728      +/-   ##
==================================================
+ Coverage           58.71%   58.85%   +0.14%     
==================================================
  Files                2309     2311       +2     
  Lines              220388   221161     +773     
  Branches            31204    34248    +3044     
==================================================
+ Hits               129391   130164     +773     
- Misses              89528    89529       +1     
+ Partials             1469     1468       -1     

Flag	Coverage Δ
backend	65.81% <ø> (+0.03%)	⬆️
frontend	58.08% <ø> (+0.18%)	⬆️
lfx	54.75% <95.08%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/lfx/src/lfx/services/initialize.py	100.00% <100.00%> (+100.00%)	⬆️
src/lfx/src/lfx/services/schema.py	100.00% <100.00%> (ø)
src/lfx/src/lfx/services/storage/factory.py	100.00% <100.00%> (ø)
src/lfx/src/lfx/services/variable/factory.py	100.00% <100.00%> (ø)
src/lfx/src/lfx/utils/image.py	73.52% <100.00%> (ø)
src/lfx/src/lfx/schema/image.py	44.60% <80.00%> (-10.24%)	⬇️

... and 103 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ogabrielluiz]

Hey @dkaushik94,

I think the Python 3.14 failure is a real regression, not a flake. Registering a default storage service flips the get_storage_service() is None checks that a few places use to mean "no backend, read the file straight off disk." lfx/schema/image.py is the one failing: before this, bare lfx had no storage service, so an absolute path like /tmp/foo.png took the direct-read branch. Now storage is always registered, so the same path goes through storage_service.parse_file_path() and gets rejected because the flow_id ends up with slashes in it.

Repro on the branch:

initialize_services()
get_files(["/var/.../tmp.png"])   # FileNotFoundError: Invalid flow_id: contains path separators

It only shows up on the full suite because something registers storage globally before the schema tests run, so running test_image.py on its own passes. That's why it looks like a 3.14 flake.

I think the fix belongs here, since the registration is what changes the contract. Could the file reading decide local vs storage by the shape of the path (an absolute local path vs a flow_id/filename key) instead of by whether a storage service exists? image.py:104 and :140 are the spots.

The same service is None pattern shows up at the variable-service call sites too (unified_models/credentials.py, model_utils.py, build_config.py, connector_base.py) and in graph/vertex/param_handler.py. I didn't confirm those break, but they're the same shape, worth checking before this lands. base/data/storage_utils.py looks fine, it keys off storage_type == "s3".

What do you think?

[dkaushik94]

Hey @dkaushik94,

I think the Python 3.14 failure is a real regression, not a flake. Registering a default storage service flips the get_storage_service() is None checks that a few places use to mean "no backend, read the file straight off disk." lfx/schema/image.py is the one failing: before this, bare lfx had no storage service, so an absolute path like /tmp/foo.png took the direct-read branch. Now storage is always registered, so the same path goes through storage_service.parse_file_path() and gets rejected because the flow_id ends up with slashes in it.

Repro on the branch:

initialize_services()
get_files(["/var/.../tmp.png"])   # FileNotFoundError: Invalid flow_id: contains path separators

It only shows up on the full suite because something registers storage globally before the schema tests run, so running test_image.py on its own passes. That's why it looks like a 3.14 flake.

I think the fix belongs here, since the registration is what changes the contract. Could the file reading decide local vs storage by the shape of the path (an absolute local path vs a flow_id/filename key) instead of by whether a storage service exists? image.py:104 and :140 are the spots.

The same service is None pattern shows up at the variable-service call sites too (unified_models/credentials.py, model_utils.py, build_config.py, connector_base.py) and in graph/vertex/param_handler.py. I didn't confirm those break, but they're the same shape, worth checking before this lands. base/data/storage_utils.py looks fine, it keys off storage_type == "s3".

What do you think?

Yeah, this is a failing test. I was looking into it. Thanks for the pointers, I'll fix and push a clean build.

[Jkavia]

One small thing: the storage service has nice validation guards to block path traversal (like ../etc or null bytes in the flow_id). But the test suite doesn't actually verify those guards work when the factory creates the service.

Not a blocker at all, just a thought: a test like test_storage_rejects_traversal() that tries to write to ../etc and confirms it fails would be good insurance against future regressions. But if you want to keep this PR scoped to just factory registration, totally fine to punt on that.