Skip to content

feat: Langflow SDK and Flow DevOps API Toolkit#12245

Open
erichare wants to merge 8 commits intorelease-1.9.0from
feature/flow-devops-toolkit
Open

feat: Langflow SDK and Flow DevOps API Toolkit#12245
erichare wants to merge 8 commits intorelease-1.9.0from
feature/flow-devops-toolkit

Conversation

@erichare
Copy link
Copy Markdown
Collaborator

@erichare erichare commented Mar 18, 2026

Flow DevOps Toolkit (lfx CLI)

This PR expands the capabilities of lfx, a command-line toolkit for managing Langflow flows as code — enabling a proper dev → version control → deploy lifecycle outside the UI.

What's included

New lfx CLI (src/lfx) with the following commands, grouped by lifecycle stage:

Group Commands
Setup init, login
Authoring create, requirements, validate
Running run, serve
Remote status, push, pull, export

Key capabilities:

  • lfx init — scaffolds a local project with a flows/ directory and config
  • lfx create — creates a new flow from a template (e.g. hello-world)
  • lfx validate — validates one or all flows in flows/ against the Langflow schema
  • lfx push / pull — syncs flows between local disk and a running Langflow server
  • lfx status — shows ahead/behind state per flow, using both hash and timestamp so local-vs-remote modifications are correctly distinguished (not just "changed")
  • lfx run / serve — runs a flow directly or starts a local dev server

Supporting infrastructure:

  • langflow-sdk (src/sdk) — a thin Python client wrapping the Langflow REST API, used by both lfx and backend components
  • Docker fixes — both build_and_push.Dockerfile and build_and_push_backend.Dockerfile now correctly include src/sdk so lfx's dependency on langflow-sdk resolves at build time
  • Unit test coverage for init, login, create, push, pull, status, and export commands

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 18, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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: 407ab31f-9b9b-4e4f-9d85-2c6a6f900f6d

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
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/flow-devops-toolkit

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

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

@erichare erichare changed the base branch from release-1.9.0 to feat-unify-models++ March 18, 2026 19:15
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 18, 2026

Frontend Unit Test Coverage Report

Coverage Summary

Lines Statements Branches Functions
Coverage: 26%
26.68% (27018/101229) 63.32% (3345/5282) 28.82% (639/2217)

Unit Test Results

Tests Skipped Failures Errors Time
2784 0 💤 0 ❌ 0 🔥 3m 54s ⏱️

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 18, 2026

Codecov Report

❌ Patch coverage is 75.58040% with 568 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.58%. Comparing base (1f1fb20) to head (fac2806).

Files with missing lines Patch % Lines
src/backend/base/langflow/api/v1/flows_helpers.py 65.25% 82 Missing ⚠️
src/lfx/src/lfx/testing/runners.py 36.50% 79 Missing and 1 partial ⚠️
src/lfx/src/lfx/testing/plugin.py 29.34% 61 Missing and 4 partials ⚠️
src/backend/base/langflow/api/v1/projects_files.py 46.01% 61 Missing ⚠️
src/lfx/src/lfx/cli/validation/semantic.py 71.30% 23 Missing and 10 partials ⚠️
src/lfx/src/lfx/testing/result.py 34.00% 32 Missing and 1 partial ⚠️
src/backend/base/langflow/api/v1/flows.py 50.00% 29 Missing ⚠️
src/backend/base/langflow/api/utils/flow_utils.py 72.94% 23 Missing ⚠️
src/backend/base/langflow/api/v1/projects.py 28.12% 23 Missing ⚠️
src/lfx/src/lfx/cli/login.py 81.89% 14 Missing and 7 partials ⚠️
... and 14 more
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                @@
##           release-1.9.0   #12245      +/-   ##
=================================================
+ Coverage          48.24%   48.58%   +0.34%     
=================================================
  Files               1860     1883      +23     
  Lines             162995   164750    +1755     
  Branches           22429    22742     +313     
=================================================
+ Hits               78630    80042    +1412     
- Misses             83482    83777     +295     
- Partials             883      931      +48     
Flag Coverage Δ
backend 54.36% <63.25%> (+0.43%) ⬆️
frontend 47.60% <ø> (-0.07%) ⬇️
lfx 46.98% <80.50%> (+2.12%) ⬆️

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

Files with missing lines Coverage Δ
src/lfx/src/lfx/__main__.py 78.57% <100.00%> (-6.80%) ⬇️
src/lfx/src/lfx/cli/_running_commands.py 100.00% <100.00%> (ø)
src/lfx/src/lfx/cli/create.py 100.00% <100.00%> (ø)
src/lfx/src/lfx/cli/export.py 100.00% <100.00%> (ø)
src/lfx/src/lfx/cli/init.py 100.00% <100.00%> (ø)
src/lfx/src/lfx/cli/validate.py 100.00% <100.00%> (ø)
src/lfx/src/lfx/cli/validation/_env_validation.py 100.00% <ø> (ø)
src/backend/base/langflow/initial_setup/setup.py 52.70% <50.00%> (ø)
src/backend/base/langflow/api/utils/core.py 73.39% <95.91%> (+5.94%) ⬆️
src/lfx/src/lfx/cli/_setup_commands.py 81.81% <81.81%> (ø)
... and 21 more

... and 29 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.

@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Mar 18, 2026
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Mar 19, 2026
@erichare erichare added the DO NOT MERGE Don't Merge this PR label Mar 19, 2026
@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Mar 19, 2026
@erichare erichare requested a review from Copilot March 19, 2026 21:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new langflow-sdk Python package for interacting with the Langflow REST API, expands the lfx CLI into a “Flow DevOps Toolkit” (init/status/pull/push/export/login/validate + environment resolution), and updates backend import/export behavior to preserve stable flow IDs and produce git-friendly exports.

Changes:

  • Add the langflow-sdk package (client-facing models, environments config, serialization helpers, pytest plugin, and unit tests).
  • Add lfx environment discovery/parsing (YAML/TOML) and new CLI commands plus CI template artifacts.
  • Update backend flow import/export to support stable IDs on upload and deterministic, git-friendly flow downloads.

Reviewed changes

Copilot reviewed 55 out of 59 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
uv.lock Add langflow-sdk workspace member and update dependency lock (incl. pyyaml).
src/lfx/tests/unit/config/test_environments.py New unit tests for lfx.config.environments resolution and error cases.
src/lfx/tests/unit/config/init.py Unit test package init (new).
src/lfx/tests/unit/cli/test_serve_simple.py Mark serve CLI tests skipped in CI.
src/lfx/templates/README.md New documentation for CI/CD templates usage.
src/lfx/src/lfx/templates/shell/ci-validate.sh New shell script template for static flow validation.
src/lfx/src/lfx/templates/shell/ci-test.sh New shell script template for running integration tests with flow_runner.
src/lfx/src/lfx/templates/shell/ci-push.sh New shell script template for pushing flows to a remote instance.
src/lfx/src/lfx/templates/gitlab-ci/langflow.yml New GitLab CI template for validate/test/deploy stages.
src/lfx/src/lfx/templates/github-actions/langflow-validate.yml New GitHub Actions template to validate flows in PRs.
src/lfx/src/lfx/templates/github-actions/langflow-test.yml New GitHub Actions template to run flow integration tests.
src/lfx/src/lfx/templates/github-actions/langflow-push.yml New GitHub Actions template to deploy flows on merge.
src/lfx/src/lfx/config/environments.py New lfx environment discovery + YAML/TOML parsing + env var fallbacks.
src/lfx/src/lfx/config/init.py Export resolve_environment and types for CLI commands.
src/lfx/src/lfx/cli/status.py New lfx status command comparing local flows vs remote.
src/lfx/src/lfx/cli/push.py New lfx push command with upsert-by-stable-ID and project targeting.
src/lfx/src/lfx/cli/pull.py New lfx pull command writing normalized flows to disk.
src/lfx/src/lfx/cli/login.py New lfx login command to validate connectivity/auth and guide setup.
src/lfx/src/lfx/cli/init.py New lfx init scaffolding for flows/tests/config + CI templates.
src/lfx/src/lfx/cli/export.py New lfx export for local normalization and remote export.
src/lfx/src/lfx/main.py Wire up new CLI commands and options.
src/lfx/pyproject.toml Add pyyaml dependency and register pytest plugin entry point.
src/langflow-sdk/tests/test_testing.py Tests for SDK pytest plugin fixtures and helpers (mocked HTTP).
src/langflow-sdk/tests/test_streaming.py Tests for streaming chunk model + client stream/run behavior.
src/langflow-sdk/tests/test_serialization.py Tests for normalization/serialization helpers.
src/langflow-sdk/tests/test_push.py Tests for upsert/push flows behavior with mocked HTTP.
src/langflow-sdk/tests/test_models.py Tests for Pydantic models and TOML environments config.
src/langflow-sdk/tests/test_file_io.py Tests for file-based push/pull helpers (sync + async).
src/langflow-sdk/tests/test_client_aliases.py Tests for short-name client aliases.
src/langflow-sdk/tests/init.py Test package init (new).
src/langflow-sdk/src/langflow_sdk/testing.py Pytest plugin providing flow_runner / async_flow_runner fixtures.
src/langflow-sdk/src/langflow_sdk/serialization.py Deterministic git-friendly flow normalization + JSON serialization.
src/langflow-sdk/src/langflow_sdk/models.py Pydantic API models (flows/projects/run/streaming).
src/langflow-sdk/src/langflow_sdk/exceptions.py SDK exception hierarchy.
src/langflow-sdk/src/langflow_sdk/environments.py TOML environments loader + client factories.
src/langflow-sdk/src/langflow_sdk/_version.py SDK version constant.
src/langflow-sdk/src/langflow_sdk/init.py Public SDK exports.
src/langflow-sdk/pyproject.toml Package metadata, deps, pytest config, and pytest entry point.
src/langflow-sdk/langflow-environments.toml.example Example TOML environments file for users/CI.
src/backend/tests/unit/test_database.py Update model creation path to avoid passing id=None into DB model validation.
src/backend/tests/unit/api/v1/test_projects.py Same adjustment for project-related tests.
src/backend/tests/unit/api/v1/test_files.py Same adjustment for file upload tests.
src/backend/tests/unit/api/utils/test_preserve_flow_id.py New tests for stable flow ID preservation/upsert branching.
src/backend/tests/unit/api/utils/test_export_normalisation.py New tests for export normalization + code list/string round-trip.
src/backend/tests/conftest.py Adjust test fixtures to exclude optional id when validating DB model.
src/backend/base/langflow/services/database/models/flow/model.py Make FlowCreate.id optional to support stable IDs on import/upload.
src/backend/base/langflow/initial_setup/setup.py Adjust creation path to avoid passing id=None into DB model validation.
src/backend/base/langflow/api/v1/projects.py Normalize exported flows, serialize deterministically, and normalize code on upload.
src/backend/base/langflow/api/v1/flows.py Add upsert semantics on upload with stable IDs; normalize download output.
src/backend/base/langflow/api/utils/core.py Add export normalization and import code re-joining helpers.
src/backend/base/langflow/api/utils/init.py Re-export new normalization helpers.
pyproject.toml Add workspace member and per-file Ruff ignores for new modules/tests.

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

@github-actions github-actions bot added enhancement New feature or request and removed enhancement New feature or request labels Mar 19, 2026
@erichare
Copy link
Copy Markdown
Collaborator Author

@ogabrielluiz this is marked as DO NOT MERGE as its still a WIP, but this is what I have so far on the DevOps API (lfx validate, push, pull, all that). There's plenty of TODOs remaining:

  1. Finalize naming, location within the repo, etc
  2. There are dependencies on some unmerged PRs at the API level (doesn't have to block the work on this, but will block the functionality from working) - you nicely listed many of them out in the epic ticket
  3. There also is at least one PR that has some of this work already, so i'll want to reconcile it to see what if any can be reused - its just hard to work on any one piece of this without getting a framework for all of it

Again, this is very early, but because of the scope i wanted to get this in early, especially if we're trying to get it into 1.9. The PR is up to date with the 1.9 release branch with conflicts handled so that's a good start. I think at a high level it has most of the functionality of the epic, but just with tons and tons of details to square away. I tried to match as close as possible to the tickets and completion criteria though :)

@erichare
Copy link
Copy Markdown
Collaborator Author

Screenshot 2026-03-22 at 10 57 57 AM

This comment was marked as off-topic.

Copy link
Copy Markdown
Member

@Cristhianzl Cristhianzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executive Summary

This PR introduces a Flow DevOps CLI (lfx) and a Python SDK (langflow-sdk) for managing Langflow flows as code. It also adds backend API enhancements for flow upsert, export normalization, and starter project fixes.

Verdict: REQUEST CHANGES — Multiple CRITICAL blockers must be resolved before merge.


Scoring Summary

Category Status Count
CRITICAL (Blockers) FAIL 12 violations
IMPORTANT (Must fix) FAIL 18 violations
RECOMMENDED (Should fix) INFO 8 items
TESTING PASS (with notes) Good coverage overall

CRITICAL: Security & PII

PII in Logs — ZERO TOLERANCE

  • PASS — No email, name, phone, or address found in any log/print statement across all 108 files.
  • PASS — No PII in webhook messages.

General Security

  • PASS — No hardcoded API keys, tokens, or passwords.
  • PASS — No secrets in code (.secrets.baseline updated appropriately).
# Severity File Line(s) Finding
S1 IMPORTANT sdk/environments.py 110-111 Literal API key allowed in config file. api_key field accepts plaintext values from TOML config. While documented as "not recommended," config files can be committed to VCS. Fix: Emit a runtime warning when a literal api_key is found, or require env var references only.
S2 RECOMMENDED sdk/environments.py 75 __repr__ leaks 8 chars of API key. f"{self.api_key[:8]}..." is too revealing for tracebacks/logs. Fix: Show 4 chars max or use "***".
S3 IMPORTANT backend/api/v1/projects.py 289 LIKE wildcard injection. stmt.where(Flow.name.like(f"%{search}%")) — the search query param is not escaped for % and _ wildcards, allowing users to craft patterns matching all flows. SQLAlchemy parameterizes the value (no SQL injection), but wildcards bypass intended filtering. Fix: Escape % and _ before interpolation.
S4 IMPORTANT backend/api/utils/core.py 541 Internal flow ID leaked in error response. f"User not found for public flow {flow_id}" exposes internal UUIDs to clients via HTTPException detail. Fix: Use a generic error message.

CRITICAL: DRY Violations

# File(s) Line(s) Violation Fix
D1 lfx/cli/export.py, login.py, pull.py, push.py 45, 42, 64, 66 _load_sdk() duplicated 4 times. Identical function except error message string. Extract to lfx/cli/_sdk.py as load_sdk(command_name: str).
D2 lfx/cli/export.py, pull.py 199, 75 _safe_filename() duplicated identically. Extract to shared utility.
D3 sdk/client.py 165-500, 538-883 LangflowClient and AsyncLangflowClient are near-complete copies (~400 lines of duplicated logic). Every CRUD method is duplicated with async/await sprinkled in. Use a base class, mixin, or code generation to define logic once and derive sync/async variants.
D4 sdk/testing.py 92-117, 138-188, 213-273 3 sets of duplicated sync/async logic. _resolve_url_client / _resolve_async_url_client, fixture functions, and FlowRunner / AsyncFlowRunner call bodies. Extract shared logic; parameterize the client class.
D5 lfx/testing.py 339-468, 476-590, 683-750 3 sets of duplicated sync/async logic. LocalFlowRunner/AsyncLocalFlowRunner, RemoteFlowRunner/AsyncRemoteFlowRunner, and _resolve_remote_client/_resolve_async_remote_client. Same approach as D3/D4.
D6 backend/api/v1/flows.py 326-339, 540-553, 612-619, 862-875 "UNIQUE constraint failed" error handling block copy-pasted 4 times. Extract to a helper function.
D7 backend/models/flow/model.py 72-85, 274-289 validate_endpoint_name duplicated in FlowBase and FlowUpdate. Extract to standalone validator function.
D8 backend/api/utils/core.py 46-53, 59-67 _get_validated_file_name and _get_validated_folder_name are near-identical. Merge into single parameterized validator.

CRITICAL: File Structure Violations

Files Exceeding 500-Line Limit

# File Lines Assessment
F1 sdk/client.py 883 BLOCKER — Over limit by 76%. Contains 5+ responsibility groups (HTTP utils, Flow CRUD, Run ops, Project CRUD, File I/O) AND massive sync/async duplication. Must be split.
F2 lfx/testing.py 863 BLOCKER — Over limit by 72%. Mix of result building, flow execution, pytest plugin, fixture creation, remote client resolution. Split into testing/result.py, testing/runners.py, testing/plugin.py.
F3 lfx/cli/validate.py 786 BLOCKER — Over limit by 57%. 15 functions (all validation-related). Split into structural checks, semantic checks, and orchestration/rendering.
F4 backend/api/v1/flows.py 1046 BLOCKER — Over limit by 2x. Contains route handlers, filesystem ops, upload/download logic, upsert logic. Split into route handlers, filesystem helpers, flow CRUD helpers.
F5 backend/api/v1/projects.py 751 BLOCKER — Over limit by 50%. Mixes CRUD, MCP server management, file upload/download, ZIP handling.
F6 lfx/__main__.py 729 BLOCKER — Over limit by 46%. 12 command wrappers in one file. Consider command registration module or grouping.
F7 backend/api/utils/core.py 581 Borderline — Over 500 but within 600-700 range. Mixes constants, enums, flow graph builders, exception formatters, cascade delete, pagination, header extraction. Fails SRP, so the relaxed threshold does NOT apply.
F8 backend/initial_setup/setup.py 1358 Pre-existing — Over limit by 3x. Not introduced by this PR but changes touch it. Flag for future cleanup.

Responsibility Mixing

# File Issue
R1 sdk/client.py 5+ different responsibility groups: HTTP utilities, Flow CRUD, Run operations, Project CRUD, File I/O — all in one file.

IMPORTANT: SOLID Principles

# Principle File Line(s) Violation
SO1 SRP backend/api/v1/projects.py 63-222 create_project is ~160 lines mixing project creation, name deduplication, auth settings, and MCP server registration (~85 lines). Extract MCP block to a helper.
SO2 DIP backend/models/flow/model.py 76-84, 278-286 Pydantic validators raise HTTPException — couples the data model to the web framework. Validators should raise ValueError; the HTTP layer should catch and translate.

IMPORTANT: Error Handling

# File Line(s) Issue
E1 lfx/cli/push.py 162 Silent failure: except Exception: pass — comparison failure silently swallowed. Bugs in comparison logic will never surface. Fix: Add logger.debug() at minimum.
E2 lfx/testing.py 235 Silent failure: with contextlib.suppress(Exception) — errors when loading/patching flow JSON for tweaks are silently ignored. Tweaks silently not applying produces confusing test failures.
E3 sdk/client.py 311-312, 724-726 Silent failure: except (json.JSONDecodeError, KeyError): continue — malformed stream chunks silently swallowed. Could hide server-side bugs. Fix: Log at DEBUG level.
E4 sdk/background_job.py 108 Overly broad suppression: with contextlib.suppress(asyncio.CancelledError, Exception) — suppresses ALL exceptions, not just cancellation-related ones.
E5 lfx/cli/export.py 183 Generic except Exception as exc — should catch specific exceptions (json.JSONDecodeError, OSError).
E6 lfx/cli/init.py 195 Generic except Exception — masks serious errors (disk full, permission denied).
E7 lfx/cli/pull.py 109, 195-196, 212-213, 242-243 Multiple generic except Exception as exc blocks with similar error handling.
E8 lfx/cli/validate.py 609 except (OSError, PermissionError)PermissionError is a subclass of OSError, so catching both is redundant.
E9 backend/api/v1/flows.py 265 Fragile parsing: int(flow.endpoint_name.split("-")[-1]) — raises ValueError if last segment isn't a number. No try/except.
E10 backend/api/v1/projects.py 92 Fragile parsing: int(name.split("(")[-1].split(")")[0]) — raises ValueError for non-numeric content in parentheses.

IMPORTANT: Typing Issues

# File Line(s) Issue
T1 lfx/cli/status.py 68, 80 _load_sdk returns tuple[object, object, object] and _flow_hash takes object. # type: ignore[operator] confirms typing is insufficient.
T2 lfx/testing.py 45+ Extensive use of Any type throughout.
T3 sdk/client.py 20, 142, 176, 515+ Extensive Any usage for JSON payloads where dict[str, object] would be more appropriate.
T4 backend/api/v1/flows.py 627 current_user parameter has no type annotation.

IMPORTANT: Code Quality

# File Line(s) Issue
CQ1 backend/api/v1/projects.py 106, 167, 176, 199, 388, 434, 541+ f-strings in logger calls — should use %s formatting for lazy evaluation.
CQ2 backend/api/utils/core.py 537, 281 f-strings in logger calls.
CQ3 backend/models/flow/model.py 125 f-string in logger.warning.
CQ4 lfx/cli/push.py 129 Magic string "dry-run" used as status. Consider enum/constant (like status.py does with _STATUS_*).
CQ5 lfx/cli/status.py 167 Import ordering bug: from langflow_sdk.exceptions import LangflowNotFoundError is outside the _load_sdk lazy-import pattern. If langflow_sdk is not installed, this line crashes before _load_sdk is called.
CQ6 lfx/__main__.py 334, 396 run_command_wrapper and serve_command_wrapper have return type -> None but explicitly return the result of run()/serve().
CQ7 lfx/cli/login.py 67-68 Dead code: _ = all_envs_raw — variable assigned and immediately discarded.
CQ8 sdk/environments.py 177-188 Double file read: TOML file is read and parsed twice in get_environment — once by load_environments() and again to get the default name.
CQ9 lfx/testing.py 397, 452 Side-effectful ternary expression _load_dotenv(...) if (...) else None used for control flow. Should be a proper if statement.
CQ10 backend/api/v1/flows.py 961 Global mutable state: `all_starter_folder_flows_response: Response

IMPORTANT: YAGNI

# File Line(s) Issue
Y1 sdk/models.py 264-272 is_in_progress() always returns False. YAGNI — the method exists for "API parity" but misleads users into thinking progress tracking is available.
Y2 sdk/models.py 209-236 Trivial one-line wrapper methods (get_chat_output, get_text_outputs, get_all_outputs) duplicate the API surface for "V2-SDK naming parity." If backward compat is needed, use deprecation warnings.

RECOMMENDED Items

# File Issue
RC1 sdk/client.py No logging anywhere. An SDK client with zero logging makes debugging integration issues very difficult. Add DEBUG-level logs for connection errors and HTTP errors.
RC2 lfx/cli/init.py lines 59-92 Large string constants (_TEST_FLOWS_PY) embedded inline. Could be moved to template files for consistency.
RC3 sdk/testing.py Session-scoped fixtures return clients but never close them. Use yield + cleanup.
RC4 lfx/config/environments.py lines 200-202 Literal API key from config — no runtime warning emitted. A warning would help teams avoid accidentally committing secrets.
RC5 lfx/cli/validate.py line 443 Magic number _max_sample = 3 — well-named local constant but could be module-level.

TESTING Review

Overall Assessment: PASS (with notes)

Test File Lines Tests Happy Path Adversarial Edge Cases Over 500?
test_create_command.py 362 35 Yes Yes Yes No
test_export_command.py 723 42 Yes Yes Yes Yes
test_init_command.py 658 49 Yes Yes Yes Yes
test_login_command.py 535 30 Yes Yes Yes Yes
test_pull_command.py 845 49 Yes Yes Yes Yes
test_push_command.py 662 35 Yes Yes Yes Yes
test_status_command.py 881 44 Yes Yes Yes Yes
test_validate_command.py 900 44 Yes Yes Yes Yes
test_environments.py 463 33 Yes Yes Yes No
test_testing_plugin.py 643 40 Yes Yes Yes Yes
test_export_normalisation.py 263 22 Yes N/A* Yes No
test_preserve_flow_id.py 208 10 Yes Yes Partial No
test_background_job.py 324 22 Yes Yes Yes No

*Pure transformation with no error paths.

Test Strengths

  • All test files have both happy-path and adversarial tests
  • External dependencies consistently and properly mocked
  • Excellent test independence via tmp_path and monkeypatch
  • Clear Arrange-Act-Assert structure throughout
  • Good edge case coverage

Test Issues

# File Issue
TS1 test_login_command.py 2 pairs of near-duplicate tests: test_auth_error_raises_exit_1/test_auth_failure_exits_code_1 and test_connection_error_raises_exit_1/test_connection_error_exits_code_1.
TS2 test_preserve_flow_id.py TestUploadUpsertBranches tests simulate branching logic manually instead of invoking actual endpoint function — low fidelity.
TS3 test_status_command.py _render_table tests only assert "does not crash" without checking output content.
TS4 8 of 13 test files Exceed 500 lines. While test files are typically exempt from line-count rules, the largest (900, 881, 845 lines) could benefit from splitting.
TS5 All Test naming uses test_X pattern rather than should_X_when_Y. Internally consistent and descriptive enough — acceptable.
TS6 Coverage not run/shown. Per REVIEWER_RULE.md, coverage must be executed and shown with >= 75% minimum.

Pre-Submission Checklist

CRITICAL (Blockers)
[x] No PII in any logs, prints, or webhook messages
[x] No secrets/credentials in code
[ ] No duplicate types, classes, or logic (DRY) ................... FAIL (D1-D8)
[ ] No file exceeds 500 lines ..................................... FAIL (F1-F8)
[ ] No file has more than 5 functions with DIFFERENT responsibilities FAIL (R1)
[x] No file has more than 10 functions even with same responsibility
[x] No file has more than 1 main class
[x] No mixed responsibility prefixes in same file

IMPORTANT (Must fix)
[ ] Each file/function has single responsibility (SRP) ............ FAIL (SO1)
[x] No growing if/elif chains for type handling (OCP)
[x] No subclass that breaks parent's contract (LSP)
[x] No interface forcing useless implementations (ISP)
[ ] High-level modules depend on abstractions (DIP) ............... FAIL (SO2)
[ ] No speculative code for future requirements (YAGNI) ........... FAIL (Y1, Y2)
[x] No deep call chains reaching through object internals
[ ] Proper error handling (no silent failures) .................... FAIL (E1-E10)
[ ] Strong typing (no any/object types) ........................... FAIL (T1-T4)
[x] Inputs validated at boundaries
[x] Types in dedicated types file (where applicable)
[x] Constants in dedicated constants file (where applicable)

RECOMMENDED (Should fix)
[ ] Appropriate logging at key points ............................. FAIL (RC1)
[x] No unnecessary comments
[x] No over-engineering

TESTING
[x] Unit tests for core logic
[x] Tests cover success, error, and edge cases
[x] Tests have BOTH happy path AND adversarial tests
[x] Adversarial tests included
[ ] Coverage ran and shown >= 75% ................................. NOT VERIFIED (TS6)
[ ] All created tests pass — zero failures ........................ NOT VERIFIED
[x] Not prolonging legacy bad patterns

Priority Action Items

Must Fix Before Merge (CRITICAL)

  1. D1: Extract _load_sdk() to shared utility — duplicated across 4 CLI files
  2. D2: Extract _safe_filename() to shared utility — duplicated in export.py and pull.py
  3. D3: Eliminate sync/async duplication in sdk/client.py — ~400 lines of copy-paste. Use base class, mixin, or codegen
  4. D4-D5: Eliminate sync/async duplication in testing modules — same pattern in both SDK and lfx testing
  5. D6: Extract UNIQUE constraint error handler in flows.py — copy-pasted 4 times
  6. F1: Split sdk/client.py (883 lines) — into separate modules by responsibility
  7. F2: Split lfx/testing.py (863 lines) — into result, runners, plugin modules
  8. F3: Split lfx/cli/validate.py (786 lines) — into structural, semantic, orchestration
  9. F4: Split backend/api/v1/flows.py (1046 lines) — into handlers, helpers, CRUD
  10. F6: Split lfx/__main__.py (729 lines) — command registration

Must Fix Before Merge (IMPORTANT)

  1. E1: Remove silent except Exception: pass in push.py:162 — add debug log
  2. E2: Remove silent suppression in testing.py:235 — tweaks silently not applied
  3. E3: Add debug logging for silent stream parse failures in client.py
  4. SO2: Move HTTPException out of Pydantic validators in model.py
  5. CQ5: Fix import ordering bug in status.py:167 — will crash if SDK not installed
  6. S3: Escape LIKE wildcards in projects.py:289
  7. TS6: Run and show test coverage — required by review rules

Should Fix (RECOMMENDED)

  1. RC1: Add debug-level logging to SDK client
  2. RC4: Emit warning for literal API keys in config
  3. S2: Reduce API key exposure in __repr__ to 4 chars

Files Not Requiring Changes

The following files were reviewed and found clean:

  • lfx/cli/create.py — 139 lines, well-structured, no violations
  • lfx/config/__init__.py — 6 lines, clean re-export
  • sdk/exceptions.py — 55 lines, clean exception hierarchy
  • sdk/_version.py — 2 lines, clean
  • sdk/serialization.py — 225 lines, well-structured
  • sdk/background_job.py — 111 lines, clean (minor E4 issue)
  • Docker changes — correct and minimal
  • Starter project JSON updates — trivial version bumps
  • pyproject.toml — correct dependency additions

Notes on Pre-Existing Issues

The following issues exist in files touched by this PR but were NOT introduced by this PR:

  • backend/initial_setup/setup.py (1358 lines) — pre-existing oversized file. The changes in this PR (deepcopy fixes, model_dump adjustments) are good fixes that don't introduce new violations.
  • backend/api/v1/flows.py and projects.py — pre-existing large files. The PR adds ~100 lines to each. The new code itself is reasonable, but it pushes already-oversized files further over the limit.

@erichare
Copy link
Copy Markdown
Collaborator Author

@Cristhianzl - thanks so much for the review. Could you do a re-review when you get a chance? I think the points are more or less all addressed now.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 84 out of 95 changed files in this pull request and generated 5 comments.


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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 83 out of 97 changed files in this pull request and generated 7 comments.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants