[AJDA-2813] Conditional flows support, drop orchestrator (0.57.0)#387
Conversation
|
ondrajodas
left a comment
ondrajodas
left a comment
There was a problem hiding this comment.
Review of #387 — [AJDA-2813] Conditional flows support, drop orchestrator (0.56.0)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR rewrites the kbagent flow command group around the Conditional Flow component (keboola.flow), drops keboola.orchestrator support, adds two new commands (flow validate, flow schema --full), and performs a thorough pass over all silent-drift surfaces (AGENT_CONTEXT, keboola-expert.md, commands-reference.md, gotchas.md, CLAUDE.md, SKILL.md, flow-workflow.md). The implementation is well-structured: pure-Python validation in a separate flow_validation.py module, graceful schema-fetch degradation, merge-aware update, and full service+CLI+E2E test coverage.
One blocking finding: the two new CLI commands (flow validate and flow schema --full) have no corresponding REST routes in server/routers/flows.py, and the PR description does not document a skip rationale as required by CONTRIBUTING.md. This breaks the 1:1 CLI-to-REST convention that external consumers (Web UI, scheduled agents) rely on.
Verdict: REQUEST CHANGES.
Verdict
- Verdict: REQUEST CHANGES
- Blocking findings: 1
- Non-blocking findings: 2
- Nits: 2
Blocking findings
[B-1] src/keboola_agent_cli/server/routers/flows.py — flow validate and flow schema --full have no REST endpoints
The PR adds two new CLI commands (flow validate, flow schema --full) but flows.py has 8 routes for the 9 total commands. Neither POST /flows/validate nor GET /flows/{project}/schema is present.
Per CONTRIBUTING.md > "Checklist: Adding a New CLI Command" — "HTTP API endpoint in server/routers/<group>.py" — every command needs a matching route, and the only documented exception is for genuinely terminal-only commands (interactive prompts, Rich-rendered-only output, doctor/init/update-style infrastructure). Neither flow validate nor flow schema --full meet that bar: both produce structured JSON output ({"valid", "errors", "warnings", "notes"} and {"format", "schema"}) that external consumers can use, and FlowService.fetch_flow_schema() is already a public service method making the plumbing trivial. The PR description does not mention a skip rationale.
Fix: add a POST /flows/validate route (body: FlowValidate(phases, tasks, project)) that calls flow_validation.validate_conditional_flow with an optional live schema fetch, and add GET /flows/{project}/schema that calls service.fetch_flow_schema(project). If there is a deliberate reason to skip (e.g., validate is treated as a pure-client pre-flight that should never traverse the serve API), document it in the PR description with a one-line reason.
Non-blocking findings
[NB-1] plugins/kbagent/skills/kbagent/references/schedule-workflow.md:50 — stale keboola.orchestrator example in schedule-workflow.md
schedule-workflow.md was not changed in this PR, but it contains an example JSON with "parent_component_id": "keboola.orchestrator". After 0.56.0 drops orchestrator support from the flow group, an AI agent reading this as an archetypal schedule example may infer that orchestrator flows are still actively supported. The schedule list command itself is broader (it lists every keboola.scheduler config regardless of its target component) so the field value is technically not wrong — legacy schedules whose targets are orchestrators will still appear — but the example is confusing when paired with the 0.56.0 gotcha that says orchestrators are dropped.
Fix: add a clarifying sentence: "The example shows keboola.orchestrator as parent_component_id because legacy orchestrator schedules still appear in schedule list output; since 0.56.0 new flows use keboola.flow." Alternatively, update the example JSON to show a keboola.flow target.
[NB-2] src/keboola_agent_cli/services/flow_service.py:154 — broad except Exception in _fetch_flow_schema
_fetch_flow_schema catches except Exception as exc: return None, str(exc) to convert network-level or unexpected errors into a graceful (None, reason) result. This is intentional — a schema-fetch failure must not block a write — but the broad catch swallows errors that might indicate real bugs (e.g., a KeyError from a malformed AiServiceClient response). CONTRIBUTING.md § "Specific exception handling -- never bare except:" applies to production code.
Fix: replace with except (OSError, ConnectionError, TimeoutError) as exc: for transport errors and let unexpected exceptions propagate. The KeboolaApiError path already handles 4xx/5xx API errors; the except Exception catch is only needed for low-level transport issues. Alternatively, keep it as-is but add a comment justifying the broad catch (e.g., # intentional: any failure here must degrade, not block the write).
Nits
-
[NIT-1]src/keboola_agent_cli/services/flow_service.py:77,227,251,547,641,710—exc.error_code == "NOT_FOUND"uses a raw string literal instead ofErrorCode.NOT_FOUND.StrEnumsubclassesstrso comparisons work, but the patternErrorCode.NOT_FOUNDis the stated convention (CONTRIBUTING.md > "Error codes -- enum only, never raw strings"). This pattern exists in one other place (org_service.py:353) and is grandfathered; consider a sweep but not urgent. -
[NIT-2]src/keboola_agent_cli/commands/flow.py:415—import json as _jsonandfrom rich.syntax import Syntaxare deferred inside theflow schema --fullbranch. Deferred imports are fine for rarely-executed paths and keep startup fast, but they are inconsistent with how the samerich.syntax.Syntaximport is written in the sibling non---fullbranch (also deferred, line 432). Both occurrences are in the same function; extract them to the function top (or to the module top) to avoid the duplicated deferred-import pattern. This is cosmetic.
Verification log
gh pr view 387 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 31 files, +4724/-1600, state OPEN, headRefNameondra/AJDA-2813✓git rev-parse --abbrev-ref HEAD→ondra/AJDA-2813(working tree matches PR branch) ✓grep typer src/keboola_agent_cli/services/flow_service.py→ empty ✓ (no layer violation)grep httpx src/keboola_agent_cli/commands/flow.py→ empty ✓ (no layer violation)grep '"flow\.' src/keboola_agent_cli/permissions.py→ 9 entries includingflow.validate: read,flow.schema: read,flow.new: write,flow.update: write,flow.delete: destructive,flow.schedule: write,flow.schedule-remove: destructive,flow.list: read,flow.detail: read✓ (all commands registered)grep '@flow_app.command' commands/flow.py | wc -l→ 9 commandsgrep '@router\.' server/routers/flows.py | wc -l→ 8 routes (MISMATCH:flow validateandflow schema --fullhave no REST route — [B-1])make check→3827 passed, 8 skippedin 87 seconds, exit 0 ✓- Silent-drift surfaces checked:
src/keboola_agent_cli/commands/context.pyAGENT_CONTEXT:flow validateandflow schema --fullpresent ✓CLAUDE.md ## All CLI Commands:flow validateandflow schema --fullpresent ✓plugins/kbagent/agents/keboola-expert.md§2 Tool Selection Matrix: "Author / edit a conditional flow" row updated for 0.56.0 ✓; §3 inline gotchas updated ✓; §1 Rule 6 VERSION GATE: no new version-gated example needed (0.56.0 features are in the matrix row's NEVER column) ✓plugins/kbagent/skills/kbagent/references/commands-reference.md: flow section updated with 0.56.0 note and new commands ✓plugins/kbagent/skills/kbagent/references/gotchas.md: 7 new entries all taggedsince v0.56.0✓plugins/kbagent/skills/kbagent/references/flow-workflow.md: full rewrite with CF structure ✓plugins/kbagent/skills/kbagent/SKILL.md: regenerated ✓plugins/kbagent/.claude-plugin/plugin.json: version synced to 0.56.0 ✓src/keboola_agent_cli/permissions.pyOPERATION_REGISTRY: all 9 flow commands registered ✓INVALID_FLOW_DAGremoved fromErrorCodeenum and replaced withINVALID_FLOW_DEFINITION; backward-compat: no remaining production code references toINVALID_FLOW_DAG✓; changelog notes the rename ✓
schedule-workflow.mdnot changed in this PR; still containskeboola.orchestratorexample (see [NB-1])- Behavior reproduction: not run against a live API (no E2E credentials in environment). E2E tests in
test_e2e.pycoverflow schema → flow validate → flow new → flow list → flow detail → flow update → flow schedule → flow schedule-remove → flow delete(steps 1-9) andflow validatewith--projectandflow schema --full --project(steps 2.1-2.2) with apytest.skipguard when the project lacksconditional_flows. Test suite passes locally without credentials (unit + CLI layer only, 3827 passed) ✓.
Open questions for the author
- Was the omission of REST routes for
flow validateandflow schema --fullintentional? If so, add a one-line skip reason to the PR description (e.g., "validate is a pre-flight safety check designed for client-side use only; serving it over the REST API would encourage callers to bypass local validation, which is the anti-pattern"). If it was an oversight, see [B-1] for the fix.
|
Review findings addressed in 2acf109:
Full suite: 3833 passed, 126 skipped; ruff check + format clean. |
ondrajodas
left a comment
ondrajodas
left a comment
There was a problem hiding this comment.
Review of #387 — [AJDA-2813] Conditional flows support, drop orchestrator (0.56.0)
Generated by
kbagent-pr-reviewersubagent. Scope: commit 2acf109 only (the review-fix commit), per the<focus>directive. The rest of the PR was covered by review 4429120221.Verdict and findings below are advisory; the human author retains every veto.
Summary
Commit 2acf109 correctly addresses all five findings from the previous review. The two new REST routes (POST /flows/validate, GET /flows/{project}/schema) are well-formed: they are declared before the path-parameter routes so literal segments win in FastAPI's declaration-order match, the error paths are correct (degrade-to-200 for validate, 502 for schema), and the six new router tests confirm both happy paths and the route-ordering guards. The ErrorCode.NOT_FOUND enum switch eliminates the last raw string comparison in flow_service.py, the broad except Exception in _fetch_flow_schema is now accompanied by an accurate justification comment, and the schedule-workflow.md example is updated. The only residual item is a leftover deferred import that the commit message does not claim to have addressed and that carries no runtime risk.
Verdict: APPROVE (zero blocking findings).
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 0
- Nits: 1
Blocking findings
(none)
Non-blocking findings
(none)
Nits
[NIT-1]src/keboola_agent_cli/commands/flow.py:467— one deferred import remains after the hoist.from ..services.flow_validation import find_unreachable_phases, validate_conditional_flowsits inside theflow_validate()function body. The commit hoistedjsonandrich.syntax.Syntaxbut left this one.flow_validationhas no circular-import risk (it imports only stdlib +jsonschema), so the import is safe to move to the module-level block alongside the other service imports. No runtime consequence; purely a style inconsistency with the rest of the file and with the commit's stated intent.
Verification log
git rev-parse --abbrev-ref HEAD→ondra/AJDA-2813✓ (working tree on PR branch)gh pr view 387 --json state→OPEN✓git show 2acf109 --stat→ 5 files changed:flows.py(+55),flow_service.py(+16),test_server_router_calls.py(+138),flow.py(+10 imports),schedule-workflow.md(+6) ✓- Route ordering (
grep -n '@router\.' server/routers/flows.py):POST /validateat line 51,GET /{project}/schemaat line 80,GET ""at line 92,GET /{project}/{config_id}at line 105,POST /{project}at line 116. Literal-segment routes declared before path-param routes in both cases ✓ - Route ordering guard tests:
test_flows_validate_without_project_is_semantic_onlyassertsflow_svc.create_flow.assert_not_called();test_flows_get_schema_returns_live_schemaassertsflow_svc.get_flow_detail.assert_not_called()✓ - Error paths: 502 on schema-fetch failure confirmed in
test_flows_get_schema_fetch_failure_is_502; degrade-to-200 on validate schema-fetch failure confirmed intest_flows_validate_schema_fetch_failure_degrades✓ ErrorCode.NOT_FOUNDswitch:grep '"NOT_FOUND"' src/keboola_agent_cli/services/flow_service.py→ empty ✓ (all raw comparisons replaced)- Broad except comment in
_fetch_flow_schema(flow_service.py:155): comment present and accurate — explains httpx.HTTPError / OSError inheritance gap ✓ - Deferred imports in flow.py:
jsonandrich.syntax.Syntaxhoisted to module top ✓;flow_validationimport still deferred at line 467 (NIT-1 above) - schedule-workflow.md: both example occurrences of
"componentId": "keboola.orchestrator"switched to"keboola.flow"; note added for legacy orchestrators ✓ uv run pytest tests/test_server_router_calls.py -k flows -v→ 6 passed ✓make check→3833 passed, 8 skipped— all checks passed, lint clean,make check-error-codesclean ✓
Open questions for the author
(none)
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
ondrajodas
force-pushed
the
ondra/AJDA-2813
branch
from
d375b00 to
8b0f24b
Compare
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Review of #387 — Conditional flows support, drop orchestrator (0.56.0)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR rewrites the kbagent flow command group around the Conditional Flow component (keboola.flow), drops keboola.orchestrator support entirely, adds two new commands (flow validate, flow schema --full), migrates the error code INVALID_FLOW_DAG → INVALID_FLOW_DEFINITION, and removes --component-id from every flow subcommand. The implementation is thorough: all major plugin sync surfaces are updated, make check passes clean (3866 passed, 8 skipped), the agent prompt stays within the 62 KB budget (48 840 bytes), the command-sync CI gate is green (229 commands registered), and SKILL.md is fresh. There are zero blocking findings. Two non-blocking issues are flagged: two semantically distinct 2-tuples added to flow_service.py when the coding convention calls for dataclasses, and flow.py crossing the soft LOC ceiling for the first time. One NIT on the REST backward-compat story. Verdict: APPROVE.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 1
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/flow_service.py:140,173 — two new semantically-distinct 2-tuples instead of dataclasses
_fetch_flow_schema (line 140) and the public fetch_flow_schema (line 173) both return tuple[dict[str, Any] | None, str | None] where the first element is the schema and the second is a failure reason. These are two semantically distinct values (schema payload vs. human-readable error string), and both are new additions in this PR. Per CONTRIBUTING.md > "Code Quality Patterns" > "Return values": "two-element tuples should use a dataclass when the values are semantically distinct" and "existing tuple[...] returns in services are grandfathered, but do not add new ones." A named dataclass (e.g. @dataclass(frozen=True) class SchemaFetchResult: schema: dict | None; failure_reason: str | None) would make every call site self-documenting, which matters because the pattern is reused in three locations (_fetch_flow_schema, fetch_flow_schema, and the REST router's validate endpoint).
Fix: introduce a SchemaFetchResult dataclass adjacent to FlowService and update the three call sites; the router keeps its local unpacking.
[NB-2] src/keboola_agent_cli/commands/flow.py:1–946 — file crosses soft LOC ceiling for first time
Before this PR flow.py was 778 lines (under the 800-line soft ceiling). This PR takes it to 946 lines (past soft, still under the 1200-line hard ceiling). Per CONTRIBUTING.md > "File-size budgets": "When a file crosses the soft ceiling, the next PR that adds material to it should split first." The file now owns command parsing for 8 subcommands (list, detail, schema, validate, new, update, delete, schedule, schedule-remove) plus substantial private helper functions (_load_flow_yaml, _format_flow_detail, _format_flows_table, _format_schedule_info). A natural split point would be a _flow_formatters.py private helper for the Rich output helpers, leaving the Typer command functions in flow.py.
This does not block the current PR because the ceiling was not yet crossed before; it is flagged so the next PR touching flow.py is on notice.
Nits
[NIT-1]src/keboola_agent_cli/server/routers/flows.py:16-38—FlowCreate,FlowUpdate, andFlowSchedulesilently droppedcomponent_id(wasstr = "keboola.flow"default). The PR description calls this a breaking change at the CLI level, but the REST router breaking change gets no mention there. Any external caller that currently passescomponent_idin the POST body would get a422 Unprocessable Entity(Pydantic extra-field rejection) after this ships. If the REST API consumer set is small and well-known (just the Web UI and scheduled agents), this is acceptable given the version bump to 0.56.0; if not, anextra = "ignore"model config or an explicit deprecation header would make the transition safer. Worth a one-sentence note in the PR description and changelog entry for REST API consumers.
Verification log
gh pr view 387 --json title,body,files,state→ 35 files, +4938/-1612, state OPEN. PR title lacks conventional-commit prefix but commits usefeat(flow)!:which is correct. ✓git rev-parse --abbrev-ref HEAD→ondra/AJDA-2813aftergh pr checkout 387. ✓- Layer violation checks (typer in services, httpx in commands, formatter in clients) → all empty. ✓
grep -E '^\+.*error_code\s*=\s*"[A-Z_]+"' /tmp/kbagent-pr-387.diff | grep -v tests/→ only test files use raw string literals; production code usesErrorCodeenum throughout. ✓grep -E '^\+\s*except\s*:'→ empty (no bare excepts). ✓grep -E '^\+\s*print\(' ... src/→ empty. ✓grep -n 'flow\.' src/keboola_agent_cli/permissions.py→flow.schema: read,flow.validate: readboth present alongside existing flow entries. ✓make command-sync-check→OK: all 229 CLI commands are registered. ✓make skill-check→SKILL.md is up-to-date. ✓wc -c plugins/kbagent/agents/keboola-expert.md→ 48 840 bytes (budget 62 000). ✓tests/test_agent_prompt.pybudget assertion: 48 840 < PROMPT_BYTE_BUDGET. ✓grep 'INVALID_FLOW_DAG' src/ tests/ plugins/ docs/→ no live production reference; historical changelog text only. ✓ (ErrorCode dropped cleanly, changelog notes it.)grep -rn 'ORCHESTRATOR_COMPONENTS' src/ tests/→ single hit inchangelog.pyhistory text; removed fromsync/config_format.py. ✓grep 'component_id' src/keboola_agent_cli/server/routers/flows.py→ empty;FlowCreate/FlowUpdate/FlowSchedulemodels no longer carrycomponent_id. ✓grep 'since.*0\.56\|0\.56\.0' plugins/.../gotchas.md→ 9 tagged entries covering CF-only migration, string IDs, dropped orchestrator, validate semantics, live schema fetch, graceful degradation,--component-idremoval. ✓wc -l src/keboola_agent_cli/commands/flow.py→ 946 lines (soft ceiling 800, hard ceiling 1200). Crossing soft ceiling → NB-2.grep '-> tuple' src/keboola_agent_cli/services/flow_service.py(new additions) →_fetch_flow_schemaandfetch_flow_schemaboth returntuple[dict|None, str|None]added in this PR → NB-1.uv sync --extra server && make check→3866 passed, 8 skipped, 12 warningsin 81.95s; ruff + format + ty + SKILL + version + command-sync + changelog + error-codes all green. ✓flow validateexit code: 0 on valid (warnings still printed), exit 2 on errors — confirmed incommands/flow.py:516-517. ✓flow listdual-mode legacy warning: JSON passeslegacy_orchestrator_countin result dict (programmatically readable); human mode printsformatter.warning(...)via_format_flows_table. ✓flow validateJSON shape{valid, errors, warnings, notes}confirmed atcommands/flow.py:505. Matches PR description claim. ✓- Behavior reproduction against real API: not possible without live credentials; the E2E suite at
tests/test_e2e.py:5185(CF round-trip) and5342(validation rejects invalid) is the substitute and is present.
Open questions for the author
(none)
--component-id removed from all flow subcommands and the /flows REST surface; orchestrator configs dropped from flow list (legacy count warning). Payload validation runs against the live conditional-flow JSON Schema fetched at runtime from the AI Service component registry (never bundled), with semantic-only graceful degradation when the fetch fails. New flow validate [--project] and flow schema --full. Detail rendering rewritten for conditional flows. INVALID_FLOW_DAG renamed to INVALID_FLOW_DEFINITION. Docs and plugin surfaces synced.
- add POST /flows/validate and GET /flows/{project}/schema REST routes
to keep the 1:1 CLI-to-REST parity for the new flow commands; routes
are declared before the path-param routes so literal segments win
- add router call tests for both endpoints (incl. route-ordering guards
and the fetch-failure degrade/502 paths)
- document why the broad except in _fetch_flow_schema is intentional
(httpx errors do not subclass OSError; any failure must degrade)
- use ErrorCode.NOT_FOUND enum instead of raw string in flow_service
- hoist deferred json/rich.syntax imports to module top in commands/flow.py
- schedule-workflow.md: switch example target to keboola.flow and note
legacy orchestrator schedules still appear in schedule list
Follow-up to the review-fix commit: the deferred import inside flow_validate() was the last remaining one; flow_validation is pure (stdlib + jsonschema), so module-level import is circular-import safe.
TestPullJobsFallback asserted MagicMock.call_count on a path where _fetch_jobs_per_config calls the mock from a ThreadPoolExecutor. MagicMock.call_count is incremented without a lock (read-modify-write), so concurrent calls lose updates -- reproducible locally with a lowered sys.setswitchinterval (30/30 runs lose increments) and surfaced in CI where coverage tracing widens the race window (observed 174 != 200). call_args_list.append IS atomic under the GIL, so assert on len(call_args_list) instead; the 200-config test additionally checks the distinct (component_id, config_id) pairs to guarantee exactly-once fetching. Production code is unaffected (results were already collected under an explicit lock).
'3 legacy keboola.orchestrator flow(s) ... migrate to keboola.flow' read like internal component IDs; reword to product terminology: 'Legacy Flows were dropped in 0.56.0; migrate them to Conditional Flows'.
main shipped v0.56.0 as a maintenance re-release (#399) — claiming the 0.56.0 version, git tag, and GitHub Latest release — to push the 0.55.0 reference-data commands through the auto-update version gate. The conditional-flows work therefore moves to 0.57.0. Bumps pyproject + plugin.json + marketplace.json + uv.lock, and rewrites every (since v0.56.0) doc tag to v0.57.0 across CLAUDE.md, README, keboola-expert.md, gotchas.md, commands-reference.md, flow/schedule workflows, AGENT_CONTEXT, and code comments. The changelog 0.57.0 key carries the conditional-flows entry; the 0.56.0 maintenance entry is preserved.
Addresses self-review findings on the conditional-flows work:
- Replace the (schema, reason) tuple from FlowService._fetch_flow_schema / fetch_flow_schema with a frozen FlowSchemaFetch dataclass (CONTRIBUTING.md forbids new semantically-distinct tuple returns). Updates all six src call sites (flow_service, flows router, flow command) and the tuple mocks in test_flow_service / test_flow_cli / test_server_router_calls.
- Add a structural integer-id rejection test (test_flow_validation): an integer id is rejected only when the live schema is supplied; without a schema the semantic checks coerce it and stay silent.
- Add router-level tests that POST/PATCH /flows/{project} call create_flow / update_flow WITHOUT component_id, proving the orchestrator-drop is enforced at the REST layer (FlowCreate/FlowUpdate silently drop a legacy component_id).
Rebased onto
|
CONTRIBUTING.md requires semantically-distinct multi-value returns to use a @DataClass, not a bare tuple. That rule is subjective ('semantically distinct'), so unlike the deterministic error_code check it cannot be a CI gate -- it slipped past CI to an LLM/human review on #387 (the FlowSchemaFetch finding). Adds to the reviewer's Step 3.8 a grep that surfaces '-> tuple[...]' return annotations added in the diff (skipping variadic 'tuple[X, ...]'), plus a judging rubric: ignore the ~63 grandfathered returns and the BaseService parallel-result shape; flag a newly-added heterogeneous tuple of semantically-distinct values as NON-BLOCKING with a @DataClass recommendation.
padak
left a comment
padak
left a comment
There was a problem hiding this comment.
Review of #387 — [AJDA-2813] Conditional flows support, drop orchestrator (0.57.0)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This is a well-executed breaking-change PR that rewrites the flow command group
around the Conditional Flow component (keboola.flow), drops keboola.orchestrator
support, adds flow validate and flow schema --full, and ships a live-schema
validation path for flow new/flow update. All silent-drift surfaces from the
Plugin synchronization map are updated (AGENT_CONTEXT, CLAUDE.md, keboola-expert.md,
SKILL.md, commands-reference.md, gotchas.md, flow-workflow.md), the command-sync gate
passes (229 commands registered), and make check is green (3870 passed). Two
NON-BLOCKING findings and two NITs; no blocking issues. Verdict: APPROVE.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 2
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] docs/build-your-own-client.md:269 — stale INVALID_FLOW_DAG error code in public-facing docs
The ErrorCode.INVALID_FLOW_DAG enum member was removed in this PR and replaced by
INVALID_FLOW_DEFINITION. docs/build-your-own-client.md is the project's
public "third-party client" guide and still lists INVALID_FLOW_DAG as a
domain-specific code (line 269: ``INVALID_FLOW_DAG, …). A downstream developer
reading this doc will implement against a code that no longer exists and will have
a silent no-match on error handling. Fix by replacing `INVALID_FLOW_DAG` with
`INVALID_FLOW_DEFINITION` in that sentence. This file is not CI-checked and was not
in the modified files list.
[NB-2] src/keboola_agent_cli/services/flow_service.py:73-75 — _count_phases_tasks is dead code with a banned tuple return
The helper _count_phases_tasks (returns tuple[int, int]) was defined but the two
actual callsites (list_flows lines 358-359 and get_flow_detail lines 428-429)
compute len(phases) and len(tasks) directly instead of calling it. The function
is only exercised in test_flow_service.py:130,133, which tests the helper in
isolation. Per CONTRIBUTING.md "Code Quality Patterns", two-element tuples should use
a @dataclass when the values are semantically distinct (phase count vs. task count
clearly are). Since the function is effectively unused in production paths, the
simplest fix is either (a) delete it and the two test lines, or (b) convert to a
dataclass PhaseTaskCounts and actually route the callsites through it so the name
pays off. Option (a) is faster; option (b) is the intended convention path.
Nits
-
[NIT-1]src/keboola_agent_cli/commands/flow.pyis at 948 LOC, approaching the
800-LOC soft ceiling forcommands/*.py. Not blocking now, but the next PR that
adds materially to this file should split first (e.g. extract thevalidate/schema
commands or the Rich-rendering helpers into a_flow_render.pysibling). -
[NIT-2]PR title and description say "(0.56.0)" but the actual version bump is
to0.57.0(pyproject.toml,changelog.py,gotchas.md,keboola-expert.md
all correctly say0.57.0). The description also says "Version bump to 0.56.0".
This is cosmetic (the code is right; the PR description is stale text from an
intermediate draft), but worth correcting before merge so the GitHub search history
isn't misleading.
Verification log
gh pr view 387 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 34 files, +5039/-1612, state=OPEN ✓git rev-parse --abbrev-ref HEAD→ondra/AJDA-2813(worktree checked out viagh pr checkout 387) ✓- 3-layer compliance:
grep -E '(from typer|import typer|formatter\.|console\.print)' diff | grep services/→ empty ✓;grep -E '(from httpx|import httpx)' diff | grep commands/→ empty ✓ - Plugin synchronization map (all "NO" rows walked):
src/keboola_agent_cli/commands/context.pyAGENT_CONTEXT →flow validate,flow schema --fullpresent under### Flowsheading ✓CLAUDE.md## All CLI Commands→flow schema [--full --project NAME]andflow validate --file @flow.yaml|- [--project NAME]both present ✓permissions.pyOPERATION_REGISTRY →flow.validate: readadded (line 291) ✓; all 9 flow entries present ✓keboola-expert.md§2 Tool Selection Matrix →flowrow updated withflow validateas First choice,--component-idandkeboola.orchestratorin NEVER column ✓; §3 Inline Gotchas →since 0.57.0gotcha block present ✓plugins/kbagent/skills/kbagent/references/commands-reference.md→flow validateandflow schema [--full]present under## Flowssection ✓plugins/kbagent/skills/kbagent/references/gotchas.md→ 9×(since v0.57.0)version tags on all new CF gotchas ✓plugins/kbagent/skills/kbagent/references/flow-workflow.md→ full rewrite for CF;flow validateworkflow at lines 96 and 161 ✓SKILL.md→make skill-check→ "SKILL.md is up-to-date" ✓uv run python scripts/check_command_sync.py→ "OK: all 229 CLI commands are registered" ✓
- Layer violations: none found ✓
- Magic numbers / raw error_code strings in production code:
"UNEXPECTED_ERROR"atflow_service.py:300is a pre-existing grandfathered pattern present inbase.py:163,branch_service.py:73, etc. -- not introduced by this PR ✓; all newKeboolaApiErrorraises useErrorCode.INVALID_FLOW_DEFINITION✓ INVALID_FLOW_DAGremoval: removed fromErrorCode; still present indocs/build-your-own-client.md:269(NB-1) and in historicalchangelog.pyentries (acceptable per design doc) ✓- FlowSchemaFetch return type:
@dataclass(frozen=True)atflow_service.py:39-42, not a tuple ✓ _count_phases_taskstuple return:tuple[int, int]at line 73, function is defined but unused in production paths; tested in isolation (NB-2)keboola-expert.mdbyte budget: 48840 bytes vs 62000-byte hard cap ✓ (13160 bytes headroom)flow.pysize: 948 LOC vs 800 soft / 1200 hard ceiling -- approaching soft ceiling (NIT-1) ✓flow_service.pysize: 774 LOC vs 1000 soft ceiling ✓- Server router (
server/routers/flows.py):POST /flows/validateandGET /flows/{project}/schemaadded;component_idfield silently dropped by Pydantic for backward compat; testtest_flows_create_drops_component_idcovers this ✓; all 9 CLI flow subcommands have matching REST routes ✓ make check: 3870 passed, 8 skipped, 14 warnings (all pre-existing asyncio coroutine warnings from MCP tests) -- exit 0 ✓- Backward compat -- exit codes:
INVALID_FLOW_DAGremoved fromErrorCode; not found in any production code path insrc/-- only inchangelog.pyhistorical text,errors.pycomment, anddocs/build-your-own-client.md✓ (compat break is intentional and changelog-announced) - Token discipline: no new token exposure in flow_service, flow_validation, commands/flow ✓
- E2E tests:
test_flow_crud_and_schedule,test_flow_validation_rejects_invalid_definition,test_flow_list_no_project_returns_all,test_flow_list_with_schedulesall present;flow validateexercised with and without--project✓ - Behavior reproduction: not run against a live project (E2E credentials not available in reviewer environment); see E2E test suite for coverage
Open questions for the author
(none)
- docs/build-your-own-client.md: INVALID_FLOW_DAG -> INVALID_FLOW_DEFINITION. The error code was renamed in this PR; the client-docs example still referenced the removed name (docs/ was outside the original rename sweep, which only grepped src/ + tests/). - Remove the dead _count_phases_tasks helper from flow_service.py: the conditional-flows rewrite orphaned it (zero src callers, exercised only by an isolated test) and it carried a banned tuple[int, int] return. Drops its two tests too.
padak
changed the title
|
Addressed the latest review's findings in
Left as a follow-up (not in this PR):
|
2 participants
Summary
Rewrites the
kbagent flowcommand group around the Conditional Flow component (keboola.flow) and dropskeboola.orchestratorsupport entirely. Closes the gap where the CLI treatedkeboola.flowas if it shared the legacy orchestratordependsOnshape — the old template was wrong and_validate_dagvalidated nothing real.Linear: https://linear.app/keboola/issue/AJDA-2813/cf-add-support-in-new-cli
Breaking changes
--component-idremoved from allflowsubcommands andcomponent_idremoved from the/flowsREST surface —keboola.flowis the only flow component.flow listno longer returnskeboola.orchestratorconfigs; a count of ignored legacy flows is surfaced as a warning.flow new/flow updatevalidate payloads against the conditional-flow schema; invalid definitions are rejected withINVALID_FLOW_DEFINITION(replacesINVALID_FLOW_DAG).flow schemaprints the conditional-flow YAML template; the legacydependsOntemplate is gone.Key design points
configurationSchemaofkeboola.flow), so validation always matches what the stack actually serves — no vendored copy to drift.task.phase/gotoreferences, mandatory default transition, ≥1 enabled task per phase, operator/function operand arity, unreachable-phase warnings) are pure Python and always on.flow validate [--project](offline semantic-only, or full validation against the live schema) andflow schema --full --project(dump the live JSON schema).flow detailhuman rendering rewritten for phases/transitions/conditions, task type badges and retry info; JSON output stays full-body passthrough.update_flowstays merge-aware: validation runs on the merged result when only phases or only tasks are provided.ComponentDetailnow tolerates explicitnulldocumentation fields returned by the AI Service (e.g. forkeboola.flow).Docs & plugin sync
All silent-drift surfaces updated per convention #17: CLAUDE.md command list,
AGENT_CONTEXT,keboola-expert.md,SKILL.md(regenerated),commands-reference.md,flow-workflow.md(full rewrite),gotchas.md(since v0.57.0entries), README, changelog.Testing
tests/test_flow_validation.py(validator unit tests incl. arity and reachability).test_flow_service.py/test_flow_cli.pyrewritten with CF payloads; schema-fetch success/failure/empty fallback covered with mocked AI client.conditional_flows.Release
Version bump to 0.57.0 with a breaking-change changelog entry.