Skip to content

fix(AI-3664): migrate flow integration tests off legacy Orchestrator to Conditional Flows - #653

Merged
tomasfejfar merged 3 commits into
mainfrom
martinvasko-ai-3664-migrate-flow-integration-tests-off-legacy-orchestrator
Aug 5, 2026
Merged

fix(AI-3664): migrate flow integration tests off legacy Orchestrator to Conditional Flows#653
tomasfejfar merged 3 commits into
mainfrom
martinvasko-ai-3664-migrate-flow-integration-tests-off-legacy-orchestrator

Conversation

@Matovidlo

Copy link
Copy Markdown
Contributor

Description

Linear: AI-3664

Change Type

  • Major (breaking changes, significant new features)
  • Minor (new features, enhancements, backward compatible)
  • Patch (bug fixes, small improvements, no new features)

Summary

Integration Tests started failing on all 3 Python versions on main (discovered via PR #652 CI,
unrelated to that PR's actual change). Root cause: the pooled integtest project now rejects
legacy Orchestrator flow creation:

403 Forbidden — code: orchestrator.creationDisabled
"Creation of new Flows (keboola.orchestrator configurations) is disabled. Please use Conditional Flows instead."

The last successful CI run on main was 3 days prior — this is a platform-side change to the
shared test project, not a code regression, but it breaks Integration Tests for every PR right
now.

This PR migrates every flow integration test that actually creates a legacy Orchestrator
(keboola.orchestrator) flow via the real API to create a Conditional Flow (keboola.flow)
instead:

  • conftest.py: initial_lf fixture (legacy create_flow) → initial_cf2, a second Conditional
    Flow instance.
  • test_tools.py: removed test_create_and_retrieve_flow (legacy-only, was a duplicate of
    test_create_and_retrieve_conditional_flow), dropped legacy parametrize cases from
    test_update_flow, switched test_get_flows_list and test_flow_lifecycle_integration to use
    two Conditional Flow instances instead of one legacy + one conditional.
  • test_scheduler.py: test_scheduler_lifecycle_tooling now schedules against a Conditional Flow.

Left unchanged (don't hit the disabled create endpoint): test_get_flow_schema (GET-only) and
test_create_legacy_flow_invalid_structure (client-side validation, raises before any API call).

No RFC — this is a test fix, not new behavior (per CONTRIBUTING.md RFC table).

Testing

  • Tested with Cursor AI desktop (Streamable-HTTP transports)

Optional testing

  • Tested with Cursor AI desktop (all transports)
  • Tested with claude.ai web and canary-orion MCP (Streamable-HTTP)
  • Tested with In Platform Agent on canary-orion
  • Tested with RO chat on canary-orion

Checklist

  • Self-review completed
  • Unit tests added/updated (if applicable)
  • Integration tests added/updated (if applicable)
  • Project version bumped according to the change type
  • Documentation updated (if applicable)

@linear-code

linear-code Bot commented Aug 3, 2026

Copy link
Copy Markdown

AI-3664

@Matovidlo

Copy link
Copy Markdown
Contributor Author

@claude review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 updates the flow-related integration tests to avoid creating legacy Orchestrator (keboola.orchestrator) flows, which are now rejected in the pooled integtest project, by migrating those scenarios to Conditional Flows (keboola.flow). It also bumps the project version to reflect the patch-level change.

Changes:

  • Replaced legacy-flow creation coverage in flow integtests with Conditional Flow creation (including adding a second conditional-flow fixture for tests that need two flows).
  • Updated scheduler integtests to operate on a Conditional Flow instead of a legacy Orchestrator flow.
  • Bumped keboola-mcp-server version from 1.74.3 to 1.74.4 (and synced uv.lock).

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
uv.lock Updates the editable package version to 1.74.4 to match the project bump.
pyproject.toml Bumps project version to 1.74.4.
integtests/tools/flow/conftest.py Replaces the legacy-flow fixture with a second conditional-flow fixture (initial_cf2).
integtests/tools/flow/test_tools.py Removes legacy-flow create test, adjusts update/list/lifecycle tests to use Conditional Flows.
integtests/tools/flow/test_scheduler.py Switches scheduler tooling test to target a Conditional Flow (keboola.flow).

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

Comment thread integtests/tools/flow/test_tools.py Outdated
Comment thread integtests/tools/flow/test_tools.py Outdated
@Matovidlo

Copy link
Copy Markdown
Contributor Author

@claude review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 4 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

integtests/tools/flow/test_tools.py:833

  • The cleanup code swallows deletion failures but logs only the exception message, which drops the traceback and makes CI flakes hard to diagnose. Prefer logging with exc_info=True (or LOG.exception) when catching broad Exception.
            except Exception as e:
                LOG.warning(f'Failed to delete {flow_type} flow {flow_id}: {e}')

@Matovidlo
Matovidlo marked this pull request as ready for review August 3, 2026 12:49
@Matovidlo
Matovidlo requested a review from a team as a code owner August 3, 2026 12:49
@Matovidlo
Matovidlo requested review from cjayyy and removed request for a team August 3, 2026 12:49

@keboola-pr-reviewer-bot keboola-pr-reviewer-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Reviewer error — this is not a verdict. The PR reviewer could not complete this review, so no approval is implied. The failure has been logged for the operator.

reviewer could not complete: reviewer model call errored (subtype=success, api_status=400) after 1/18 turn(s).

Retry with @keboola-pr-reviewer review once the underlying issue clears.

@jordanrburger

Copy link
Copy Markdown
Contributor

Review

Verified the diff, the branch state vs main, CI, and the surrounding code.

What the PR does

Integration Tests were red on main for every PR because the shared integtest project started rejecting legacy Orchestrator creation (403 orchestrator.creationDisabled). This PR migrates every flow integtest that creates a keboola.orchestrator config via the real API over to create_conditional_flow.

Diagnosis is right and the fix works — Integration Tests pass on 3.10/3.11/3.12. Net −192 lines, and test_update_flow drops from 12 params × 2 flow creations to 6 × 1, which is a real CI time saving. test_get_flow_schema (GET-only) and test_create_legacy_flow_invalid_structure (raises client-side) were correctly left alone.

The keboola-pr-reviewer-bot failure is an infrastructure error (api_status=400), not a verdict.

Must fix

1. Version bump collides with main. The branch was cut at 5260b35d and is behind main; #650 landed since and already took 1.74.4:

main:  1.74.4  (4d13b9dc chore(deps): bump dependencies...)
PR:    1.74.3 → 1.74.4

Both sides make the identical edit, so git merges it cleanly with no conflict — and this PR ends up shipping no version bump at all, which the "every PR must bump pyproject.toml" rule exists to prevent. Merge main in, go to 1.74.5, re-run uv lock.

Should fix

2. Legacy create_flow is still shipped and now has zero real-API creation coverage. The tool is unconditionally registered, and ToolsFilteringMiddleware.on_list_tools hides it only for projects without the hide-conditional-flows feature (src/keboola_mcp_server/mcp.py:396-398). So the legacy path is still live for hide-conditional-flows projects — and the only reason these tests ever reached the legacy endpoint is that the flow conftest.py deliberately mocks out on_call_tool ("allow all tool calls regardless the testing project features").

Deleting the cases rather than skipping them means that coverage never comes back and nobody notices. Two reasonable options:

  • Gate the legacy params on project capability (skip when hide-conditional-flows is absent from project features) so they self-restore on a legacy-capable project — this also stops the tests from lying about what the middleware would actually permit.
  • Or, if legacy flow creation is dead platform-wide, file the follow-up to deprecate/remove the create_flow tool and delete these tests as part of that change, so tool and test disappear together.

Either way, worth a line in the PR body saying which — right now the deletion reads as incidental.

3. Flow leak in test_flow_lifecycle_integration — the finally fix is incomplete. Cleanup is now correctly wrapped, but created_flows.append(...) runs after the assertions on the create result:

first_result = await create_conditional_flow(...)
assert isinstance(first_result, FlowToolOutput)
assert first_result.success is True
...
created_flows.append((CONDITIONAL_FLOW_COMPONENT_ID, first_result.configuration_id))

If any of those asserts fails, the flow was already created in the shared project but never registered for deletion. test_get_flows_empty asserts exactly zero flows, so one leak cascades into unrelated failures — the precise failure mode the original review comment was about. Move each append to the line immediately after the create_conditional_flow call, before the asserts.

Nits

4. initial_cf2 is a ~40-line verbatim copy of initial_cf, differing only in name/description. Project guidance is explicit about avoiding test bloat; a factory fixture removes the duplication and the drift risk:

@pytest_asyncio.fixture
def create_test_cf(mcp_client, configs, keboola_client):
    @asynccontextmanager
    async def _create(name: str) -> AsyncGenerator[FlowToolOutput, None]:
        ...  # single copy of the create/yield/delete body
    return _create

5. test_update_flow hardcodes the fixture's name. expected_name = updates.get('name') or 'Initial Test Flow' silently breaks if the fixture is ever renamed — use updates.get('name') or initial_flow.name. And since the parametrization no longer carries flow_type, the local flow_type = CONDITIONAL_FLOW_COMPONENT_ID would be better as an assertion on the fixture (assert initial_cf.component_id == CONDITIONAL_FLOW_COMPONENT_ID).

6. Worth taking the suppressed Copilot comment. LOG.warning(f'Failed to delete {flow_type} flow {flow_id}: {e}') drops the traceback, and cleanup failures in a shared project are exactly what you'll want to debug later — add exc_info=True.

7. Comment rot. "Legacy Orchestrator flow creation is disabled on the test project" describes a transient platform state in a permanent place. Reference AI-3664 or the platform decision instead, so the comment doesn't become misleading if the project config changes.

8. make_conditional_phases_and_tasks() takes no arguments and returns identical data both times — the only thing it buys is avoiding shared-dict aliasing between the two creates. A module-level constant plus copy.deepcopy, or a suffix parameter so the two flows are actually distinguishable, would read more honestly. Very minor.

Verdict

Good to go after the version bump is corrected to 1.74.5 (must-fix #1). #2 and #3 are worth resolving before merge but neither blocks unblocking CI — given that Integration Tests are currently red for everyone, landing this quickly with #1 fixed and #2/#3 as immediate follow-ups is a defensible call.


🤖 Reviewed with Claude Code

@jordanrburger jordanrburger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving — correct root-cause fix and Integration Tests are green on all three Python versions.

Please still bump to 1.74.5 before merging (main already took 1.74.4 via #650, so as-is this PR ships no version bump) and re-run uv lock. The rest of my review comment — the lost legacy create_flow coverage and the flow-leak window in test_flow_lifecycle_integration — is fine as follow-up given CI is red for everyone right now.

…nditional Flows

The pooled integtest project now rejects legacy Orchestrator (keboola.orchestrator)
flow creation with a 403 ("Creation of new Flows is disabled. Please use
Conditional Flows instead."), breaking Integration Tests CI for every PR.

- conftest.py: initial_lf fixture (created via legacy create_flow) replaced with
  initial_cf2, a second Conditional Flow instance.
- test_tools.py: removed test_create_and_retrieve_flow (legacy-only duplicate of
  test_create_and_retrieve_conditional_flow), dropped legacy parametrize cases from
  test_update_flow, switched test_get_flows_list and test_flow_lifecycle_integration
  to use two Conditional Flow instances instead of one legacy + one conditional.
- test_scheduler.py: test_scheduler_lifecycle_tooling now schedules against a
  Conditional Flow instead of a legacy Orchestrator flow.

test_get_flow_schema and test_create_legacy_flow_invalid_structure are unchanged
since they never hit the disabled create endpoint (GET-only / client-side
validation respectively).
- test_update_flow: drop the flow_type parametrize axis (every case was
  already Conditional Flow only) instead of leaving a param that no longer
  drives flow_id selection — avoids a latent flow_type/configuration_id
  mismatch if a legacy case were ever added back.
- test_flow_lifecycle_integration: wrap flow creation/assertions in
  try/finally so created flows are always cleaned up, even if an assertion
  fails mid-test (previously a failure could leak flows into the project).
Main already took 1.74.4 via #650; re-bump to avoid shipping this PR with no version change.
@Matovidlo
Matovidlo force-pushed the martinvasko-ai-3664-migrate-flow-integration-tests-off-legacy-orchestrator branch from 4949f09 to 41dc8ca Compare August 5, 2026 04:31
@tomasfejfar
tomasfejfar merged commit f50f7dd into main Aug 5, 2026
19 checks passed
@tomasfejfar
tomasfejfar deleted the martinvasko-ai-3664-migrate-flow-integration-tests-off-legacy-orchestrator branch August 5, 2026 06:26
claude Bot pushed a commit that referenced this pull request Aug 5, 2026
Rebased onto main after #653 merged carrying 1.74.5, which is the version
this branch previously set. The identical bump was dropped as already-applied
during the rebase, so the branch is re-bumped to 1.74.6 to stay strictly above
main.

uv.lock carries only the version line change: the locally available uv (0.12.1)
re-resolves some dependency environment markers differently from the uv that
generated main's lock in #653, so a full 'uv lock' would have reverted that
unrelated marker churn. 'uv sync --frozen --extra dev --extra tests' -- the
check CI actually runs -- succeeds against this lock.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GKKhNMuwHvtoTFt5hUVpLj
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

🚀 Shipped in release v1.74.6.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants