From 52c77dd13ccca38ab924513c02cc8940029c43d2 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Thu, 9 Jul 2026 23:49:51 +0300 Subject: [PATCH 01/12] docs(kafka): spec + plan for multi-cluster Kafka support Brainstormed design (named kafka.clusters mirroring database.connections, --cluster flag, per-pattern/reactor cluster binding, v2->v3 + config migrate) and the 4-task implementation plan. Co-Authored-By: Claude --- .../2026-07-09-agctl-kafka-multi-cluster.md | 282 ++++++++++++++++++ ...-07-09-agctl-kafka-multi-cluster-design.md | 164 ++++++++++ 2 files changed, 446 insertions(+) create mode 100644 docs/superpowers/plans/active/2026-07-09-agctl-kafka-multi-cluster.md create mode 100644 docs/superpowers/specs/active/2026-07-09-agctl-kafka-multi-cluster-design.md diff --git a/docs/superpowers/plans/active/2026-07-09-agctl-kafka-multi-cluster.md b/docs/superpowers/plans/active/2026-07-09-agctl-kafka-multi-cluster.md new file mode 100644 index 0000000..9733726 --- /dev/null +++ b/docs/superpowers/plans/active/2026-07-09-agctl-kafka-multi-cluster.md @@ -0,0 +1,282 @@ +# Multi-Cluster Kafka Support — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let one `agctl.yaml` define N named Kafka clusters and let `kafka produce/consume/assert` and `mocks.kafka.reactors` target any of them, mirroring the existing `database.connections` model. + +**Architecture:** Restructure the single `Config.kafka` object into `kafka.clusters.` (a named map) plus `kafka.default_cluster`; add a `resolve_cluster_name` helper that mirrors `resolve_connection_name`; thread a resolved `KafkaCluster` (not the whole `KafkaConfig`) into `new_kafka_client`; bump the config schema `2 → 3` with a `config migrate` rewrite that lifts the flat `kafka:` block into a named cluster. + +**Tech Stack:** Python ≥3.11, Pydantic v2, Click, pytest, optional `confluent-kafka` extra (lazy-imported). + +## Global Constraints + +(From the spec; every task's requirements implicitly include these.) + +- Python ≥3.11; Pydantic v2 models; no new hard dependencies — `confluent-kafka`/`jq` stay behind the `kafka` extra, lazy-imported inside client constructors/methods only. +- Config stays stateless and `${ENV}`-interpolatable; for `kafka.ssl.*`, an empty string counts as unset (never silently downgrade TLS). +- One-JSON-envelope-per-invocation contract holds; exit codes `0` ok / `1` assertion / `2` config-tool-env error. `kafka produce/consume/assert` remain `@envelope`-wrapped. +- Unit tests run **without** `confluent_kafka` installed (clients built with `consumer_factory=`/`producer_factory=` fakes; `new_kafka_client` monkeypatched). +- Test conventions (mirror exactly): no `tests/unit/conftest.py`; per-file local helpers/fakes; pure helpers tested by building dicts/`Config` directly; commands tested via `CliRunner().invoke(cli, [...])` + `json.loads(result.output)` envelope assertions; tmp configs via `tmp_path / "agctl.yaml"`. Shared fixture at `tests/fixtures/agctl.yaml`. +- After the final task, run the `docs-watcher` subagent (CLAUDE.md "Docs Sync") to sync `DESIGN.md` / `ARCHITECTURE.md` / `skills/`. + +--- + +## File Structure + +**Modify:** +- `agctl/config/models.py` — restructure `KafkaConfig`; add `KafkaCluster`; add `cluster` field to `KafkaPattern` and `KafkaReactor`. +- `agctl/config/loader.py` — bump `TOOL_MAJOR_VERSION` to `"3"`; update `_check_version` advice text. +- `agctl/config/migrate.py` — rename `migrate_match_exprs` → `migrate_config`; `TO_VERSION="3"`; `MigrateResult.already_v2` → `already_current`; add structural `kafka:` lift (v1/v2 → v3). +- `agctl/config/validator.py` — replace the flat `kafka.brokers` check with cluster cross-refs; add `pattern.cluster` / `default_cluster` / `reactor.cluster` dangling-ref checks. +- `agctl/commands/kafka_commands.py` — add `resolve_cluster_name`; change `new_kafka_client` to take a resolved `KafkaCluster`; add `--cluster` flag to produce/consume/assert; consume `KafkaPattern.cluster`. +- `agctl/commands/config_commands.py` — update `config_migrate` consumer for the renamed API + `already_current` envelope key + v3 docstring. +- `agctl/commands/mock_commands.py` — build a per-reactor client map (resolving each reactor's cluster); pass to the engine. +- `agctl/commands/discover_commands.py` — surface a pattern's resolved cluster in the kafka-patterns item detail. +- `agctl/mock/engine.py` — accept a per-reactor client map; build each reactor against its own client. +- `agctl/data/sample-config.yaml` — migrate to the v3 clusters shape. +- `tests/unit/test_models.py`, `test_migrate.py`, `test_loader.py`, `test_validator.py`, `test_kafka_commands.py`, `test_mock_engine.py`, `test_mock_kafka_reactor.py`, `test_discover_command.py` — update/add per task. + +**Create:** none (no new modules; resolution helper lives in `kafka_commands.py` next to its DB analog). + +--- + +## Task 1: Config schema v3 — `kafka.clusters`, version bump, and `config migrate` + +This task restructures the schema, bumps the version, and extends migration so a flat v1/v2 config is rewritten to v3. It keeps the whole tree green: single-cluster configs work exactly as before via the new `clusters` shape, because the consumers (kafka_commands/mock_commands/engine/discover) are updated in the same task to resolve the default/single cluster. Multi-cluster *selection* (the `--cluster` flag and pattern/reactor bindings) lands in Tasks 2–3. + +**Files:** +- Modify: `agctl/config/models.py:113-119` (`KafkaConfig`), `:73-76` (`KafkaPattern`), `:256-264` (`KafkaReactor`); add `KafkaCluster`. +- Modify: `agctl/config/loader.py:215` (`TOOL_MAJOR_VERSION`), `:334-360` (`_check_version`). +- Modify: `agctl/config/migrate.py` (full: rename function, `TO_VERSION`, `MigrateResult`, add structural lift). +- Modify: `agctl/commands/config_commands.py:32` (import), `:317-365` (`config_migrate`). +- Modify: `agctl/commands/kafka_commands.py:44-108` (ssl/timeout/group/new_kafka_client operate on a cluster), add `resolve_cluster_name`. +- Modify: `agctl/commands/mock_commands.py:472-478` (resolve default cluster, build one client, guard on the resolved cluster's brokers). +- Modify: `agctl/config/validator.py:100-112` (default-cluster brokers check for reactors), `:88-96` (unchanged location — pattern warnings). +- Modify: `agctl/data/sample-config.yaml`. +- Test: `tests/unit/test_models.py`, `tests/unit/test_migrate.py`, `tests/unit/test_loader.py`, `tests/unit/test_validator.py`, `tests/unit/test_kafka_commands.py`, `tests/unit/test_mock_engine.py`. + +**Interfaces:** + +- **Consumes:** nothing from later tasks. This is the foundation. +- **Produces (contracts later tasks rely on):** + - `KafkaCluster(BaseModel)` with fields `{brokers: list[str] = [], ssl: KafkaSSL | None = None, timeout_seconds: int | None = None, default_consumer_group: str | None = None, schema_registry_url: str | None = None}`. `KafkaSSL` is reused unchanged. + - `KafkaConfig` restructured to `{clusters: dict[str, KafkaCluster] = {}, default_cluster: str | None = None, patterns: dict[str, KafkaPattern] = {}}`. + - `KafkaPattern` gains `cluster: str | None = None`. `KafkaReactor` gains `cluster: str | None = None`. (Fields added here; consumed in Tasks 2–3.) + - `TOOL_MAJOR_VERSION == "3"`. + - `resolve_cluster_name(cfg_kafka: KafkaConfig, explicit: str | None, binding_cluster: str | None = None) -> str` — returns a cluster **name** present in `cfg_kafka.clusters`. Precedence: `explicit` > `binding_cluster` > `cfg_kafka.default_cluster` > the single cluster when exactly one is defined. Raise `ConfigError("No kafka cluster specified", {})` when none resolve and >1 cluster / no default. Raise `ConfigError("Unknown kafka cluster: ", {"cluster": })` when a resolved name is absent from `clusters`. (Mirrors `resolve_connection_name` in `db_commands.py:45`.) + - `new_kafka_client(cluster: KafkaCluster, group_id: str | None = None) -> KafkaClient` — builds the client from `cluster.brokers` + `_kafka_ssl_conf(cluster)`. The module-level `new_kafka_client` remains the test seam (monkeypatched; factory signature changes from `(cfg_kafka, group_id=None)` to `(cluster, group_id=None)`). + - `migrate_config(config: dict) -> MigrateResult` (renamed from `migrate_match_exprs`). `MigrateResult` fields: `{config: dict, from_version: str, to_version: str = "3", rewrites: list[dict] = [], already_current: bool = False}`. The `config.migrate` envelope key `already_v2` becomes `already_current`. + - `MigrateResult.rewrites` entries come in two shapes: jq-prefix rewrites `{"path": str, "before": str, "after": str}` (existing) and structural rewrites `{"path": "kafka.clusters..", "before": , "after": }` — at minimum one record per lifted scalar (brokers/ssl/timeout_seconds/default_consumer_group/schema_registry_url) plus the `default_cluster` set, in deterministic order (brokers, ssl, timeout_seconds, default_consumer_group, schema_registry_url, default_cluster). + +- [ ] **Step 1: Write failing model + migrate + version tests** + + In `tests/unit/test_models.py` add: a `KafkaConfig` built from a `clusters` dict parses, and `cfg.kafka.clusters["main"].brokers == ["h:9092"]`; `cfg.kafka.default_cluster == "main"`; a `KafkaPattern` accepts `cluster="analytics"`; a `KafkaReactor` accepts `cluster="analytics"`. A `KafkaConfig()` default has empty `clusters`, `default_cluster is None`, empty `patterns`. + + In `tests/unit/test_migrate.py`: rename usages to `migrate_config`; assert `TO_VERSION`/`to_version == "3"`; assert `MigrateResult.already_current` (not `already_v2`). Add a test: a flat v2 config `{"version":"2","kafka":{"brokers":["h:9092"],"timeout_seconds":30,"default_consumer_group":"g","patterns":{"p":{"topic":"t","match":".value.x"}}}}` migrates to `config["version"]=="3"`, `config["kafka"]["clusters"]["default"]["brokers"]==["h:9092"]`, `config["kafka"]["clusters"]["default"]["timeout_seconds"]==30`, `config["kafka"]["clusters"]["default"]["default_consumer_group"]=="g"`, `config["kafka"]["default_cluster"]=="default"`, and `config["kafka"]["patterns"]` preserved unchanged. Assert `already_current is False` and `from_version == "2"`. Add a test: a v1 config (flat kafka + a `.value`-less kafka pattern match) migrates to v3 in one pass — both the jq prefix is applied AND the structural lift occurs. Add `test_migrate_idempotent_v3`: a config already `{"version":"3","kafka":{"clusters":{"main":{...}}}}` returns `already_current is True`, `rewrites == []`, deep-copied unchanged. Add a test: `migrate_config` never mutates the input dict. + + In `tests/unit/test_loader.py`: update `test_version_mismatch_raises` to assert `exc.value.detail["tool_major"] == "3"` and `"config migrate" in exc.value.message`; add `test_v2_config_rejected_pointing_at_migrate` (a `version: "2"` tmp config raises `ConfigError`, message mentions migrate). Existing happy-path loader tests must keep passing once the shared fixture is v3 (update the fixture in Step 9). + +- [ ] **Step 2: Run tests to verify they fail** + + Run: `pytest tests/unit/test_models.py tests/unit/test_migrate.py tests/unit/test_loader.py -v` + Expected: FAIL (missing `KafkaCluster`; `migrate_config` undefined; `TOOL_MAJOR_VERSION` still "2"; `already_v2`/`already_current` mismatches). + +- [ ] **Step 3: Implement models + version + migrate (minimal)** + + `models.py`: define `KafkaCluster` (fields above, reusing `KafkaSSL`); restructure `KafkaConfig`; add `cluster` to `KafkaPattern` and `KafkaReactor`. `loader.py`: set `TOOL_MAJOR_VERSION = "3"`; rewrite `_check_version`'s mismatch message to advise running `agctl config migrate` (drop the hardcoded `.body |`/`.value |` hand-prefixing advice, since v3 migrate is now structural+automatic). `migrate.py`: rename `migrate_match_exprs` → `migrate_config`; set `TO_VERSION = "3"`; rename `MigrateResult.already_v2` → `already_current` (idempotency now `source_major == "3"`); keep the three jq-prefix walkers but gate them to run **only when the source major is `"1"`** (v2→v3 needs no jq rewrite — exprs are already envelope-rooted); add a structural lift step `_lift_kafka_clusters(config, rewrites)` that, when `config["kafka"]` is a dict containing a top-level `brokers` (or any of the five flat keys) and no `clusters` key, moves `{brokers, ssl, timeout_seconds, default_consumer_group, schema_registry_url}` into `config["kafka"]["clusters"]["default"]` and sets `config["kafka"]["default_cluster"] = "default"`, recording one structural rewrite per lifted key. Defensive `.get()` chains; a missing/already-clustered `kafka` contributes no rewrites and never raises. The function never mutates its input (deep-copy first). + + `config_commands.py`: update the import to `migrate_config`; in `config_migrate` read `result.already_current` and emit envelope key `"already_current"`; update the Click command docstring to "Rewrite a v1/v2 agctl.yaml to v3 (named kafka clusters; envelope-rooted match)." Keep `.bak` backup, clobber-refusal, `--dry-run`, and `yaml.safe_dump` write exactly as today; `will_write = not result.already_current and not dry_run`. + +- [ ] **Step 4: Run model/migrate/loader tests to verify pass** + + Run: `pytest tests/unit/test_models.py tests/unit/test_migrate.py tests/unit/test_loader.py -v` + Expected: PASS. + +- [ ] **Step 5: Write failing consumer/resolution tests (default-cluster path stays green)** + + In `tests/unit/test_kafka_commands.py`: update the `install_fake` factory signature from `factory(cfg_kafka, group_id=None)` to `factory(cluster, group_id=None)` (the fake client is returned regardless of cluster). Add `test_kafka_produce_single_cluster_no_flag`: a config with one cluster `main` (brokers via `KAFKA_BROKER`) and no `--cluster` flag produces successfully (envelope `ok:true`), proving default/single-cluster resolution. Add `test_kafka_assert_unknown_cluster_error`: `--cluster ghost` → exit 2, `payload["error"]["type"]=="ConfigError"`, `payload["error"]["detail"]["cluster"]=="ghost"`. Add `test_kafka_consume_no_default_multi_cluster_error`: a config with two clusters, no `default_cluster`, no `--cluster` → exit 2, message contains "No kafka cluster specified". + + In `tests/unit/test_validator.py` (`_cfg(**overrides)` helper): update the local `KafkaConfig()` construction to the new shape. Add `test_reactor_default_cluster_missing_brokers_errors`: a `MocksConfig` with one reactor and a default cluster whose `brokers == []` → one error at path `"mocks.kafka"`. Add `test_default_cluster_dangling_ref_errors`: `cfg.kafka.default_cluster="ghost"` with empty clusters → error at `"kafka.default_cluster"`. Add `test_pattern_cluster_dangling_ref_errors`: a `KafkaPattern(cluster="ghost")` → error at `"kafka.patterns..cluster"`. + + In `tests/unit/test_mock_engine.py`: existing tests construct `MockEngine(kafka_client=...)` directly; in Task 1 this stays a single client (no signature change yet), so only the config consumed by any test that builds a `MocksConfig`/`KafkaReactor` must use the clusters shape. Add/adjust so a reactor-driven engine start still emits `started`. + +- [ ] **Step 6: Run consumer tests to verify they fail** + + Run: `pytest tests/unit/test_kafka_commands.py tests/unit/test_validator.py tests/unit/test_mock_engine.py -v` + Expected: FAIL (`cfg.kafka.brokers` AttributeError in kafka_commands/mock_commands; validator's old flat check; factory signature mismatch). + +- [ ] **Step 7: Wire consumers to the resolved default cluster** + + `kafka_commands.py`: add `resolve_cluster_name(cfg_kafka, explicit, binding_cluster=None)` per the Produces contract. Change `_kafka_ssl_conf`, `_resolve_timeout`, `_resolve_group`, and `new_kafka_client` to take a `KafkaCluster` (read `cluster.ssl`, `cluster.timeout_seconds`, `cluster.default_consumer_group`, `cluster.brokers`). In each `_core` (`_kafka_produce_core`, `_kafka_consume_core`, `_kafka_assert_core`): resolve the cluster with `explicit=None`, `binding_cluster=None` for now (Task 2 passes the real values), then `cluster = cfg.kafka.clusters[name]` and `new_kafka_client(cluster, group_id=group)`. Keep all jq/contains/predicate logic unchanged. + + `mock_commands.py` (`mock_run`, ~line 472): replace `if not cfg.kafka.brokers:` with resolving the default cluster (`resolve_cluster_name(cfg.kafka, None)`) and guarding `if not cluster.brokers:`; build `kafka_client = new_kafka_client(cluster)`. (Per-reactor clients come in Task 3; here all reactors still share the single default client, and the engine signature is unchanged.) + + `validator.py`: replace the `not cfg.kafka.brokers` block (lines 100-112) with: when `cfg.mocks.kafka.reactors` is non-empty, resolve a default cluster via the same precedence used by `resolve_cluster_name` (inline: `cfg.kafka.default_cluster`, else the single cluster if exactly one); if none resolves → error at `"mocks.kafka"` "reactors require a resolvable default cluster"; if the resolved cluster exists but `brokers` is empty → error at `"mocks.kafka"` "kafka mocks require kafka.clusters..brokers". Add a `default_cluster` dangling-ref check (path `"kafka.default_cluster"`) and a `KafkaPattern.cluster` dangling-ref check (path `"kafka.patterns..cluster"`). (Reactor.cluster dangling-ref is deferred to Task 3 where it is consumed.) + +- [ ] **Step 8: Run the full unit suite** + + Run: `pytest tests/unit -v` + Expected: PASS (all previously-passing tests, now on the v3 schema). + +- [ ] **Step 9: Migrate sample config + shared fixtures to v3** + + `agctl/data/sample-config.yaml`: restructure the `kafka:` block to `clusters.` (one cluster, e.g. `default`, carrying brokers/ssl/timeout_seconds/default_consumer_group), set `default_cluster`, set `version: "3"`. Must still validate with no env beyond what it already needs (keep `${...:-}` optionals). Update `tests/fixtures/agctl.yaml` to the same v3 shape so loader/kafka/db/mock unit tests that load it keep passing (this fixture is the shared `FIXTURE`). + + Run: `pytest tests/unit -v` then `python -m agctl config validate --config agctl/data/sample-config.yaml` (with the env the sample needs). + Expected: PASS; validate exit 0. + +- [ ] **Step 10: Commit** + + Run: `git add agctl/config/models.py agctl/config/loader.py agctl/config/migrate.py agctl/commands/config_commands.py agctl/commands/kafka_commands.py agctl/commands/mock_commands.py agctl/config/validator.py agctl/data/sample-config.yaml tests/unit/` + Run: `git commit -m "feat(kafka): restructure Config.kafka to named clusters (v3) + migrate"` (end the message with a blank line then `Co-Authored-By: Claude `). + +--- + +## Task 2: `--cluster` flag + per-pattern cluster binding + +Adds the CLI selection surface. After this, `kafka produce/consume/assert --cluster ` targets a specific cluster, and `kafka assert --pattern ` resolves both topic and cluster from the pattern. + +**Files:** +- Modify: `agctl/commands/kafka_commands.py` (Click options on the three commands + `_core` signatures + resolution call sites + pattern `cluster` consumption). +- Test: `tests/unit/test_kafka_commands.py`. + +**Interfaces:** + +- **Consumes:** `resolve_cluster_name`, `new_kafka_client(cluster, ...)`, `KafkaPattern.cluster` (from Task 1). +- **Produces:** the `--cluster ` option on `kafka produce`/`consume`/`assert`; `kafka assert --pattern` resolves the cluster from `cfg.kafka.patterns[].cluster` when `--cluster` is absent. No new public helpers. + +- [ ] **Step 1: Write failing tests** + + Add to `tests/unit/test_kafka_commands.py`: + - `test_kafka_produce_explicit_cluster`: config with clusters `main` (broker A) and `analytics` (broker B); `kafka produce --topic t --message '{}' --cluster analytics` succeeds and the captured fake client was built from `analytics`'s brokers (assert the factory received the `analytics` cluster — capture it in the patched `new_kafka_client`). Expected: exit 0, captured cluster's `.brokers == [""]`. + - `test_kafka_assert_pattern_resolves_cluster`: a `KafkaPattern("ord", cluster="analytics", topic="orders", match=".value.x")` with two clusters; `kafka assert --pattern ord --timeout 2` (no `--cluster`, no `--topic`) resolves cluster `analytics` (captured) and topic `orders`. Expected: the fake client's consumed topic is `orders` and the captured cluster is `analytics`. + - `test_kafka_consume_cluster_flag_overrides_pattern`: `--cluster main` overrides a pattern's `cluster: analytics` (precedence). Expected: captured cluster is `main`. + +- [ ] **Step 2: Run tests to verify they fail** + + Run: `pytest tests/unit/test_kafka_commands.py -k "explicit_cluster or resolves_cluster or overrides_pattern" -v` + Expected: FAIL (no `--cluster` option; pattern cluster not consumed). + +- [ ] **Step 3: Implement the flag and binding** + + Add `@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)")` to `kafka_produce`, `kafka_consume`, `kafka_assert`. Thread a `cluster: str | None` param through each `_core`. In each `_core`, determine the binding cluster: for `assert` with `--pattern`, `binding_cluster = cfg.kafka.patterns[pattern].cluster`; otherwise `binding_cluster = None`. Then `name = resolve_cluster_name(cfg.kafka, explicit=cluster, binding_cluster=binding_cluster)`; `resolved = cfg.kafka.clusters[name]`; `client = new_kafka_client(resolved, group_id=group)`. (For `produce`/`consume` there is no pattern, so `binding_cluster=None`.) No other behavior changes. + +- [ ] **Step 4: Run tests to verify pass** + + Run: `pytest tests/unit/test_kafka_commands.py -v` + Expected: PASS (new tests + all prior). + +- [ ] **Step 5: Commit** + + Run: `git add agctl/commands/kafka_commands.py tests/unit/test_kafka_commands.py` + Run: `git commit -m "feat(kafka): add --cluster flag and per-pattern cluster binding"` (append the `Co-Authored-By` trailer). + +--- + +## Task 3: Per-cluster mock reactors + +Each `mocks.kafka.reactors.` can target its own cluster. The engine receives a per-reactor client map instead of one shared client. + +**Files:** +- Modify: `agctl/mock/engine.py:54-90` (`__init__` signature), `:194-213` (reactor build uses per-reactor client). +- Modify: `agctl/commands/mock_commands.py:113-137` (`new_mock_engine` seam), `:472-503` (build per-cluster client map). +- Modify: `agctl/config/validator.py` (add `reactor.cluster` dangling-ref + per-reactor resolved-cluster brokers check; the Task-1 default-cluster check generalizes). +- Test: `tests/unit/test_mock_engine.py`, `tests/unit/test_mock_commands.py` (if present; otherwise `test_mock_engine.py`), `tests/unit/test_validator.py`. + +**Interfaces:** + +- **Consumes:** `resolve_cluster_name`, `new_kafka_client(cluster, ...)`, `KafkaReactor.cluster` (Task 1). +- **Produces:** + - `MockEngine.__init__(..., kafka_clients: dict[str, KafkaClient] | None = None)` (replaces the single `kafka_client` param). Keyed by **reactor name**. `None` when `run_kafka=False`. + - `new_mock_engine(..., kafka_clients: dict[str, KafkaClient] | None)` (the test seam mirrors the new signature). + - The engine builds each reactor with `client=self._kafka_clients[reactor_name]`. + - Validator: `reactor.cluster` → unknown cluster = error (path `"mocks.kafka.reactors..cluster"`); each reactor's resolved cluster (reactor.cluster → default → single) must exist and have non-empty `brokers`. + +- [ ] **Step 1: Write failing tests** + + In `tests/unit/test_mock_engine.py`: update existing reactor constructions from `kafka_client=` to `kafka_clients={: }`. Add `test_reactors_use_per_cluster_clients`: a `MocksConfig` with two reactors `rA` (cluster `main`) and `rB` (cluster `analytics`); construct `MockEngine(mocks=..., run_kafka=True, kafka_clients={"rA": clientA, "rB": clientB}, emit_fn=capture)`. After `start()` (with fakes that make `probe` a no-op), the started line lists both reactors, and each reactor was wired to its own client (assert via the fake clients recording which broker config / `probe` call each got — capture client identity per reactor). + + In `tests/unit/test_validator.py`: add `test_reactor_cluster_dangling_ref_errors` (`KafkaReactor(cluster="ghost")` → error at `"mocks.kafka.reactors..cluster"`); add `test_reactor_resolved_cluster_missing_brokers_errors` (reactor with `cluster="main"` but `main.brokers==[]` → error at `"mocks.kafka.reactors."`, message names the cluster). + + In `tests/unit/test_mock_engine.py` (or `test_mock_commands.py` if it exists): add `test_mock_run_builds_per_cluster_clients` — monkeypatch `mock_commands.new_kafka_client` with a factory that records the `KafkaCluster` it received per call; invoke `mock run` via `CliRunner` against a two-reactor/two-cluster config (no real broker — fake client returned for every cluster). Assert the factory was called once per distinct cluster with the right `KafkaCluster.brokers`. + +- [ ] **Step 2: Run tests to verify they fail** + + Run: `pytest tests/unit/test_mock_engine.py tests/unit/test_validator.py -v` + Expected: FAIL (`kafka_client=` kwarg no longer accepted; reactor.cluster not validated; per-cluster wiring absent). + +- [ ] **Step 3: Implement per-reactor clients** + + `engine.py`: change `__init__` param `kafka_client` → `kafka_clients: dict[str, KafkaClient] | None = None`; store `self._kafka_clients`. In `start()` Step 1 (line ~203), build each reactor with `client=self._kafka_clients[name]` (lookup by reactor name); keep `prepare()`/probe ordering and the `_kafka_client is None` guard generalized to `self._kafka_clients is None`. `KafkaReactor` (the reactor class) and `kafka_reactor.py` are unchanged — a reactor still takes one `client`. + + `mock_commands.py`: `new_mock_engine` signature mirrors the engine (`kafka_clients=`). In `mock_run` (~line 472): when `run_kafka`, for each distinct cluster resolved across reactors (`resolve_cluster_name(cfg.kafka, None, binding_cluster=reactor.cluster)` per reactor), build one `KafkaClient` via `new_kafka_client(cluster)`; assemble `kafka_clients = {reactor_name: client_for_its_cluster}` (reusing the same client instance for reactors sharing a cluster); drop the single `kafka_client = new_kafka_client(...)` and the old single-cluster brokers guard in favor of per-reactor resolution. Pass `kafka_clients=` to `new_mock_engine`. Keep the `--only kafka` / engine-resolution logic intact. + + `validator.py`: generalize the Task-1 default-cluster reactor check into a per-reactor resolved-cluster check: for each reactor, resolve its cluster (reactor.cluster → default → single), and error if (a) the explicit `reactor.cluster` names an unknown cluster (path `"mocks.kafka.reactors..cluster"`), or (b) the resolved cluster has empty `brokers` (path `"mocks.kafka.reactors."`). Keep the `default_cluster` and `pattern.cluster` checks from Task 1. + +- [ ] **Step 4: Run tests to verify pass** + + Run: `pytest tests/unit/test_mock_engine.py tests/unit/test_validator.py tests/unit/test_mock_kafka_reactor.py -v` + Expected: PASS (reactor-level tests unchanged since `Reactor(client=...)` is untouched; engine/validator updated). + +- [ ] **Step 5: Commit** + + Run: `git add agctl/mock/engine.py agctl/commands/mock_commands.py agctl/config/validator.py tests/unit/` + Run: `git commit -m "feat(mock): per-cluster Kafka reactors via per-reactor client map"` (append the `Co-Authored-By` trailer). + +--- + +## Task 4: Discovery surfacing + docs sync + +Surfaces a pattern's resolved cluster in `discover`, then syncs all user-facing docs via `docs-watcher`. + +**Files:** +- Modify: `agctl/commands/discover_commands.py` (kafka-patterns item detail). +- Modify (via `docs-watcher`): `docs/DESIGN.md`, `docs/ARCHITECTURE.md`, `skills/agctl*`. +- Test: `tests/unit/test_discover_command.py`. + +**Interfaces:** + +- **Consumes:** `resolve_cluster_name`, `KafkaPattern.cluster` (Tasks 1–2). +- **Produces:** `discover --category kafka-patterns --name ` result includes a `"cluster"` key (the resolved cluster name). Summary counts unchanged. + +- [ ] **Step 1: Write failing test** + + In `tests/unit/test_discover_command.py`: add `test_kafka_pattern_item_shows_cluster` — a config with two clusters and a pattern `cluster="analytics"`; `discover --category kafka-patterns --name

` returns `payload["result"]["cluster"] == "analytics"`. And `test_kafka_pattern_item_cluster_defaults` — a pattern with no `cluster` field and `default_cluster="main"` → `"cluster" == "main"`. + +- [ ] **Step 2: Run test to verify it fails** + + Run: `pytest tests/unit/test_discover_command.py -k "cluster" -v` + Expected: FAIL (no `cluster` key in the item result). + +- [ ] **Step 3: Surface the resolved cluster** + + In the kafka-patterns item-detail `_core` (where `pat = cfg.kafka.patterns[name]`), add `"cluster": resolve_cluster_name(cfg.kafka, None, binding_cluster=pat.cluster)` to the result dict (after the existing `topic`/`match`/`params`/`example`). No other discover changes. + +- [ ] **Step 4: Run the full unit suite** + + Run: `pytest tests/unit -v` + Expected: PASS. + +- [ ] **Step 5: Docs sync** + + Invoke the `docs-watcher` subagent to sync `DESIGN.md` (§2.1 kafka schema → clusters; §3.2 `--cluster`; §3.5 reactor `cluster`; §5 version = config schema version; §3.7 `config migrate` v3 result shape `already_current`) and `ARCHITECTURE.md` (§5 `TOOL_MAJOR_VERSION=3` + structural migrate; §8 `new_kafka_client(cluster, ...)` + `resolve_cluster_name` alongside `resolve_connection_name`; §15 remove "schema_registry_url parsed but unused" only if scope changed — it is not, keep it). Also sync the consumer `skills/agctl-config`, `skills/agctl`, `skills/agctl-write-test-runbook`, `skills/agctl-run-test-runbook` to mention `--cluster` and the v3 clusters shape where they author/run Kafka commands. + +- [ ] **Step 6: Commit** + + Run: `git add agctl/commands/discover_commands.py tests/unit/test_discover_command.py docs/ skills/` + Run: `git commit -m "feat(discover): surface kafka pattern cluster; docs sync to v3"` (append the `Co-Authored-By` trailer). + +--- + +## Integration (not a separate task — extend the existing suite) + +`tests/integration/test_kafka_commands.py`: the existing `require_kafka` / `AGCTL_TEST_KAFKA_BROKER` path already exercises the default cluster via the v3 fixture (updated in Task 1 Step 9) — it must stay green with no flag. Add a self-skipping `test_kafka_second_cluster_round_trip` gated on a new `AGCTL_TEST_KAFKA_BROKER_2` env var (mirror the `require_kafka` skip-when-absent pattern): `produce --cluster two` then `assert --cluster two --from-beginning`. If `AGCTL_TEST_KAFKA_BROKER_2` is unset, `pytest.skip()`. This adds no hard CI dependency. Fold this into Task 2 (it tests `--cluster`) as a follow-up step after Step 4 there, committed with Task 2. + +--- + +## Self-Review (run after writing; fixes applied inline) + +- **Code scan:** No method bodies / algorithms / copy-paste code in the plan — only signatures, data shapes, precedence rules, and test scenarios with expected results. ✓ +- **Self-containment:** Each task's Produces block defines the exact types/signatures/error cases the next task consumes (`KafkaCluster` fields, `resolve_cluster_name` precedence + two `ConfigError` shapes, `new_kafka_client(cluster,...)`, `MigrateResult` fields, `MockEngine`/`new_mock_engine` `kafka_clients` map). ✓ +- **Spec coverage:** §6 schema (Task 1), §6 resolution + §9 `--cluster` + pattern binding (Task 2), §7 reactors (Task 3), §8 migrate (Task 1), §5 validation cross-refs (Tasks 1 + 3), discover surfacing (Task 4), sample-config v3 (Task 1), docs (Task 4). ✓ +- **Type consistency:** `new_kafka_client(cluster, group_id=None)` used identically in Tasks 1/2/3; `kafka_clients: dict[str, KafkaClient]` consistent across engine/seam in Task 3; `resolve_cluster_name(cfg_kafka, explicit, binding_cluster=None) -> str` consistent everywhere; `MigrateResult.already_current` consistent in Task 1 + config_commands. ✓ diff --git a/docs/superpowers/specs/active/2026-07-09-agctl-kafka-multi-cluster-design.md b/docs/superpowers/specs/active/2026-07-09-agctl-kafka-multi-cluster-design.md new file mode 100644 index 0000000..ef4fb7d --- /dev/null +++ b/docs/superpowers/specs/active/2026-07-09-agctl-kafka-multi-cluster-design.md @@ -0,0 +1,164 @@ +# Design: Multi-Cluster Kafka Support + +**Status:** Approved (design) — ready for implementation plan +**Date:** 2026-07-09 +**Author:** brainstorming session +**Affects:** `agctl/config/models.py`, `agctl/config/loader.py`, `agctl/config/migrate.py`, `agctl/config/validator.py`, `agctl/commands/kafka_commands.py`, `agctl/commands/mock_commands.py`, `agctl/commands/discover_commands.py`, `agctl/mock/engine.py`, `agctl/data/sample-config.yaml` +**Relation to docs:** **Changes the `agctl` config contract.** `DESIGN.md` §2.1 (`kafka:` schema), §3.2 (`kafka` CLI), §3.5 (`mocks.kafka.reactors`), and `ARCHITECTURE.md` §5 (load/version pipeline) and §8 (KafkaClient construction) require updates — this is **not** a docs no-op. `docs-watcher` runs after implementation per CLAUDE.md "Docs Sync". + +--- + +## 1. Background & Problem + +`agctl` drives every named resource through a **named map**: `services` (`Config.services: dict[str, ServiceConfig]`), `database.connections` (`dict[str, DatabaseConnection]`), `logs.sources` (`dict[str, LogSource]`). The one exception is Kafka. + +`Config.kafka` is a **single object**, not a map (`config/models.py:308` → `KafkaConfig`, `models.py:113`): one `brokers` list, one `ssl` block, one `timeout_seconds`, one `default_consumer_group`, one `patterns` map. Every `kafka` command resolves that single object with no selector — `kafka_commands.py` builds the client via `new_kafka_client(cfg.kafka, ...)` in `produce` (`:128`), `consume` (`:208`), and `assert` (`:421`, `:462`). There is **no `--cluster` flag** anywhere. + +This means `agctl` cannot talk to two Kafka brokers in one config — a real gap for systems that span a primary cluster and an analytics/mirror cluster, or prod + staging side-by-side. It is also asymmetric with `database.connections`, which already solved the named-instance + default + per-template-binding problem. + +A side effect: `mocks.kafka.reactors` (`KafkaReactor`, `models.py:256`) are pinned to that one broker too. The validator errors when reactors exist with no top-level `kafka.brokers` (`validator.py:100`); the `MockEngine` builds each reactor's client against `cfg.kafka` (`mock/engine.py:196-203`). So multi-cluster must reach the mock layer too. + +## 2. Goals + +- Support **N named Kafka clusters** in one `agctl.yaml`, each with its own brokers / TLS / timeout / consumer group. +- Mirror the proven `database.connections` model exactly: a named map, a default, and per-binding selection — so the pattern is familiar and the resolution rules are identical in spirit to `resolve_connection_name` (`db_commands.py:45`). +- Let a `kafka.patterns.` and a `mocks.kafka.reactors.` self-locate their cluster (like a DB template's `connection` field), so a `--pattern`/reactor doesn't need a flag every time. +- Make the upgrade **non-silent and mechanical**: a structured `config migrate` rewrite lifts the old flat block into a named cluster, exactly as the v1→v2 jq-dialect migration already does. + +## 3. Scope & Design Constraints + +- **Named clusters under `kafka:`** — `kafka.clusters.`. Per-broker knobs (`brokers`, `ssl`, `timeout_seconds`, `default_consumer_group`, `schema_registry_url`) move *inside* each cluster. `patterns` stays a top-level map under `kafka:`, now cluster-aware. +- **Cluster selection mirrors DB connection selection.** Precedence: explicit CLI flag > binding's own field > configured default. Resolution is a new helper alongside `resolve_connection_name`, sharing the same contract shape. +- **Version bump `2 → 3`.** The `version` field is the major-schema switch already enforced by `_check_version` (`loader.py`). v3 is the clusters schema; a v2/v1 config is rejected at load with a pointer to `config migrate` — the same forcing function used today for v1-under-v2. +- **`config migrate` extended** to lift the flat `kafka:` block into `kafka.clusters.` and bump the version. v1 inputs are carried through the existing jq-dialect rewrite first, so v1 → v3 works in one pass. +- **Mock reactors included.** Each reactor gains an optional `cluster` field (defaults via the same resolution rules). `MockEngine` builds each reactor's client against that reactor's resolved cluster. +- **`KafkaSSL` unchanged and reused** per cluster. No new TLS surface. + +## 4. Non-Goals + +- **Not changing the jq dialect.** v3 carries the same envelope-rooted `match` semantics as v2. The version bump is structural (the `kafka:` shape), not a match-root change. +- **Not a broker-HA / connection-pooling feature.** A cluster is a named broker *configuration*; we do not add failover, caching, or shared client lifecycle across invocations (per the stateless-invocation invariant, ARCHITECTURE §2). +- **Not multi-tenant auth or per-topic ACLs.** `ssl`/SASL config stays at cluster granularity. SASL beyond `security.protocol` remains deferred (today's `KafkaSSL` enum already allows `SASL_SSL`/`SASL_PLAINTEXT` but supplies no SASL mechanism fields — out of scope here). +- **Not topic→cluster glob routing.** Selection is by named cluster (flag or binding field), not by a routing table over topic names (rejected, §10). +- **Not wiring `schema_registry_url`.** It moves per-cluster (broker-scoped in reality) but stays **parsed-but-unused**, as today (ARCHITECTURE §15 "Known Limitations"). + +## 5. Decisions (recorded with rationale) + +| # | Decision | Rationale | +|---|---|---| +| D1 | **Named clusters map `kafka.clusters.`, mirroring `database.connections`.** A new `KafkaCluster` model holds `{brokers, ssl, timeout_seconds, default_consumer_group, schema_registry_url}`; `KafkaConfig` becomes `{clusters, default_cluster, patterns}`. | Direct symmetry with the already-proven DB model. One pattern, one mental model; the resolution and validation logic are near-copies of the DB equivalents, minimizing new design surface and new failure modes. | +| D2 | **Cluster resolution mirrors `resolve_connection_name` (`db_commands.py:45`).** Precedence: explicit `--cluster` > pattern/reactor `.cluster` > `kafka.default_cluster`. None resolved → `ConfigError`. Resolved name unknown → `ConfigError`. | Identical contract to DB connection resolution (explicit > template > default > error), so behavior is predictable and the two systems read the same way. | +| D3 | **Single-cluster auto-default.** When `default_cluster` is unset **and** exactly one cluster is defined, that cluster is the default. When >1 cluster and no `default_cluster`, an unresolvable command/reactor is a `ConfigError`. | A micro-deviation from DB (which always requires `defaults.database_connection`): the overwhelmingly common config has one cluster, and forcing `default_cluster` boilerplate on it is hostile. The >1 case still fails loudly. | +| D4 | **Default lives at `kafka.default_cluster`, not `defaults.kafka_cluster`.** | Keeps all Kafka concerns self-contained under `kafka:` (the approved design). DB's default lives at `defaults.database_connection` because DB has no enclosing `database.default_*` slot; here the enclosing `kafka:` block is the natural home. Flagged as an intentional asymmetry. | +| D5 | **Version bump `2 → 3` + `config migrate` rewrite.** `TOOL_MAJOR_VERSION` → `"3"`; a v2/v1 config is rejected at load pointing at `config migrate`. Migrate lifts the flat `kafka:` into `kafka.clusters.` (default name `"default"`), sets `default_cluster`, preserves `patterns`, and bumps the version. | The repo's established upgrade path: the v1→v2 jq-dialect migration already does backup → rewrite → bump with `--dry-run`. A structural change to a top-level block warrants the same non-silent, mechanical treatment. Rejecting old configs at load prevents a silently-misinterpreted shape. | +| D6 | **`new_kafka_client` takes a resolved `KafkaCluster`, not `cfg.kafka`.** `_kafka_ssl_conf`, `_resolve_timeout`, `_resolve_group` read off the resolved cluster. | The client only ever needs one broker's config; passing the whole `KafkaConfig` is the current coupling that blocks multi-cluster. Narrowing the input to the resolved cluster is the minimal seam change and keeps the existing test fakes (`kafka_commands.new_kafka_client` monkeypatch) working. | +| D7 | **Each `KafkaReactor` gains optional `cluster` (resolved like a command). The `MockEngine` builds per-reactor clients against resolved clusters.** | A reactor must join a specific broker; under multi-cluster it cannot keep the implicit single-broker assumption. Resolving per-reactor (not one broker for all reactors) lets a mock span clusters — the whole point. | +| D8 | **`patterns` stays a top-level map under `kafka:`, cluster-aware via an optional `cluster` field per pattern.** | A pattern is "a named filter on a topic"; the topic lives on one cluster, so the binding is per-pattern. Keeping `patterns` global (not nested under each cluster) matches DB templates (global map, `connection` field) and avoids duplicating shared patterns across clusters. | + +## 6. Core: Config Schema & Cluster Resolution + +**New `kafka:` shape (contract):** + +```yaml +version: "3" + +kafka: + clusters: + main: + brokers: ["${KAFKA_HOST}:9092"] + ssl: { ca_location: "${KAFKA_SSL_CA:-}", ... } # KafkaSSL, unchanged + timeout_seconds: 30 + default_consumer_group: agctl-consumer + schema_registry_url: "${SCHEMA_REGISTRY_URL:-}" # parsed, still unused + analytics: + brokers: ["${KAFKA_AN}:9092"] + timeout_seconds: 15 + + default_cluster: main # required only when >1 cluster (D3) + + patterns: # global map, now cluster-aware + order-created: + cluster: analytics # optional; default = default_cluster + description: "An ORDER_CREATED event" + topic: orders.created + match: '.value.eventType == "ORDER_CREATED"' +``` + +**Model changes (`config/models.py`, design-level — field names, no bodies):** + +- New `KafkaCluster(BaseModel)` = `{brokers: list[str], ssl: KafkaSSL | None, timeout_seconds: int | None, default_consumer_group: str | None, schema_registry_url: str | None}` (the fields currently on `KafkaConfig`, `models.py:113-119`). +- `KafkaConfig` restructured to `{clusters: dict[str, KafkaCluster], default_cluster: str | None, patterns: dict[str, KafkaPattern]}`. +- `KafkaPattern` (`models.py:73`) gains `cluster: str | None = None`. +- `KafkaReactor` (`models.py:256`) gains `cluster: str | None = None`. +- `KafkaSSL` (`models.py:79`) **unchanged**. +- `Defaults` (`models.py:62`) **unchanged** — the Kafka default lives under `kafka:` (D4). + +**Cluster resolution (new helper, contract mirrors `resolve_connection_name`):** + +- Inputs: the CLI-provided cluster name (optional), a binding's `.cluster` (optional, from a pattern or reactor), and `cfg.kafka`. +- Precedence: CLI name > binding `.cluster` > `cfg.kafka.default_cluster` > (if exactly one cluster) that single cluster. +- None resolvable and >1 cluster and no default → `ConfigError("No kafka cluster specified")`. +- Resolved name not in `cfg.kafka.clusters` → `ConfigError("Unknown kafka cluster: ")` with `detail.cluster`. +- Returns a cluster **name** (callers then index `cfg.kafka.clusters[name]`); the resolved `KafkaCluster` feeds `new_kafka_client`. + +**Command wiring (`commands/kafka_commands.py`):** + +- `produce` / `consume` / `assert` gain an optional `--cluster ` Click option. +- `_resolve_timeout` / `_resolve_group` / `_kafka_ssl_conf` / `new_kafka_client` operate on the resolved `KafkaCluster`. +- `assert` with `--pattern`: the pattern's `.cluster` participates in resolution (so a pattern both supplies the topic **and** the cluster, like a DB template supplies SQL + connection). + +## 7. Mock Reactors + +- `KafkaReactor.cluster` (optional) resolves via the same helper as commands (reactor.cluster → default_cluster → single-cluster). +- `MockEngine` (`mock/engine.py:196-203`) builds each reactor's `KafkaClient` against that reactor's resolved cluster's brokers/TLS, instead of one `cfg.kafka`. +- The validator's "mocks.kafka requires kafka.brokers" check (`validator.py:100-110`) becomes: every reactor's resolved cluster must exist and define a non-empty `brokers`. + +## 8. Migration (`v2 → v3`) + +`agctl config migrate` (`config/migrate.py`, design-level steps — no algorithm bodies): + +1. **v1 input** → run the existing jq-dialect rewrite (`.body |` / `.value |` prefixes on the three match-site families) first; the input is now logically v2. +2. **Structural lift** → move `kafka.{brokers, ssl, timeout_seconds, default_consumer_group, schema_registry_url}` into `kafka.clusters.default.{...}`; set `kafka.default_cluster: "default"`; leave `kafka.patterns` in place. +3. **Version bump** → set `version: "3"`. +4. **Safety** → back up original to `.bak` (refuse to clobber an existing `.bak`); support `--dry-run`; idempotent (an already-v3 config with a `clusters` map is a clean no-op, `already_v3: true`). +5. **Result shape** extends today's `config.migrate` envelope with the structural rewrites list (path/before/after) alongside the existing jq rewrites. + +`config/loader.py::_check_version`: `TOOL_MAJOR_VERSION` becomes `"3"`; the version is now "config schema version" (the jq dialect is a *consequence* of v2+, not its sole meaning — documented in DESIGN §2 / ARCHITECTURE §5). + +## 9. CLI Surface + +- **`--cluster `** — new optional option on `kafka produce`, `kafka consume`, `kafka assert`. Resolved per D2/D3; omitted on a single-cluster config just works. +- **`config migrate`** — extended to perform the v2→v3 structural lift (and v1→v3 in one pass). `config validate` / `config show` otherwise unchanged (they consume the new typed `Config`). +- **`discover`** — `kafka-patterns` item detail surfaces the pattern's resolved cluster name (the summary count is unchanged). +- **Exit codes unchanged:** `0` success, `1` assertion fail, `2` config/tool/env error (unknown cluster, no-cluster-specified, migrate failure, v2-under-v3 load rejection). + +## 10. Rejected Alternatives (ADR-style) + +- **Flat clusters, global patterns, `--cluster` required every call (approach B from brainstorm).** Rejected (D1/D8): forces the flag on every command, prevents a `--pattern` from self-locating, and diverges from the DB model's per-binding field. More friction, less consistency. +- **Topic→cluster glob routing (approach C from brainstorm).** Rejected (Non-Goals): adds a routing table to design, validate, and debug; selection by explicit name is simpler and matches DB. Routing is trivially addable later if a real need appears. +- **Dual-parse (accept flat `kafka:` and `kafka.clusters:` forever, no version bump).** Rejected (D5): two valid shapes to support indefinitely, loader/validator branching, and ambiguous errors when both appear. The repo's culture is one current schema + a mechanical migrate, not perpetual dual support. +- **Hard break, no migrate.** Rejected (D5): worst UX — every consumer hand-edits on upgrade. The migrate command is cheap and already templated by v1→v2. +- **Defer mock reactors to a follow-up.** Rejected (D7): leaves reactors implicitly pinned to "the" broker, which no longer exists under multi-cluster — an immediate inconsistency and a false-green surface (a reactor silently joining the wrong/default cluster). Bundling is the consistent choice. +- **Nest `patterns` under each cluster.** Rejected (D8): would duplicate shared patterns and diverge from DB templates (global map + binding field). A pattern is a named filter, not cluster-owned state. +- **Put the default at `defaults.kafka_cluster`.** Rejected (D4): the enclosing `kafka:` block is the natural home; mirroring DB's `defaults.*` placement here would split Kafka config across two top-level sections for no gain. + +## 11. Validation Strategy + +- **Unit — model/restructure:** `KafkaConfig` accepts the clusters shape; `KafkaCluster` carries the migrated fields; `KafkaPattern`/`KafkaReactor` accept `cluster`. +- **Unit — resolution precedence (mirrors DB `resolve_connection_name` tests):** `--cluster` > binding `.cluster` > `default_cluster`; single-cluster auto-default; unknown-cluster error; no-cluster-specified error (>1 cluster, no default, no flag/binding). +- **Unit — migrate:** flat `kafka:` → `kafka.clusters.default` + `default_cluster`; `version` 2→3; v1 input carried through jq rewrite then lifted to v3 in one pass; idempotency on an already-v3 config; `--dry-run` no-write; `.bak` clobber refusal. Golden before/after YAML. +- **Unit — validator cross-refs:** pattern `.cluster` → unknown cluster = error; reactor `.cluster` → unknown = error; `default_cluster` → unknown = error; reactor whose resolved cluster lacks `brokers` = error; existing pattern description-warning loop unchanged. +- **Unit — kafka commands (fake client, no broker):** `produce`/`consume`/`assert` build the client against the **resolved** cluster (brokers/ssl/timeout/group read off it); `assert --pattern` resolves topic **and** cluster from the pattern. +- **Unit — mock engine (fake client):** each reactor's client is built against that reactor's resolved cluster; reactors with different `cluster` values get different broker configs. +- **Integration — default cluster unchanged:** the existing `AGCTL_TEST_KAFKA_BROKER` suite runs against the default cluster with no `--cluster` flag (single-cluster auto-default); self-skipping when the broker is absent, as today. +- **Integration — second cluster:** when `AGCTL_TEST_KAFKA_BROKER_2` is set, a `produce`/`assert` round-trip targets cluster 2 by name; self-skipping otherwise (no new hard CI dependency). +- **Drift guard:** `sample-config.yaml` is migrated to the v3 clusters shape and stays `${ENV}`-clean (validates with no env beyond what it already needs). + +After implementation, `docs-watcher` runs (CLAUDE.md "Docs Sync") — **expected real updates** to `DESIGN.md` §2.1 (kafka schema), §3.2 (`--cluster`), §3.5 (reactor `cluster`), §5 (version semantics), and `ARCHITECTURE.md` §5 (v3 guard / migrate) and §8 (`new_kafka_client` cluster input), plus the `skills/agctl*` consumer references. + +## 12. Docs & Skill Impact + +- **`DESIGN.md`** §2.1: rewrite the `kafka:` block to the clusters shape; document `default_cluster`, per-pattern/reactor `cluster`, and the v3 version. §3.2: add `--cluster` to `produce`/`consume`/`assert`; document the resolution precedence. §3.5: document reactor `cluster`. §5: broaden "version = jq-dialect switch" to "version = config schema version." +- **`ARCHITECTURE.md`** §5: note `TOOL_MAJOR_VERSION=3` and the structural migrate step. §8: `new_kafka_client(cluster, ...)` and the resolution helper alongside `resolve_connection_name`. +- **`skills/` tree:** `agctl-config` (consumer reference) gains the clusters shape, `--cluster`, and the v2→v3 migrate step; `agctl-write-test-runbook` / `agctl-run-test-runbook` gain `--cluster` awareness where Kafka commands are authored/run. All ship as portable consumer artifacts under `skills/`. +- **`config migrate`** result shape and `cli_flags_note` are extended for the structural lift; CLI `--cluster` flags in scripts/runbooks are **not** rewritten by migrate (out of scope, same caveat as today's `--match`). From 821d6831bf2a82eee8806eecab3402e473fab73c Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 00:26:13 +0300 Subject: [PATCH 02/12] feat(kafka): restructure Config.kafka to named clusters (v3) + migrate Co-Authored-By: Claude --- README.md | 50 ++-- agctl/commands/config_commands.py | 16 +- agctl/commands/kafka_commands.py | 96 ++++++-- agctl/commands/mock_commands.py | 17 +- agctl/config/loader.py | 9 +- agctl/config/migrate.py | 198 +++++++++++---- agctl/config/models.py | 33 ++- agctl/config/validator.py | 59 ++++- agctl/data/sample-config.yaml | 50 ++-- tests/fixtures/agctl.yaml | 37 +-- tests/unit/test_cli.py | 20 +- tests/unit/test_config_commands.py | 30 ++- tests/unit/test_db_commands.py | 2 +- tests/unit/test_discover_command.py | 12 +- tests/unit/test_kafka_commands.py | 126 +++++++++- tests/unit/test_loader.py | 119 +++++---- tests/unit/test_migrate.py | 358 ++++++++++++++++++++-------- tests/unit/test_mock_commands.py | 44 ++-- tests/unit/test_mock_lifecycle.py | 11 +- tests/unit/test_models.py | 87 ++++++- tests/unit/test_overlay.py | 73 +++--- tests/unit/test_validator.py | 77 +++++- 22 files changed, 1109 insertions(+), 415 deletions(-) diff --git a/README.md b/README.md index bc162ac..01e12cb 100644 --- a/README.md +++ b/README.md @@ -203,8 +203,8 @@ version is the same file with secrets/hosts moved into `${...}` and sourced from ```yaml # agctl.yaml -# Version tracks the agctl MAJOR version only (currently "2"). -version: "2" +# Version tracks the agctl MAJOR version only (currently "3"). +version: "3" # --- services: named HTTP base URLs for services under test ----------------- services: @@ -218,31 +218,39 @@ services: health_path: "/health" timeout_seconds: 15 -# --- kafka: broker config -------------------------------------------------- +# --- kafka: named clusters + patterns -------------------------------------- +# Mirrors database.connections: a named map of clusters, a default_cluster, +# and a global patterns map. A single cluster needs no default_cluster +# (auto-defaulted), but it is set here for clarity. kafka: - brokers: - - "localhost:9092" - default_consumer_group: "agctl-consumer" - schema_registry_url: "" # optional; omit/leave empty if unused - timeout_seconds: 30 # default consume/assert timeout - - # Optional TLS/mTLS — uncomment for brokers that require SSL. Setting ANY - # field to a non-empty value enables TLS (security.protocol defaults to "SSL"). - # ca_location is optional: unset → librdkafka uses the system trust store - # (fine for publicly-trusted brokers; pin a CA for private-PKI brokers). - # Hostname verification stays ON unless endpoint_identification_algorithm: "none". - # ssl: - # ca_location: "" - # certificate_location: "" # path to client cert (mTLS) - # key_location: "" # path to client private key (mTLS) - # key_password: "" # optional private-key password - # # endpoint_identification_algorithm: "none" # disable hostname verification - # # security_protocol: "SSL" # default; set SASL_SSL when adding SASL + clusters: + default: + brokers: + - "localhost:9092" + default_consumer_group: "agctl-consumer" + schema_registry_url: "" # optional; omit/leave empty if unused + timeout_seconds: 30 # default consume/assert timeout + + # Optional TLS/mTLS — uncomment for brokers that require SSL. Setting ANY + # field to a non-empty value enables TLS (security.protocol defaults to "SSL"). + # ca_location is optional: unset → librdkafka uses the system trust store + # (fine for publicly-trusted brokers; pin a CA for private-PKI brokers). + # Hostname verification stays ON unless endpoint_identification_algorithm: "none". + # ssl: + # ca_location: "" + # certificate_location: "" # path to client cert (mTLS) + # key_location: "" # path to client private key (mTLS) + # key_password: "" # optional private-key password + # # endpoint_identification_algorithm: "none" # disable hostname verification + # # security_protocol: "SSL" # default; set SASL_SSL when adding SASL + + default_cluster: default # patterns: named Kafka filters, analogous to HTTP templates. # topic: Kafka topic # match: jq boolean predicate over each message envelope; # supports {placeholder} substitution via --param at assert time + # cluster: optional named cluster this pattern binds to (default = default_cluster) patterns: order-created: description: "An ORDER_CREATED event for a specific order" diff --git a/agctl/commands/config_commands.py b/agctl/commands/config_commands.py index e60cc2f..d00143c 100644 --- a/agctl/commands/config_commands.py +++ b/agctl/commands/config_commands.py @@ -29,7 +29,7 @@ from ..config import ConfigError, load_config from ..config.loader import compose_config, discover_config_path -from ..config.migrate import migrate_match_exprs +from ..config.migrate import migrate_config from ..config.validator import validate_config from ..mock.capture_validate import collect_capture_placement_errors from ..mock.jq_precompile import collect_jq_compile_errors @@ -317,11 +317,11 @@ def config_init(ctx: click.Context, output: str | None, force: bool) -> None: def config_migrate( ctx: click.Context, config_path: str | None, dry_run: bool ) -> None: - """Rewrite a v1 agctl.yaml to dialect \"2\" (envelope-rooted match exprs). + """Rewrite a v1/v2 agctl.yaml to v3 (named kafka clusters; envelope-rooted match). Backs up the original to ``.bak`` and writes the rewritten config back to ````. With ``--dry-run`` the rewrite is reported but nothing - is written. A config already at dialect \"2\" is a clean no-op. + is written. A config already at dialect \"3\" is a clean no-op. """ start = time.monotonic() explicit = config_path or ctx.obj.get("config_path") @@ -329,18 +329,18 @@ def config_migrate( path = discover_config_path(explicit=explicit, env=os.environ) raw = path.read_text(encoding="utf-8") parsed = yaml.safe_load(raw) or {} - result = migrate_match_exprs(parsed) - # Write only when migrating for real (not already_v2, not --dry-run). - will_write = not result.already_v2 and not dry_run + result = migrate_config(parsed) + # Write only when migrating for real (not already_current, not --dry-run). + will_write = not result.already_current and not dry_run base_result = { "path": str(path), - "already_v2": result.already_v2, + "already_current": result.already_current, "from_version": result.from_version, "to_version": result.to_version, "rewritten": result.rewrites, "cli_flags_note": _CLI_FLAGS_NOTE, # Surfaced only when the file is actually rewritten — on --dry-run - # and already_v2 nothing is reformatted, so the note would be noise. + # and already_current nothing is reformatted, so the note would be noise. "formatting_note": _FORMATTING_NOTE if will_write else None, } if will_write: diff --git a/agctl/commands/kafka_commands.py b/agctl/commands/kafka_commands.py index e8c382e..1f8daa8 100644 --- a/agctl/commands/kafka_commands.py +++ b/agctl/commands/kafka_commands.py @@ -28,7 +28,7 @@ from ..command import envelope, load_config_or_raise if TYPE_CHECKING: - from ..config.models import KafkaConfig + from ..config.models import KafkaCluster, KafkaConfig from ..errors import AssertionFailure, ConfigError, TemplateNotFound from ..params import parse_params from ..resolution import fill_placeholders @@ -38,11 +38,48 @@ "kafka_consume", "kafka_assert", "new_kafka_client", + "resolve_cluster_name", ] -def _kafka_ssl_conf(cfg_kafka: "KafkaConfig") -> dict[str, str]: - """Translate ``cfg.kafka.ssl`` into librdkafka conf keys. +def resolve_cluster_name( + cfg_kafka: "KafkaConfig", + explicit: str | None, + binding_cluster: str | None = None, +) -> str: + """Resolve the Kafka cluster **name** to use (DESIGN §6, D2/D3). + + Precedence (mirrors :func:`resolve_connection_name` in ``db_commands``, + plus single-cluster auto-default): + + * ``explicit`` (the ``--cluster`` flag — wired in Task 2) + * ``binding_cluster`` (a pattern/reactor ``.cluster`` — Tasks 2-3) + * ``cfg_kafka.default_cluster`` + * the single cluster when exactly one is defined + + Raises :class:`ConfigError` if none resolves (>1 cluster / no default), or + if the resolved name is absent from ``cfg_kafka.clusters``. Returns the + cluster NAME; callers index ``cfg_kafka.clusters[name]``. + """ + name = explicit + if name is None: + name = binding_cluster + if name is None: + name = cfg_kafka.default_cluster + if name is None: + # Single-cluster auto-default (D3): the overwhelmingly common config. + cluster_names = list(cfg_kafka.clusters.keys()) + if len(cluster_names) == 1: + name = cluster_names[0] + if name is None: + raise ConfigError("No kafka cluster specified", {}) + if name not in cfg_kafka.clusters: + raise ConfigError(f"Unknown kafka cluster: {name}", {"cluster": name}) + return name + + +def _kafka_ssl_conf(cluster: "KafkaCluster") -> dict[str, str]: + """Translate a cluster's ``ssl`` block into librdkafka conf keys. Returns an empty dict when no TLS knobs are configured. When any knob is set, ``security.protocol`` defaults to ``"SSL"`` (mTLS) unless @@ -55,7 +92,7 @@ def _kafka_ssl_conf(cfg_kafka: "KafkaConfig") -> dict[str, str]: absent env var to ``""``, and an empty ``ssl.endpoint.identification.algorithm`` must NOT flip verification off the way librdkafka treats ``""`` == ``"none"``. """ - ssl = cfg_kafka.ssl + ssl = cluster.ssl if ssl is None: return {} conf: dict[str, str] = {} @@ -74,37 +111,39 @@ def _kafka_ssl_conf(cfg_kafka: "KafkaConfig") -> dict[str, str]: return conf -def new_kafka_client(cfg_kafka, group_id=None): - """Build a real :class:`KafkaClient` from ``cfg.kafka``. +def new_kafka_client(cluster, group_id=None): + """Build a real :class:`KafkaClient` from a resolved :class:`KafkaCluster`. Test seam: tests monkeypatch this attribute (``monkeypatch.setattr(kafka_commands, "new_kafka_client", factory)``) to return a KafkaClient built with FakeConsumer/FakeProducer, avoiding any real - broker connection. + broker connection. The factory signature takes the resolved ``cluster`` + (not ``cfg.kafka``) so multi-cluster selection (Tasks 2-3) is decoupled + from client construction. """ from ..clients.kafka_client import KafkaClient return KafkaClient( - cfg_kafka.brokers, + cluster.brokers, group_id=group_id, - extra_conf=_kafka_ssl_conf(cfg_kafka), + extra_conf=_kafka_ssl_conf(cluster), ) -def _resolve_timeout(cli_timeout, cfg_kafka_timeout, fallback=30): - """First non-None of (cli, cfg.kafka.timeout_seconds, 30); coerced to float.""" - for candidate in (cli_timeout, cfg_kafka_timeout, fallback): +def _resolve_timeout(cli_timeout, cluster_timeout, fallback=30): + """First non-None of (cli, cluster.timeout_seconds, 30); coerced to float.""" + for candidate in (cli_timeout, cluster_timeout, fallback): if candidate is not None: return float(candidate) return float(fallback) -def _resolve_group(cli_group, cfg_kafka): - """``--consumer-group`` > ``cfg.kafka.default_consumer_group`` > default.""" +def _resolve_group(cli_group, cluster): + """``--consumer-group`` > ``cluster.default_consumer_group`` > default.""" if cli_group is not None: return cli_group - if cfg_kafka.default_consumer_group is not None: - return cfg_kafka.default_consumer_group + if cluster.default_consumer_group is not None: + return cluster.default_consumer_group return "agctl-consumer" @@ -125,7 +164,10 @@ def _kafka_produce_core( value = json.loads(message) headers = parse_params(header) if header else None - client = new_kafka_client(cfg.kafka) + # Resolve the cluster (Task 1: no flag/binding yet -> default/single-cluster). + name = resolve_cluster_name(cfg.kafka, None) + cluster = cfg.kafka.clusters[name] + client = new_kafka_client(cluster) return client.produce(topic, value, key=key, headers=headers or None) @@ -179,7 +221,11 @@ def _kafka_consume_core( ) match_expr = match if match is not None else filter_key - resolved_timeout = _resolve_timeout(timeout, cfg.kafka.timeout_seconds) + # Resolve the cluster (Task 1: no flag/binding yet -> default/single-cluster). + name = resolve_cluster_name(cfg.kafka, None) + cluster = cfg.kafka.clusters[name] + + resolved_timeout = _resolve_timeout(timeout, cluster.timeout_seconds) if resolved_timeout <= 0: # timeout <= 0 makes the poll deadline already-passed, so the loop never # runs and consume returns ok:0 — a silent no-op masquerading as success. @@ -189,7 +235,7 @@ def _kafka_consume_core( ) # D6: default lookback = resolved timeout. resolved_lookback = float(lookback) if lookback is not None else resolved_timeout - group = _resolve_group(consumer_group, cfg.kafka) + group = _resolve_group(consumer_group, cluster) # Build the optional jq filter as an inline predicate so consume_window can # apply it incrementally AND short-circuit as soon as --expect-count matching @@ -205,7 +251,7 @@ def _kafka_consume_core( def predicate(msg, _expr=match_expr): return jq_bool(msg, _expr) - client = new_kafka_client(cfg.kafka, group_id=group) + client = new_kafka_client(cluster, group_id=group) matched = client.consume_window( topic, lookback_seconds=resolved_lookback, @@ -414,11 +460,15 @@ def _kafka_assert_core( {"timeout": resolved_timeout}, ) resolved_lookback = float(lookback) if lookback is not None else resolved_timeout - group = _resolve_group(consumer_group, cfg.kafka) + # Resolve the cluster (Task 1: no flag/binding yet -> default/single-cluster; + # Task 2 passes the pattern's cluster as binding_cluster). + name = resolve_cluster_name(cfg.kafka, None) + cluster = cfg.kafka.clusters[name] + group = _resolve_group(consumer_group, cluster) # DESIGN §9.3: a custom assertion mode evaluates the full consumed window. if assertion is not None: - client = new_kafka_client(cfg.kafka, group_id=group) + client = new_kafka_client(cluster, group_id=group) messages = client.consume_window( inferred_topic, lookback_seconds=resolved_lookback, @@ -459,7 +509,7 @@ def _kafka_assert_core( filled_pattern_match=filled_pattern_match, ) - client = new_kafka_client(cfg.kafka, group_id=group) + client = new_kafka_client(cluster, group_id=group) start = time.monotonic() matched, scanned = client.find_in_window( inferred_topic, diff --git a/agctl/commands/mock_commands.py b/agctl/commands/mock_commands.py index ac2383d..8f7447a 100644 --- a/agctl/commands/mock_commands.py +++ b/agctl/commands/mock_commands.py @@ -31,7 +31,7 @@ __all__ = ["mock_run", "new_mock_engine", "mock_start", "mock_stop", "mock_status"] # Import from kafka_commands to avoid duplication (no circular import) -from .kafka_commands import new_kafka_client +from .kafka_commands import new_kafka_client, resolve_cluster_name # Import daemon lifecycle helpers (Task 2: pidfile, liveness; Task 3: log parser) from ..mock.daemon import ( @@ -469,13 +469,20 @@ def mock_run( # Guard 2+3: Resolve engines to run run_http, run_kafka = _resolve_engines(only, cfg.mocks) - # Guard 5: If run_kafka, require non-empty kafka.brokers + # Guard 5: If run_kafka, resolve a default cluster and require its brokers. + # (Task 1: all reactors share the single default client; per-reactor + # cluster selection lands in Task 3.) kafka_client = None if run_kafka: - if not cfg.kafka.brokers: - raise ConfigError("kafka.brokers is required when running Kafka reactors", {}) + cluster_name = resolve_cluster_name(cfg.kafka, None) + cluster = cfg.kafka.clusters[cluster_name] + if not cluster.brokers: + raise ConfigError( + "kafka.clusters..brokers is required when running Kafka reactors", + {"cluster": cluster_name}, + ) # Build KafkaClient (may raise ConfigError if kafka extra missing) - kafka_client = new_kafka_client(cfg.kafka) + kafka_client = new_kafka_client(cluster) # Guard 6: Resolve http_listen if http_listen is not None: diff --git a/agctl/config/loader.py b/agctl/config/loader.py index bad32f5..63254f6 100644 --- a/agctl/config/loader.py +++ b/agctl/config/loader.py @@ -212,7 +212,7 @@ def discover_config_path(explicit: str | None = None, env: dict[str, str] | None raise ConfigError("No agctl.yaml found (use --config or AGCTL_CONFIG, or add agctl.yaml)", {}) -TOOL_MAJOR_VERSION = "2" +TOOL_MAJOR_VERSION = "3" class ComposedConfig(NamedTuple): @@ -238,7 +238,7 @@ def compose_config( 1. Resolve env (defaults to os.environ). 2. Discover base config path. 3. Load and interpolate base YAML. - 4. Check base version (must be v2). + 4. Check base version (must be v3). 5. For each overlay (in order): - Verify file exists. - Load and interpolate overlay YAML. @@ -349,10 +349,7 @@ def _check_version(data: dict) -> None: else: message = ( f"Config dialect v{major} is no longer supported by agctl v{TOOL_MAJOR_VERSION} " - f"(config_version='{version}'). Run `agctl config migrate` to upgrade, " - f"or manually bump `version: \"{TOOL_MAJOR_VERSION}\"` and prefix each HTTP " - f"`match` expression with `.body | ` and each Kafka `match` expression with " - f"`.value | `." + f"(config_version='{version}'). Run `agctl config migrate` to upgrade." ) raise ConfigError( message, diff --git a/agctl/config/migrate.py b/agctl/config/migrate.py index c373fd6..9a4135e 100644 --- a/agctl/config/migrate.py +++ b/agctl/config/migrate.py @@ -1,37 +1,58 @@ -"""Pure helper that rewrites a v1 config to dialect ``"2"`` (Task 6). +"""Pure helper that rewrites a v1/v2 config to dialect ``"3"``. -``agctl`` v2 made every ``match`` expression envelope-rooted (HTTP ``.body``, -Kafka ``.value``). A v1 config loaded by the v2 tool is rejected by the loader -with a pointer to ``agctl config migrate``; this module performs the actual -rewrite — :func:`migrate_match_exprs` is a pure dict→dict transform with no -file I/O, so the Click command in :mod:`agctl.commands.config_commands` can -drive it for both real runs and ``--dry-run`` previews. +``agctl`` v3 restructured ``Config.kafka`` from a single flat object into a +named map ``kafka.clusters.`` (mirroring ``database.connections``), plus +the version bump ``2 -> 3``. v1 configs additionally need the v2 jq-dialect +rewrite (every ``match`` expression envelope-rooted: HTTP ``.body``, Kafka +``.value``); :func:`migrate_config` carries a v1 input through BOTH the jq +rewrite and the structural lift in one pass. A v2 input needs only the lift +(its match exprs are already envelope-rooted). A config already at v3 is a +clean no-op. -Semantics (DESIGN §3.5; Task 6 brief): +:func:`migrate_config` is a pure dict->dict transform with no file I/O, so the +Click command in :mod:`agctl.commands.config_commands` can drive it for both +real runs and ``--dry-run`` previews. -- ``source_major = str(config.get("version", "")).strip().split(".")[0]`` — the +Semantics (DESIGN §8; Task 1 brief): + +- ``source_major = str(config.get("version", "")).strip().split(".")[0]`` -- the ``.strip()`` mirrors the loader's ``_check_version`` so a loader-accepted - ``version: " 2 "`` is seen as already-v2 here too (not force-rewritten). -- If ``source_major == "2"``: already-v2 — return ``already_v2=True``, deep - copy unchanged, ``rewrites=[]``. A v2-native config may carry a v2-style - expr like ``.body.amount`` (no ``.body | `` prefix); the helper must NOT - prepend to it. -- Otherwise (``"1"``, ``""``, or any legacy value): deep-copy the config, - set ``config["version"] = "2"``, walk the three match-site families and, for - each expression that does NOT already ``startswith`` its transport prefix, - prepend the prefix and record a rewrite: - - * ``mocks.http.stubs..match.jq`` → prefix ``.body | `` + ``version: " 3 "`` is seen as already-current here too (not force-lifted). +- If ``source_major == "3"``: already-current -- return ``already_current=True``, + deep copy unchanged, ``rewrites=[]``. +- Otherwise (``"1"``, ``"2"``, ``""``, or any legacy value): deep-copy the + config, set ``config["version"] = "3"``. Then, **only when source_major is + ``"1"``**, walk the three match-site families and prefix each expression that + does NOT already ``startswith`` its transport prefix: + + * ``mocks.http.stubs..match.jq`` -> prefix ``.body | `` (only when both ``match`` and ``match.jq`` are present). - * ``mocks.kafka.reactors..match`` → prefix ``.value | `` + * ``mocks.kafka.reactors..match`` -> prefix ``.value | `` (only when ``match`` is present and a string). - * ``kafka.patterns..match`` → prefix ``.value | `` + * ``kafka.patterns..match`` -> prefix ``.value | `` (only when ``match`` is present and a string). -Traversal order for ``rewrites``: HTTP stubs (dict order), then Kafka reactors -(dict order), then ``kafka.patterns`` (dict order). ``capture.*.from`` and -``match.body`` are NOT visited (out of scope). Defensive ``.get()`` chains: a -missing section contributes no sites and never raises. + (v2 inputs skip the jq walkers entirely -- their exprs are already + envelope-rooted, and force-prepending would double-prefix v2-native exprs + like ``.body.amount`` into the broken ``.body | .body.amount``.) + +- Then run the structural lift ``_lift_kafka_clusters`` unconditionally (both + v1 and v2 sources may carry a flat ``kafka:`` block): when + ``config["kafka"]`` is a dict containing at least one of the five flat keys + (brokers/ssl/timeout_seconds/default_consumer_group/schema_registry_url) and + NO ``clusters`` key, move those keys into + ``config["kafka"]["clusters"]["default"]`` and set + ``config["kafka"]["default_cluster"] = "default"``, recording one structural + rewrite per lifted key (deterministic order: brokers, ssl, timeout_seconds, + default_consumer_group, schema_registry_url) plus the ``default_cluster`` set. + A missing or already-clustered ``kafka`` contributes no rewrites and never + raises. + +Traversal order for ``rewrites``: jq-prefix rewrites first (HTTP stubs (dict +order), then Kafka reactors (dict order), then ``kafka.patterns`` (dict +order)), then structural rewrites. ``capture.*.from`` and ``match.body`` are +NOT visited (out of scope). Defensive ``.get()`` chains throughout. The +function never mutates its input (deep-copy first). """ from __future__ import annotations @@ -40,36 +61,53 @@ from dataclasses import dataclass, field from typing import Any -#: Prefix prepended to HTTP ``match.jq`` expressions. +#: Prefix prepended to HTTP ``match.jq`` expressions (v1 sources only). HTTP_PREFIX = ".body | " -#: Prefix prepended to Kafka ``match`` expressions. +#: Prefix prepended to Kafka ``match`` expressions (v1 sources only). KAFKA_PREFIX = ".value | " #: Target dialect for any migration. -TO_VERSION = "2" +TO_VERSION = "3" + +#: The five flat ``kafka:`` keys lifted into ``kafka.clusters.``. +#: Order is the deterministic rewrite-emission order. +_FLAT_KAFKA_KEYS = ( + "brokers", + "ssl", + "timeout_seconds", + "default_consumer_group", + "schema_registry_url", +) + +#: Default cluster name assigned to a lifted flat ``kafka:`` block. +_DEFAULT_CLUSTER_NAME = "default" @dataclass class MigrateResult: - """Result of :func:`migrate_match_exprs`. + """Result of :func:`migrate_config`. Attributes: - config: A deep-copied, transformed config (always a fresh dict — the - input is never mutated). When ``already_v2`` is True, this is a - deep copy of the input unchanged. + config: A deep-copied, transformed config (always a fresh dict -- the + input is never mutated). When ``already_current`` is True, this is + a deep copy of the input unchanged. from_version: The original raw ``version`` value (or ``""`` if absent). - to_version: Always ``"2"``. - rewrites: Per-site rewrite records, in traversal order, each - ``{"path": str, "before": str, "after": str}``. Empty when - ``already_v2`` is True. - already_v2: True iff ``source_major == "2"`` (no migration performed). + to_version: Always ``"3"``. + rewrites: Per-site rewrite records, in traversal order. jq-prefix + rewrites are ``{"path": str, "before": str, "after": str}``; + structural rewrites are + ``{"path": "kafka.clusters..", "before": , + "after": }`` (plus the ``default_cluster`` set record). + Empty when ``already_current`` is True. + already_current: True iff ``source_major == "3"`` (no migration + performed). """ config: dict[str, Any] from_version: str to_version: str = TO_VERSION - rewrites: list[dict[str, str]] = field(default_factory=list) - already_v2: bool = False + rewrites: list[dict[str, Any]] = field(default_factory=list) + already_current: bool = False def _prepend(expr: str, prefix: str) -> str: @@ -139,16 +177,63 @@ def _migrate_kafka_patterns( rewrites.append({"path": path, "before": before, "after": after}) -def migrate_match_exprs(config: dict[str, Any]) -> MigrateResult: - """Rewrite a v1 config to dialect ``"2"`` (pure dict→dict transform). +def _lift_kafka_clusters( + config: dict[str, Any], rewrites: list[dict[str, Any]] +) -> None: + """Lift a flat ``kafka:`` block into ``kafka.clusters.``. + + Fires only when ``config["kafka"]`` is a dict holding at least one of the + five flat keys and NO ``clusters`` key. Records one structural rewrite per + lifted key (deterministic order) plus the ``default_cluster`` set. A + missing or already-clustered ``kafka`` contributes nothing and never + raises. + """ + kafka = config.get("kafka") + if not isinstance(kafka, dict): + return + if "clusters" in kafka: + return # already clustered -- never double-lift + if not any(key in kafka for key in _FLAT_KAFKA_KEYS): + return # nothing flat to lift (e.g. only patterns) + + kafka.setdefault("clusters", {}) + cluster: dict[str, Any] = {} + kafka["clusters"][_DEFAULT_CLUSTER_NAME] = cluster + + for key in _FLAT_KAFKA_KEYS: + if key in kafka: + before = kafka[key] + cluster[key] = before + del kafka[key] + rewrites.append( + { + "path": f"kafka.clusters.{_DEFAULT_CLUSTER_NAME}.{key}", + "before": before, + "after": before, + } + ) + + before_default = kafka.get("default_cluster") + kafka["default_cluster"] = _DEFAULT_CLUSTER_NAME + rewrites.append( + { + "path": "kafka.default_cluster", + "before": before_default, + "after": _DEFAULT_CLUSTER_NAME, + } + ) + + +def migrate_config(config: dict[str, Any]) -> MigrateResult: + """Rewrite a v1/v2 config to dialect ``"3"`` (pure dict->dict transform). See the module docstring for the full contract. Returns a - :class:`MigrateResult` whose ``config`` is always a fresh deep copy — the + :class:`MigrateResult` whose ``config`` is always a fresh deep copy -- the caller's input is never mutated. """ # Match the loader's _check_version exactly: str(...).strip(). Without the - # strip, a loader-accepted `version: " 2 "` reads as source_major " 2 " and - # force-rewrites a v2-native config (double-prefixing exprs like .body.amount). + # strip, a loader-accepted `version: " 3 "` reads as source_major " 3 " and + # would force-lift an already-v3 config. from_version = str(config.get("version", "")).strip() source_major = from_version.split(".")[0] if source_major == TO_VERSION: @@ -157,21 +242,30 @@ def migrate_match_exprs(config: dict[str, Any]) -> MigrateResult: from_version=from_version, to_version=TO_VERSION, rewrites=[], - already_v2=True, + already_current=True, ) new_config = copy.deepcopy(config) new_config["version"] = TO_VERSION - rewrites: list[dict[str, str]] = [] - # Traversal order per the brief: HTTP stubs → Kafka reactors → kafka.patterns. - _migrate_http_stubs(new_config, rewrites) - _migrate_kafka_reactors(new_config, rewrites) - _migrate_kafka_patterns(new_config, rewrites) + rewrites: list[dict[str, Any]] = [] + + # jq-prefix walkers run ONLY on v1 sources: v2+ exprs are already + # envelope-rooted, so force-prepending would double-prefix them. + if source_major == "1": + # Traversal order per the brief: HTTP stubs -> Kafka reactors -> kafka.patterns. + _migrate_http_stubs(new_config, rewrites) + _migrate_kafka_reactors(new_config, rewrites) + _migrate_kafka_patterns(new_config, rewrites) + + # Structural lift runs for any non-current source (v1 and v2 both may carry + # a flat kafka: block). Defensive: a missing/already-clustered kafka is a + # no-op. + _lift_kafka_clusters(new_config, rewrites) return MigrateResult( config=new_config, from_version=from_version, to_version=TO_VERSION, rewrites=rewrites, - already_v2=False, + already_current=False, ) diff --git a/agctl/config/models.py b/agctl/config/models.py index b94cb61..55fcd53 100644 --- a/agctl/config/models.py +++ b/agctl/config/models.py @@ -74,6 +74,9 @@ class KafkaPattern(BaseModel): description: str | None = None topic: str match: str | None = None + # Named cluster this pattern binds to (DESIGN §6, consumed in Tasks 2-3). + # None -> resolved via default_cluster / single-cluster auto-default. + cluster: str | None = None class KafkaSSL(BaseModel): @@ -110,13 +113,34 @@ def _check_security_protocol(cls, v: str | None) -> str | None: return upper -class KafkaConfig(BaseModel): +class KafkaCluster(BaseModel): + """A named Kafka cluster's broker configuration (DESIGN §6, v3 schema). + + Holds the per-cluster knobs formerly on ``KafkaConfig`` (brokers / TLS / + timeout / consumer group / schema registry). Mirrors + :class:`DatabaseConnection`: a named entry in ``kafka.clusters.``, + selected by name via ``resolve_cluster_name``. + """ + brokers: list[str] = Field(default_factory=list) + ssl: KafkaSSL | None = None + timeout_seconds: int | None = None default_consumer_group: str | None = None schema_registry_url: str | None = None - timeout_seconds: int | None = None + + +class KafkaConfig(BaseModel): + """Top-level Kafka config (v3 schema). + + ``clusters`` is a named map (mirroring ``database.connections``); + ``default_cluster`` names the cluster used when no flag/binding selects one + (required only when >1 cluster is defined, per single-cluster auto-default); + ``patterns`` is a global cluster-aware map. + """ + + clusters: dict[str, KafkaCluster] = Field(default_factory=dict) + default_cluster: str | None = None patterns: dict[str, KafkaPattern] = Field(default_factory=dict) - ssl: KafkaSSL | None = None class DatabaseConnection(BaseModel): @@ -262,6 +286,9 @@ class KafkaReactor(BaseModel): match: str | None = None capture: dict[str, CaptureSpec] | None = None reaction: KafkaReaction + # Named cluster this reactor binds to (DESIGN §7, consumed in Task 3). + # None -> resolved via default_cluster / single-cluster auto-default. + cluster: str | None = None class KafkaMockConfig(BaseModel): diff --git a/agctl/config/validator.py b/agctl/config/validator.py index 5a7192c..90f4a46 100644 --- a/agctl/config/validator.py +++ b/agctl/config/validator.py @@ -85,7 +85,7 @@ def validate_config(cfg: Config) -> tuple[list[dict], list[dict]]: } ) - # §3.6 — Kafka pattern missing-description warnings. + # §3.6 — Kafka pattern missing-description warnings + cluster dangling refs. for name, pattern in cfg.kafka.patterns.items(): if _missing_description(pattern.description): warnings.append( @@ -94,22 +94,63 @@ def validate_config(cfg: Config) -> tuple[list[dict], list[dict]]: "message": "missing description (discovery degrades without it)", } ) + # KafkaPattern.cluster dangling ref (Task 2 consumes the field). + if pattern.cluster is not None and pattern.cluster not in cfg.kafka.clusters: + errors.append( + { + "path": f"kafka.patterns.{name}.cluster", + "message": ( + f"Pattern references unknown cluster '{pattern.cluster}'" + ), + } + ) + + # kafka.default_cluster dangling ref (DESIGN §3.5 dangling refs, v3). + if ( + cfg.kafka.default_cluster is not None + and cfg.kafka.default_cluster not in cfg.kafka.clusters + ): + errors.append( + { + "path": "kafka.default_cluster", + "message": ( + f"Default references unknown cluster " + f"'{cfg.kafka.default_cluster}'" + ), + } + ) # --- mock server validation ----------------------------------------------- - # Check 1: mocks.kafka requires kafka.brokers + # Check 1: mocks.kafka reactors require a resolvable default cluster with + # non-empty brokers. Resolution mirrors resolve_cluster_name (default_cluster + # -> single-cluster auto-default) but is inlined here so config/ stays free + # of a commands/ import. (Reactor.cluster dangling-ref is deferred to Task 3 + # where it is consumed.) if ( cfg.mocks is not None and cfg.mocks.kafka is not None and cfg.mocks.kafka.reactors - and not cfg.kafka.brokers ): - errors.append( - { - "path": "mocks.kafka", - "message": "kafka mocks require top-level kafka.brokers", - } - ) + reactor_cluster = cfg.kafka.default_cluster + if reactor_cluster is None and len(cfg.kafka.clusters) == 1: + reactor_cluster = next(iter(cfg.kafka.clusters)) + if reactor_cluster is None or reactor_cluster not in cfg.kafka.clusters: + errors.append( + { + "path": "mocks.kafka", + "message": "reactors require a resolvable default cluster", + } + ) + elif not cfg.kafka.clusters[reactor_cluster].brokers: + errors.append( + { + "path": "mocks.kafka", + "message": ( + f"kafka mocks require kafka.clusters.{reactor_cluster}.brokers" + ), + } + ) # Check 2: Missing description warnings for stubs and reactors if cfg.mocks is not None: diff --git a/agctl/data/sample-config.yaml b/agctl/data/sample-config.yaml index d4b2d30..bc89543 100644 --- a/agctl/data/sample-config.yaml +++ b/agctl/data/sample-config.yaml @@ -1,6 +1,6 @@ # agctl.yaml -# Version tracks the agctl MAJOR version only (currently "2"). -version: "2" +# Version tracks the agctl MAJOR version only (currently "3"). +version: "3" # --- services: named HTTP base URLs for services under test ----------------- services: @@ -14,31 +14,39 @@ services: health_path: "/health" timeout_seconds: 15 -# --- kafka: broker config -------------------------------------------------- +# --- kafka: named clusters + patterns -------------------------------------- +# Mirrors database.connections: a named map of clusters, a default_cluster, +# and a global patterns map. A single cluster needs no default_cluster +# (auto-defaulted), but it is set here for clarity. kafka: - brokers: - - "localhost:9092" - default_consumer_group: "agctl-consumer" - schema_registry_url: "" # optional; omit/leave empty if unused - timeout_seconds: 30 # default consume/assert timeout - - # Optional TLS/mTLS — uncomment for brokers that require SSL. Setting ANY - # field to a non-empty value enables TLS (security.protocol defaults to "SSL"). - # ca_location is optional: unset → librdkafka uses the system trust store - # (fine for publicly-trusted brokers; pin a CA for private-PKI brokers). - # Hostname verification stays ON unless endpoint_identification_algorithm: "none". - # ssl: - # ca_location: "" - # certificate_location: "" # path to client cert (mTLS) - # key_location: "" # path to client private key (mTLS) - # key_password: "" # optional private-key password - # # endpoint_identification_algorithm: "none" # disable hostname verification - # # security_protocol: "SSL" # default; set SASL_SSL when adding SASL + clusters: + default: + brokers: + - "localhost:9092" + default_consumer_group: "agctl-consumer" + schema_registry_url: "" # optional; omit/leave empty if unused + timeout_seconds: 30 # default consume/assert timeout + + # Optional TLS/mTLS — uncomment for brokers that require SSL. Setting ANY + # field to a non-empty value enables TLS (security.protocol defaults to "SSL"). + # ca_location is optional: unset → librdkafka uses the system trust store + # (fine for publicly-trusted brokers; pin a CA for private-PKI brokers). + # Hostname verification stays ON unless endpoint_identification_algorithm: "none". + # ssl: + # ca_location: "" + # certificate_location: "" # path to client cert (mTLS) + # key_location: "" # path to client private key (mTLS) + # key_password: "" # optional private-key password + # # endpoint_identification_algorithm: "none" # disable hostname verification + # # security_protocol: "SSL" # default; set SASL_SSL when adding SASL + + default_cluster: default # patterns: named Kafka filters, analogous to HTTP templates. # topic: Kafka topic # match: jq boolean predicate over each message envelope; # supports {placeholder} substitution via --param at assert time + # cluster: optional named cluster this pattern binds to (default = default_cluster) patterns: order-created: description: "An ORDER_CREATED event for a specific order" diff --git a/tests/fixtures/agctl.yaml b/tests/fixtures/agctl.yaml index 474992d..e3a4882 100644 --- a/tests/fixtures/agctl.yaml +++ b/tests/fixtures/agctl.yaml @@ -1,4 +1,4 @@ -version: "2" +version: "3" services: order-service: @@ -12,21 +12,26 @@ services: timeout_seconds: 15 kafka: - brokers: - - "${KAFKA_BROKER}" - default_consumer_group: "agctl-consumer" - schema_registry_url: "${SCHEMA_REGISTRY_URL:-}" - timeout_seconds: 30 - # Optional TLS/mTLS — uncomment for brokers that require SSL. Setting any - # field enables TLS (security.protocol defaults to "SSL"). Env override: - # AGCTL_KAFKA__SSL__CA_LOCATION, ...__CERTIFICATE_LOCATION, etc. - # ssl: - # ca_location: "${KAFKA_SSL_CA:-}" - # certificate_location: "${KAFKA_SSL_CERT:-}" - # key_location: "${KAFKA_SSL_KEY:-}" - # key_password: "${KAFKA_SSL_KEY_PASSWORD:-}" - # # endpoint_identification_algorithm: "none" # disable hostname verification - # # security_protocol: "SSL" # default SSL; SASL_SSL when adding SASL + clusters: + default: + brokers: + - "${KAFKA_BROKER}" + default_consumer_group: "agctl-consumer" + schema_registry_url: "${SCHEMA_REGISTRY_URL:-}" + timeout_seconds: 30 + # Optional TLS/mTLS — uncomment for brokers that require SSL. Setting any + # field enables TLS (security.protocol defaults to "SSL"). Env override: + # AGCTL_KAFKA__CLUSTERS__DEFAULT__SSL__CA_LOCATION, etc. + # ssl: + # ca_location: "${KAFKA_SSL_CA:-}" + # certificate_location: "${KAFKA_SSL_CERT:-}" + # key_location: "${KAFKA_SSL_KEY:-}" + # key_password: "${KAFKA_SSL_KEY_PASSWORD:-}" + # # endpoint_identification_algorithm: "none" # disable hostname verification + # # security_protocol: "SSL" # default SSL; SASL_SSL when adding SASL + + default_cluster: default + patterns: order-created: description: "An ORDER_CREATED event for a specific order" diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 793fc73..7db3aa8 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -52,7 +52,7 @@ def test_validate_structural_error_envelope(tmp_path): cfg_path = _write_config( tmp_path, """ -version: "2" +version: "3" services: order-service: base_url: "http://localhost:8081" @@ -121,18 +121,20 @@ def test_show_does_not_mask_ssl_key_path(tmp_path): 'key' fragment in _is_secret must not match the key_* prefix.""" cfg_path = _write_config( tmp_path, - 'version: "2"\n' + 'version: "3"\n' "kafka:\n" - " brokers: [host:9092]\n" - " ssl:\n" - " ca_location: /etc/ssl/ca.pem\n" - " certificate_location: /etc/ssl/client.crt\n" - " key_location: /etc/ssl/client.key\n" - " key_password: hunter2\n", + " clusters:\n" + " default:\n" + " brokers: [host:9092]\n" + " ssl:\n" + " ca_location: /etc/ssl/ca.pem\n" + " certificate_location: /etc/ssl/client.crt\n" + " key_location: /etc/ssl/client.key\n" + " key_password: hunter2\n", ) result = CliRunner().invoke(cli, ["config", "show", "--config", str(cfg_path)]) payload = json.loads(result.output) - ssl = payload["result"]["kafka"]["ssl"] + ssl = payload["result"]["kafka"]["clusters"]["default"]["ssl"] assert ssl["ca_location"] == "/etc/ssl/ca.pem" # path, not masked assert ssl["certificate_location"] == "/etc/ssl/client.crt" assert ssl["key_location"] == "/etc/ssl/client.key" # path, NOT masked diff --git a/tests/unit/test_config_commands.py b/tests/unit/test_config_commands.py index 73beb98..6b0885f 100644 --- a/tests/unit/test_config_commands.py +++ b/tests/unit/test_config_commands.py @@ -37,7 +37,7 @@ def test_validate_malformed_http_stub_jq_exits_2(tmp_path): ``mocks.http.stubs..match.jq`` (the jq-compile error surfaced by collect_jq_compile_errors).""" yaml_text = """ -version: "2" +version: "3" mocks: http: stubs: @@ -65,10 +65,13 @@ def test_validate_malformed_kafka_reactor_match_exits_2(tmp_path): """A Kafka reactor whose match is malformed -> exit 2 and an error whose path is ``mocks.kafka.reactors..match``.""" yaml_text = """ -version: "2" +version: "3" kafka: - brokers: - - localhost:9092 + clusters: + default: + brokers: + - localhost:9092 + default_cluster: default mocks: kafka: reactors: @@ -95,10 +98,13 @@ def test_validate_fully_valid_config_exits_0(tmp_path): """A config with a well-formed match.jq and reactor match -> exit 0, ``valid: true`` (no jq-compile errors).""" yaml_text = """ -version: "2" +version: "3" kafka: - brokers: - - localhost:9092 + clusters: + default: + brokers: + - localhost:9092 + default_cluster: default mocks: http: stubs: @@ -132,7 +138,7 @@ def test_validate_fully_valid_config_exits_0(tmp_path): def test_validate_no_mocks_section_exits_0(tmp_path): """A config with no ``mocks`` section -> exit 0 (collector returns []).""" - result = _validate(tmp_path, 'version: "2"\n') + result = _validate(tmp_path, 'version: "3"\n') assert result.exit_code == 0 payload = json.loads(result.output) assert payload["result"]["valid"] is True @@ -144,7 +150,7 @@ def test_validate_no_mocks_section_exits_0(tmp_path): def test_validate_override_warning_emitted(tmp_path): """Scenario 1: Override warning emitted when overlay overrides templates.create-order.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -182,7 +188,7 @@ def test_validate_override_warning_emitted(tmp_path): def test_validate_no_override_no_warning(tmp_path): """Scenario 2: No override warning when overlay only adds a new template.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -215,7 +221,7 @@ def test_validate_no_override_no_warning(tmp_path): def test_validate_cross_file_dangling_ref_error(tmp_path): """Scenario 3: Cross-file dangling ref is still an error.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -248,7 +254,7 @@ def test_validate_cross_file_dangling_ref_error(tmp_path): def test_validate_global_overlay_form_threads(tmp_path): """Scenario 4: Global --overlay form threads to config validate (same as scenario 1).""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 diff --git a/tests/unit/test_db_commands.py b/tests/unit/test_db_commands.py index f7c800d..0b875f2 100644 --- a/tests/unit/test_db_commands.py +++ b/tests/unit/test_db_commands.py @@ -1195,7 +1195,7 @@ def test_db_schema_no_default_no_connection_raises_config_error(install_fake, tm install_fake(schema_result={"items": []}) cfg = tmp_path / "agctl.yaml" cfg.write_text( - 'version: "2"\n' + 'version: "3"\n' "database:\n" " connections:\n" " main-db:\n" diff --git a/tests/unit/test_discover_command.py b/tests/unit/test_discover_command.py index c245207..d15da1b 100644 --- a/tests/unit/test_discover_command.py +++ b/tests/unit/test_discover_command.py @@ -477,7 +477,7 @@ def test_search_finds_mock_kafka_reactor(monkeypatch): _NO_MOCKS_CONFIG = ( - 'version: "2"\n' + 'version: "3"\n' "services:\n" " demo:\n" ' base_url: "http://localhost:9999"\n' @@ -501,7 +501,7 @@ def test_category_mock_http_stubs_absent_is_empty(tmp_path, monkeypatch): _CAPTURE_CONFIG = ( - 'version: "2"\n' + 'version: "3"\n' "services:\n" " demo:\n" ' base_url: "http://localhost:9999"\n' @@ -539,7 +539,7 @@ def test_item_mock_http_stub_with_capture(tmp_path, monkeypatch): _IPV6_CONFIG = ( - 'version: "2"\n' + 'version: "3"\n' "services:\n" " demo:\n" ' base_url: "http://localhost:9999"\n' @@ -588,7 +588,7 @@ def test_item_mock_http_stub_ipv6_wildcard_normalized(tmp_path, monkeypatch): _BASE_CONFIG = ( - 'version: "2"\n' + 'version: "3"\n' "services:\n" " demo:\n" ' base_url: "http://localhost:9999"\n' @@ -612,7 +612,7 @@ def test_item_mock_http_stub_ipv6_wildcard_normalized(tmp_path, monkeypatch): _OVERLAY_CONFIG = ( - 'version: "2"\n' + 'version: "3"\n' "templates:\n" " overlay-template:\n" ' description: "Overlay template"\n' @@ -783,7 +783,7 @@ def test_discover_item_log_sources_with_schema(tmp_path, monkeypatch): # Create a temp config with a logs source pointing at the temp file config_yaml = ( - 'version: "2"\n' + 'version: "3"\n' "services:\n" ' demo:\n' ' base_url: "http://localhost:9999"\n' diff --git a/tests/unit/test_kafka_commands.py b/tests/unit/test_kafka_commands.py index bffe644..89f76d7 100644 --- a/tests/unit/test_kafka_commands.py +++ b/tests/unit/test_kafka_commands.py @@ -17,9 +17,10 @@ from agctl.cli import cli from agctl.clients.kafka_client import KafkaClient -from agctl.config.models import KafkaConfig, KafkaSSL +from agctl.config.models import KafkaCluster, KafkaConfig, KafkaSSL from agctl.assertion_registry import Assertion from agctl.commands import kafka_commands +from agctl.errors import ConfigError FIXTURE = Path(__file__).parent.parent / "fixtures" / "agctl.yaml" @@ -44,19 +45,20 @@ # --------------------------------------------------------------------------- -def _kcfg(**ssl_kwargs): - return KafkaConfig(ssl=KafkaSSL(**ssl_kwargs)) if ssl_kwargs else KafkaConfig() +def _cluster(**ssl_kwargs): + """Build a KafkaCluster carrying the given ssl knobs (or none).""" + return KafkaCluster(ssl=KafkaSSL(**ssl_kwargs)) if ssl_kwargs else KafkaCluster() def test_ssl_conf_none_when_no_ssl(): - assert kafka_commands._kafka_ssl_conf(KafkaConfig()) == {} + assert kafka_commands._kafka_ssl_conf(KafkaCluster()) == {} # An empty ssl block also yields nothing (no knobs -> no protocol inferred). - assert kafka_commands._kafka_ssl_conf(_kcfg()) == {} + assert kafka_commands._kafka_ssl_conf(_cluster()) == {} def test_ssl_conf_full_emits_all_keys_with_inferred_protocol(): conf = kafka_commands._kafka_ssl_conf( - _kcfg( + _cluster( ca_location="/ca.pem", certificate_location="/client.crt", key_location="/client.key", @@ -77,13 +79,13 @@ def test_ssl_conf_full_emits_all_keys_with_inferred_protocol(): def test_ssl_conf_explicit_security_protocol_honored(): conf = kafka_commands._kafka_ssl_conf( - _kcfg(ca_location="/ca.pem", security_protocol="SASL_SSL") + _cluster(ca_location="/ca.pem", security_protocol="SASL_SSL") ) assert conf["security.protocol"] == "SASL_SSL" def test_ssl_conf_partial_emits_only_set_keys(): - conf = kafka_commands._kafka_ssl_conf(_kcfg(ca_location="/ca.pem")) + conf = kafka_commands._kafka_ssl_conf(_cluster(ca_location="/ca.pem")) # Only the CA knob plus the inferred protocol. assert conf == {"ssl.ca.location": "/ca.pem", "security.protocol": "SSL"} @@ -93,7 +95,7 @@ def test_ssl_conf_skips_empty_string_values(): emit bogus ssl.*.location paths nor disable hostname verification via an empty endpoint_identification_algorithm (librdkafka treats "" == "none").""" conf = kafka_commands._kafka_ssl_conf( - _kcfg( + _cluster( ca_location="/ca.pem", certificate_location="", key_location="", @@ -109,7 +111,7 @@ def test_ssl_conf_all_empty_strings_is_noop(): """A fully-unresolved ssl block (every field "") enables no TLS at all — no empty paths, no inferred security.protocol.""" conf = kafka_commands._kafka_ssl_conf( - _kcfg( + _cluster( ca_location="", certificate_location="", key_location="", @@ -316,7 +318,7 @@ def _install(messages, producer=None): captured["producer"] = producer captured["consumer"] = consumer - def factory(cfg_kafka, group_id=None): + def factory(cluster, group_id=None): # Preserve the test's canned messages; honor a caller group_id by # simply returning the same client (group has no effect on fakes). return client @@ -652,7 +654,7 @@ def test_kafka_consume_nonexistent_topic_is_connection_error(monkeypatch): consumer = FakeConsumer({}, messages=[], empty_assignment=True) client = KafkaClient(["host:9092"], consumer_factory=lambda conf: consumer) monkeypatch.setattr( - kafka_commands, "new_kafka_client", lambda cfg_kafka, group_id=None: client + kafka_commands, "new_kafka_client", lambda cluster, group_id=None: client ) result = _run( @@ -1097,3 +1099,103 @@ def test_kafka_assert_custom_mode_mutually_exclusive_with_match(install_fake): assert result.exit_code == 2 assert payload["error"]["type"] == "ConfigError" + + +# --------------------------------------------------------------------------- # +# v3 cluster resolution (Task 1: default / single-cluster path) +# --cluster flag and per-pattern bindings land in Task 2; here the resolution +# helper (the Task 1 contract) is exercised directly for the error cases that +# need an explicit name, and via the CLI for the no-flag paths. +# --------------------------------------------------------------------------- # + + +def test_kafka_produce_single_cluster_no_flag(install_fake): + """A config with one cluster and no --cluster flag produces successfully — + proving default/single-cluster resolution keeps the common case flagless.""" + cap = install_fake([]) + result = _run( + [ + "--config", str(FIXTURE), + "kafka", "produce", + "--topic", "t", + "--message", '{"a":1}', + ] + ) + payload = _payload(result) + assert result.exit_code == 0 + assert payload["ok"] is True + # The fake producer received the produce call. + assert cap["producer"].calls[0]["topic"] == "t" + + +def test_kafka_consume_no_default_multi_cluster_error(install_fake, tmp_path): + """A config with two clusters, no default_cluster, and no --cluster flag + cannot resolve a cluster -> ConfigError (exit 2), message names the gap.""" + install_fake([]) + cfg = tmp_path / "agctl.yaml" + cfg.write_text( + 'version: "3"\n' + "kafka:\n" + " clusters:\n" + " a:\n" + " brokers: [ha:9092]\n" + " b:\n" + " brokers: [hb:9092]\n" + ) + result = _run( + ["--config", str(cfg), "kafka", "consume", "--topic", "t", "--timeout", "0.02"], + env={}, + ) + payload = _payload(result) + assert result.exit_code == 2 + assert payload["error"]["type"] == "ConfigError" + assert "No kafka cluster specified" in payload["error"]["message"] + + +def test_resolve_cluster_name_explicit_wins(): + """Precedence: explicit > binding > default > single-cluster.""" + k = KafkaConfig( + clusters={ + "a": KafkaCluster(brokers=["ha:9092"]), + "b": KafkaCluster(brokers=["hb:9092"]), + }, + default_cluster="a", + ) + assert kafka_commands.resolve_cluster_name(k, "b") == "b" + # binding_cluster beats default + assert kafka_commands.resolve_cluster_name(k, None, "b") == "b" + # default when no explicit/binding + assert kafka_commands.resolve_cluster_name(k, None) == "a" + + +def test_resolve_cluster_name_single_cluster_auto_default(): + """One cluster defined, no default_cluster -> that cluster auto-resolves.""" + k = KafkaConfig(clusters={"only": KafkaCluster(brokers=["h:9092"])}) + assert kafka_commands.resolve_cluster_name(k, None) == "only" + + +def test_resolve_cluster_name_unknown_cluster_errors(): + """A resolved name absent from clusters -> ConfigError with detail.cluster + (the Task 1 contract; the CLI --cluster wiring lands in Task 2).""" + k = KafkaConfig( + clusters={"a": KafkaCluster(brokers=["ha:9092"])}, + default_cluster="a", + ) + with pytest.raises(ConfigError) as exc: + kafka_commands.resolve_cluster_name(k, "ghost") + assert "Unknown kafka cluster: ghost" in exc.value.message + assert exc.value.detail["cluster"] == "ghost" + + +def test_resolve_cluster_name_no_cluster_specified_errors(): + """No explicit/binding/default and >1 cluster -> ConfigError.""" + k = KafkaConfig( + clusters={ + "a": KafkaCluster(brokers=["ha:9092"]), + "b": KafkaCluster(brokers=["hb:9092"]), + } + ) + with pytest.raises(ConfigError) as exc: + kafka_commands.resolve_cluster_name(k, None) + assert exc.value.message == "No kafka cluster specified" + assert exc.value.detail == {} diff --git a/tests/unit/test_loader.py b/tests/unit/test_loader.py index 6552aa6..b448293 100644 --- a/tests/unit/test_loader.py +++ b/tests/unit/test_loader.py @@ -31,7 +31,9 @@ def test_load_full_config_keeps_connections_and_templates(): assert cfg.database.connections["main-db"].host == "h" assert cfg.database.templates["find-order"].connection == "main-db" assert cfg.services["order-service"].base_url == "http://localhost:8081" - assert cfg.kafka.schema_registry_url == "" # ${VAR:-} resolved to empty + # schema_registry_url now lives on the cluster (v3), ${VAR:-} -> empty. + assert cfg.kafka.clusters["default"].schema_registry_url == "" + assert cfg.kafka.default_cluster == "default" def test_missing_required_env_raises(tmp_path): @@ -50,10 +52,22 @@ def test_version_mismatch_raises(tmp_path): bad.write_text('version: "1"\n') with pytest.raises(ConfigError) as exc: load_config(str(bad), env={}) - assert exc.value.detail["tool_major"] == "2" + assert exc.value.detail["tool_major"] == "3" assert "config migrate" in exc.value.message +def test_v2_config_rejected_pointing_at_migrate(tmp_path): + """A v2 config is rejected under the v3 tool, with the message pointing at + `agctl config migrate` (structural lift).""" + bad = tmp_path / "agctl.yaml" + bad.write_text('version: "2"\nkafka:\n brokers: [h:9092]\n') + with pytest.raises(ConfigError) as exc: + load_config(str(bad), env={}) + assert exc.value.detail["tool_major"] == "3" + assert "config migrate" in exc.value.message + assert "v2" in exc.value.message + + def test_missing_version_raises_with_clear_message(tmp_path): """A config with no `version` field gets a clear missing-version message (not the awkward `Config dialect v is no longer supported ...`).""" @@ -61,7 +75,7 @@ def test_missing_version_raises_with_clear_message(tmp_path): bad.write_text('kafka:\n brokers: [h:9092]\n') with pytest.raises(ConfigError) as exc: load_config(str(bad), env={}) - assert exc.value.detail["tool_major"] == "2" + assert exc.value.detail["tool_major"] == "3" assert "missing a `version`" in exc.value.message @@ -79,70 +93,81 @@ def test_bare_version_key_treated_as_missing(tmp_path): def test_invalid_schema_raises(tmp_path): bad = tmp_path / "agctl.yaml" - bad.write_text('version: "2"\ntemplates:\n x:\n method: GET\n') # missing service/path + bad.write_text('version: "3"\ntemplates:\n x:\n method: GET\n') # missing service/path with pytest.raises(ConfigError): load_config(str(bad), env={}) def test_kafka_ssl_loaded_from_config(tmp_path): - """kafka.ssl sub-block loads into the typed KafkaSSL model (DESIGN §2.1).""" + """kafka.clusters..ssl sub-block loads into the typed KafkaSSL model + (DESIGN §2.1, v3 shape).""" cfg_file = tmp_path / "agctl.yaml" cfg_file.write_text( - 'version: "2"\n' + 'version: "3"\n' "kafka:\n" - " brokers: [host:9092]\n" - " ssl:\n" - " ca_location: /etc/ssl/kafka/ca.pem\n" - " certificate_location: /etc/ssl/kafka/client.crt\n" - " key_location: /etc/ssl/kafka/client.key\n" - " key_password: hunter2\n" - " endpoint_identification_algorithm: none\n" + " clusters:\n" + " default:\n" + " brokers: [host:9092]\n" + " ssl:\n" + " ca_location: /etc/ssl/kafka/ca.pem\n" + " certificate_location: /etc/ssl/kafka/client.crt\n" + " key_location: /etc/ssl/kafka/client.key\n" + " key_password: hunter2\n" + " endpoint_identification_algorithm: none\n" ) cfg = load_config(str(cfg_file), env={}) - assert cfg.kafka.ssl is not None - assert cfg.kafka.ssl.ca_location == "/etc/ssl/kafka/ca.pem" - assert cfg.kafka.ssl.certificate_location == "/etc/ssl/kafka/client.crt" - assert cfg.kafka.ssl.key_location == "/etc/ssl/kafka/client.key" - assert cfg.kafka.ssl.key_password == "hunter2" - assert cfg.kafka.ssl.endpoint_identification_algorithm == "none" + ssl = cfg.kafka.clusters["default"].ssl + assert ssl is not None + assert ssl.ca_location == "/etc/ssl/kafka/ca.pem" + assert ssl.certificate_location == "/etc/ssl/kafka/client.crt" + assert ssl.key_location == "/etc/ssl/kafka/client.key" + assert ssl.key_password == "hunter2" + assert ssl.endpoint_identification_algorithm == "none" # security_protocol is optional and defaults to None (inferred at use time). - assert cfg.kafka.ssl.security_protocol is None + assert ssl.security_protocol is None def test_kafka_ssl_env_override(tmp_path): - """AGCTL_KAFKA__SSL__* overrides reach the typed model for free (§8).""" + """AGCTL_KAFKA__CLUSTERS____SSL__* overrides reach the typed model + for free (§8) under the v3 clusters shape.""" cfg_file = tmp_path / "agctl.yaml" cfg_file.write_text( - 'version: "2"\n' + 'version: "3"\n' "kafka:\n" - " brokers: [host:9092]\n" + " clusters:\n" + " default:\n" + " brokers: [host:9092]\n" ) cfg = load_config( str(cfg_file), - env={"AGCTL_KAFKA__SSL__CA_LOCATION": "/override/ca.pem"}, + env={"AGCTL_KAFKA__CLUSTERS__DEFAULT__SSL__CA_LOCATION": "/override/ca.pem"}, ) - assert cfg.kafka.ssl is not None - assert cfg.kafka.ssl.ca_location == "/override/ca.pem" + ssl = cfg.kafka.clusters["default"].ssl + assert ssl is not None + assert ssl.ca_location == "/override/ca.pem" def test_kafka_ssl_path_interpolated(tmp_path): - """${VAR} interpolation resolves into nested kafka.ssl fields (the documented - primary TLS config mechanism); ${VAR:-} resolves to empty (== unset).""" + """${VAR} interpolation resolves into nested cluster ssl fields (the + documented primary TLS config mechanism); ${VAR:-} resolves to empty.""" cfg_file = tmp_path / "agctl.yaml" cfg_file.write_text( - 'version: "2"\n' + 'version: "3"\n' "kafka:\n" - " brokers: [host:9092]\n" - " ssl:\n" - ' ca_location: "${KAFKA_SSL_CA}"\n' # bare var, required - ' key_password: "${KAFKA_SSL_KEY_PASSWORD:-}"\n' # default-to-empty + " clusters:\n" + " default:\n" + " brokers: [host:9092]\n" + " ssl:\n" + ' ca_location: "${KAFKA_SSL_CA}"\n' # bare var, required + ' key_password: "${KAFKA_SSL_KEY_PASSWORD:-}"\n' # default-to-empty ) cfg = load_config(str(cfg_file), env={"KAFKA_SSL_CA": "/from/env/ca.pem"}) - assert cfg.kafka.ssl.ca_location == "/from/env/ca.pem" - assert cfg.kafka.ssl.key_password == "" # ${VAR:-} resolved to empty + ssl = cfg.kafka.clusters["default"].ssl + assert ssl.ca_location == "/from/env/ca.pem" + assert ssl.key_password == "" # ${VAR:-} resolved to empty def test_kafka_ssl_invalid_security_protocol_raises(tmp_path): @@ -150,11 +175,13 @@ def test_kafka_ssl_invalid_security_protocol_raises(tmp_path): not as an opaque connect-time error.""" cfg_file = tmp_path / "agctl.yaml" cfg_file.write_text( - 'version: "2"\n' + 'version: "3"\n' "kafka:\n" - " brokers: [host:9092]\n" - " ssl:\n" - " security_protocol: SSLT\n" # typo + " clusters:\n" + " default:\n" + " brokers: [host:9092]\n" + " ssl:\n" + " security_protocol: SSLT\n" # typo ) with pytest.raises(ConfigError): load_config(str(cfg_file), env={}) @@ -164,12 +191,14 @@ def test_kafka_ssl_security_protocol_normalized(tmp_path): """security.protocol is normalized to librdkafka's uppercase form.""" cfg_file = tmp_path / "agctl.yaml" cfg_file.write_text( - 'version: "2"\n' + 'version: "3"\n' "kafka:\n" - " brokers: [host:9092]\n" - " ssl:\n" - " ca_location: /ca.pem\n" - " security_protocol: ssl\n" + " clusters:\n" + " default:\n" + " brokers: [host:9092]\n" + " ssl:\n" + " ca_location: /ca.pem\n" + " security_protocol: ssl\n" ) cfg = load_config(str(cfg_file), env={}) - assert cfg.kafka.ssl.security_protocol == "SSL" + assert cfg.kafka.clusters["default"].ssl.security_protocol == "SSL" diff --git a/tests/unit/test_migrate.py b/tests/unit/test_migrate.py index 89c2042..428e42b 100644 --- a/tests/unit/test_migrate.py +++ b/tests/unit/test_migrate.py @@ -1,17 +1,21 @@ -"""Tests for `agctl config migrate` (Task 6). - -Scope: the pure helper :func:`agctl.config.migrate.migrate_match_exprs` rewrites -a v1 config to dialect ``"2"`` (prepend ``.body | `` to HTTP ``match.jq``, -``.value | `` to Kafka ``match``, bump ``version``). The ``config migrate`` -Click command backs up the file and writes the rewrite (or previews with +"""Tests for `agctl config migrate` (v1/v2 -> v3). + +Scope: the pure helper :func:`agctl.config.migrate.migrate_config` rewrites a +v1/v2 config to dialect ``"3"`` — v1 inputs get the jq-prefix rewrite +(``.body | `` on HTTP ``match.jq``, ``.value | `` on Kafka ``match``) AND the +structural lift of a flat ``kafka:`` block into ``kafka.clusters.default``; v2 +inputs get only the structural lift (their match exprs are already +envelope-rooted). ``version`` bumps to ``"3"``. The ``config migrate`` Click +command backs up the file and writes the rewrite (or previews with ``--dry-run``). -Layering: ``migrate_match_exprs`` is a pure dict→dict transform in +Layering: ``migrate_config`` is a pure dict->dict transform in ``agctl/config/migrate.py``; the Click command in ``config_commands.py`` does the file I/O and envelope emit (mirroring ``config_validate`` / -``config_init`` — it does its OWN load+emit, no ``@envelope``). +``config_init`` -- it does its OWN load+emit, no ``@envelope``). """ +import copy import json import yaml @@ -19,24 +23,24 @@ from agctl.cli import cli from agctl.config import load_config -from agctl.config.migrate import migrate_match_exprs +from agctl.config.migrate import TO_VERSION, MigrateResult, migrate_config -# --- Step 1: helper tests ---------------------------------------------------- +# --- helper (jq-prefix) tests ------------------------------------------------ def test_migrate_http_stub_match_jq(): - """A v1 config with ``mocks.http.stubs..match.jq`` → prefix - ``.body | ``, bump version to "2", record one rewrite.""" + """A v1 config with ``mocks.http.stubs..match.jq`` -> prefix + ``.body | ``, bump version to "3", record one rewrite.""" config = { "version": "1", "mocks": {"http": {"stubs": {"s": {"match": {"jq": ".amount > 1000"}}}}}, } - result = migrate_match_exprs(config) - assert result.config["version"] == "2" - assert result.already_v2 is False + result = migrate_config(config) + assert result.config["version"] == "3" + assert result.already_current is False assert result.from_version == "1" - assert result.to_version == "2" + assert result.to_version == "3" assert ( result.config["mocks"]["http"]["stubs"]["s"]["match"]["jq"] == ".body | .amount > 1000" @@ -51,12 +55,12 @@ def test_migrate_http_stub_match_jq(): def test_migrate_reactor_match(): - """A v1 Kafka reactor ``match`` (string) → prefix ``.value | ``.""" + """A v1 Kafka reactor ``match`` (string) -> prefix ``.value | ``.""" config = { "version": "1", "mocks": {"kafka": {"reactors": {"r": {"match": ".command == \"X\""}}}}, } - result = migrate_match_exprs(config) + result = migrate_config(config) assert ( result.config["mocks"]["kafka"]["reactors"]["r"]["match"] == ".value | .command == \"X\"" @@ -70,13 +74,15 @@ def test_migrate_reactor_match(): ] -def test_migrate_kafka_pattern_match(): - """A v1 ``kafka.patterns..match`` → prefix ``.value | ``.""" +def test_migrate_kafka_pattern_match_no_flat_block(): + """A v1 ``kafka.patterns..match`` whose ``kafka:`` block carries ONLY + patterns (no flat broker keys) -> prefix ``.value | ``; no structural lift + (nothing to lift).""" config = { "version": "1", "kafka": {"patterns": {"p": {"match": ".eventType == \"Y\""}}}, } - result = migrate_match_exprs(config) + result = migrate_config(config) assert ( result.config["kafka"]["patterns"]["p"]["match"] == ".value | .eventType == \"Y\"" @@ -90,32 +96,51 @@ def test_migrate_kafka_pattern_match(): ] -def test_migrate_idempotent_and_already_v2(): - """Re-running on an already-migrated config is a no-op; a v2-native config - with a v2-style expr like ``.body.amount`` is NOT touched.""" - # (a) Idempotent: feed output of test 1 back in. - once = migrate_match_exprs( - { - "version": "1", - "mocks": {"http": {"stubs": {"s": {"match": {"jq": ".amount > 1000"}}}}}, - } - ) - twice = migrate_match_exprs(once.config) - assert twice.already_v2 is True - assert twice.rewrites == [] - assert ( - twice.config["mocks"]["http"]["stubs"]["s"]["match"]["jq"] - == ".body | .amount > 1000" - ) +def test_to_version_is_three(): + """TO_VERSION constant and the default to_version field are both "3".""" + assert TO_VERSION == "3" + # A migrated result's to_version always equals TO_VERSION. + result = migrate_config({"version": "1"}) + assert result.to_version == "3" + + +def test_migrate_idempotent_v3(): + """Re-running on an already-v3 config is a no-op: already_current True, + rewrites empty, deep-copied unchanged.""" + already = { + "version": "3", + "kafka": { + "clusters": {"main": {"brokers": ["h:9092"]}}, + "default_cluster": "main", + }, + } + result = migrate_config(already) + assert isinstance(result, MigrateResult) + assert result.already_current is True + assert result.rewrites == [] + assert result.config == already + assert result.config is not already # deep copy, not the same object + - # (b) A v2-native config with a v2-native expr (no `.body | ` prefix) is - # NOT rewritten — already_v2 short-circuits before traversal. - native = migrate_match_exprs( - {"version": "2", "mocks": {"http": {"stubs": {"s": {"match": {"jq": ".body.amount"}}}}}} +def test_migrate_v2_does_not_jq_rewrite_envelope_exprs(): + """A v2 config's envelope-rooted expr (``.body.amount``, no prefix) is NOT + touched: jq-prefix walkers run only on v1 sources. The version still bumps + to "3" (v2 is no longer current), but the expr is unchanged.""" + config = { + "version": "2", + "mocks": {"http": {"stubs": {"s": {"match": {"jq": ".body.amount"}}}}}, + } + result = migrate_config(config) + assert result.already_current is False + assert result.from_version == "2" + # No jq rewrite recorded. + assert result.rewrites == [] + assert result.config["version"] == "3" + # The v2-native expr is unchanged -- not double-prefixed. + assert ( + result.config["mocks"]["http"]["stubs"]["s"]["match"]["jq"] + == ".body.amount" ) - assert native.already_v2 is True - assert native.rewrites == [] - assert native.config["mocks"]["http"]["stubs"]["s"]["match"]["jq"] == ".body.amount" def test_migrate_leaves_capture_and_match_body_untouched(): @@ -134,7 +159,7 @@ def test_migrate_leaves_capture_and_match_body_untouched(): } }, } - result = migrate_match_exprs(config) + result = migrate_config(config) stub = result.config["mocks"]["http"]["stubs"]["s"] assert stub["match"]["jq"] == ".body | .x" # match.body dict is left as-is. @@ -148,34 +173,182 @@ def test_migrate_leaves_capture_and_match_body_untouched(): def test_migrate_missing_sections_no_crash(): """A v1 config with no ``mocks``/``kafka`` sections: no rewrites, version - still bumped to "2", no crash.""" - result = migrate_match_exprs({"version": "1"}) + still bumped to "3", no crash.""" + result = migrate_config({"version": "1"}) assert result.rewrites == [] - assert result.config["version"] == "2" - assert result.already_v2 is False + assert result.config["version"] == "3" + assert result.already_current is False -def test_migrate_strips_version_whitespace_already_v2(): - """``migrate_match_exprs`` must compute ``source_major`` with ``.strip()`` +def test_migrate_strips_version_whitespace(): + """``migrate_config`` must compute ``source_major`` with ``.strip()`` exactly as the loader's ``_check_version`` does. A loader-accepted - ``version: " 2 "`` is therefore already-v2 here too — a v2-native expr like - ``.body.amount`` is NOT force-rewritten (which would double-prefix it into - the broken ``.body | .body.amount``).""" + ``version: " 3 "`` is therefore already-current here too -- a v3-native expr + is NOT force-rewritten.""" config = { - "version": " 2 ", - "mocks": {"http": {"stubs": {"s": {"match": {"jq": ".body.amount > 1000"}}}}}, + "version": " 3 ", + "kafka": {"clusters": {"main": {"brokers": ["h:9092"]}}}, } - result = migrate_match_exprs(config) - assert result.already_v2 is True + result = migrate_config(config) + assert result.already_current is True assert result.rewrites == [] - # The v2-native expr is unchanged — not double-prefixed. + + +# --- structural lift (v2 -> v3) --------------------------------------------- + + +def test_migrate_flat_v2_kafka_lifted_to_clusters_default(): + """A flat v2 ``kafka:`` block is lifted into ``kafka.clusters.default``; + ``default_cluster`` is set; ``patterns`` preserved; version bumped to "3". + + v2 source -> no jq rewrite (exprs already envelope-rooted), only the + structural lift fires. + """ + config = { + "version": "2", + "kafka": { + "brokers": ["h:9092"], + "timeout_seconds": 30, + "default_consumer_group": "g", + "patterns": {"p": {"topic": "t", "match": ".value.x"}}, + }, + } + result = migrate_config(config) + assert result.already_current is False + assert result.from_version == "2" + assert result.config["version"] == "3" + assert result.config["kafka"]["clusters"]["default"]["brokers"] == ["h:9092"] + assert result.config["kafka"]["clusters"]["default"]["timeout_seconds"] == 30 assert ( - result.config["mocks"]["http"]["stubs"]["s"]["match"]["jq"] - == ".body.amount > 1000" + result.config["kafka"]["clusters"]["default"]["default_consumer_group"] + == "g" ) + assert result.config["kafka"]["default_cluster"] == "default" + # patterns preserved unchanged (the v2 match is already envelope-rooted). + assert result.config["kafka"]["patterns"] == { + "p": {"topic": "t", "match": ".value.x"} + } + # Structural rewrites, deterministic order: + # brokers, timeout_seconds, default_consumer_group, default_cluster. + structural = [r for r in result.rewrites if r["path"].startswith("kafka.")] + assert structural == [ + { + "path": "kafka.clusters.default.brokers", + "before": ["h:9092"], + "after": ["h:9092"], + }, + { + "path": "kafka.clusters.default.timeout_seconds", + "before": 30, + "after": 30, + }, + { + "path": "kafka.clusters.default.default_consumer_group", + "before": "g", + "after": "g", + }, + {"path": "kafka.default_cluster", "before": None, "after": "default"}, + ] -# --- Step 5: command tests --------------------------------------------------- + +def test_migrate_flat_v2_full_lift_order(): + """All five flat keys lift in deterministic order + (brokers, ssl, timeout_seconds, default_consumer_group, schema_registry_url), + then default_cluster is set.""" + config = { + "version": "2", + "kafka": { + "brokers": ["h:9092"], + "ssl": {"ca_location": "/ca.pem"}, + "timeout_seconds": 15, + "default_consumer_group": "grp", + "schema_registry_url": "http://sr:8081", + }, + } + result = migrate_config(config) + cluster = result.config["kafka"]["clusters"]["default"] + assert cluster["brokers"] == ["h:9092"] + assert cluster["ssl"] == {"ca_location": "/ca.pem"} + assert cluster["timeout_seconds"] == 15 + assert cluster["default_consumer_group"] == "grp" + assert cluster["schema_registry_url"] == "http://sr:8081" + + paths = [r["path"] for r in result.rewrites] + assert paths == [ + "kafka.clusters.default.brokers", + "kafka.clusters.default.ssl", + "kafka.clusters.default.timeout_seconds", + "kafka.clusters.default.default_consumer_group", + "kafka.clusters.default.schema_registry_url", + "kafka.default_cluster", + ] + + +def test_migrate_v1_one_pass_jq_and_structural(): + """A v1 config (flat kafka + a ``.value``-less kafka pattern match) migrates + to v3 in ONE pass: the jq prefix is applied AND the structural lift occurs. + """ + config = { + "version": "1", + "kafka": { + "brokers": ["h:9092"], + "patterns": { + "p": {"topic": "t", "match": '.eventType == "Y"'} + }, + }, + } + result = migrate_config(config) + assert result.config["version"] == "3" + assert result.from_version == "1" + # jq prefix applied. + assert ( + result.config["kafka"]["patterns"]["p"]["match"] + == '.value | .eventType == "Y"' + ) + # structural lift applied. + assert result.config["kafka"]["clusters"]["default"]["brokers"] == ["h:9092"] + assert result.config["kafka"]["default_cluster"] == "default" + + paths = [r["path"] for r in result.rewrites] + # jq rewrite first, then structural rewrites. + assert paths[0] == "kafka.patterns.p.match" + assert "kafka.clusters.default.brokers" in paths + assert "kafka.default_cluster" in paths + + +def test_migrate_already_clustered_kafka_not_lifted(): + """A v2 config whose ``kafka:`` already has a ``clusters`` key is NOT + re-lifted (defensive: never double-lift). Only version bumps.""" + config = { + "version": "2", + "kafka": { + "clusters": {"main": {"brokers": ["h:9092"]}}, + "default_cluster": "main", + }, + } + result = migrate_config(config) + assert result.config["version"] == "3" + # No structural rewrites (already clustered). + assert [r for r in result.rewrites if r["path"].startswith("kafka.")] == [] + assert result.config["kafka"]["clusters"]["main"]["brokers"] == ["h:9092"] + + +def test_migrate_does_not_mutate_input(): + """``migrate_config`` never mutates its input dict (deep-copy first).""" + config = { + "version": "2", + "kafka": {"brokers": ["h:9092"], "patterns": {"p": {"topic": "t"}}}, + } + snapshot = copy.deepcopy(config) + result = migrate_config(config) + assert config == snapshot # input untouched + # The output is a separate object. + assert result.config is not config + assert result.config["kafka"] is not config["kafka"] + + +# --- command tests ----------------------------------------------------------- def _migrate(tmp_path, args): @@ -194,7 +367,7 @@ def test_config_migrate_dry_run_writes_nothing(tmp_path): " stubs:\n" " s:\n" " match:\n" - " jq: \".amount > 1000\"\n" + ' jq: ".amount > 1000"\n' ) cfg.write_text(original) @@ -203,7 +376,7 @@ def test_config_migrate_dry_run_writes_nothing(tmp_path): assert result.exit_code == 0 payload = json.loads(result.output) assert payload["ok"] is True - assert payload["result"]["already_v2"] is False + assert payload["result"]["already_current"] is False assert payload["result"]["rewritten"][0]["after"] == ".body | .amount > 1000" # File unchanged on disk; no backup written. @@ -213,13 +386,13 @@ def test_config_migrate_dry_run_writes_nothing(tmp_path): def test_config_migrate_writes_file_and_backup(tmp_path): """Without ``--dry-run``: a ``.bak`` of the original is created, the file - is rewritten with `version: "2"` and the prefixed exprs, and the rewritten - file round-trips through ``load_config`` (proving a valid v2 config). + is rewritten with `version: "3"` and the prefixed exprs, and the rewritten + file round-trips through ``load_config`` (proving a valid v3 config). The v1 fixture is a *complete* config exercising all three match-site families (HTTP stub ``match.jq``, Kafka reactor ``match``, a ``kafka.patterns`` entry) plus a ``capture`` (which migration must leave - untouched) — so the migrated file passes Pydantic validation on load.""" + untouched) -- so the migrated file passes Pydantic validation on load.""" cfg = tmp_path / "agctl.yaml" original = ( 'version: "1"\n' @@ -263,9 +436,9 @@ def test_config_migrate_writes_file_and_backup(tmp_path): assert bak.exists() assert bak.read_text() == original - # Rewritten file: version 2, all three match exprs prefixed. + # Rewritten file: version 3, all three match exprs prefixed. rewritten = yaml.safe_load(cfg.read_text()) - assert rewritten["version"] == "2" + assert rewritten["version"] == "3" assert ( rewritten["mocks"]["http"]["stubs"]["s"]["match"]["jq"] == ".body | .amount > 1000" @@ -278,31 +451,30 @@ def test_config_migrate_writes_file_and_backup(tmp_path): rewritten["kafka"]["patterns"]["p"]["match"] == '.value | .eventType == "Y"' ) - # capture.from is NOT a match site — migration leaves it untouched. + # capture.from is NOT a match site -- migration leaves it untouched. assert ( rewritten["mocks"]["http"]["stubs"]["s"]["capture"]["cid"]["from"] == ".body.correlationId" ) - # Round-trip: the rewritten file loads cleanly via the v2 gate. + # Round-trip: the rewritten file loads cleanly via the v3 gate. loaded = load_config(str(cfg), env={}) assert loaded.mocks.http.stubs["s"].match.jq == ".body | .amount > 1000" assert loaded.mocks.kafka.reactors["r"].match == '.value | .command == "X"' assert loaded.kafka.patterns["p"].match == '.value | .eventType == "Y"' -def test_config_migrate_already_v2(tmp_path): - """A v2 config: ``already_v2=True``, no rewrites, no ``.bak``, file +def test_config_migrate_already_v3(tmp_path): + """A v3 config: ``already_current=True``, no rewrites, no ``.bak``, file unchanged.""" cfg = tmp_path / "agctl.yaml" original = ( - 'version: "2"\n' - "mocks:\n" - " http:\n" - " stubs:\n" - " s:\n" - " match:\n" - " jq: \".body | .amount > 1000\"\n" + 'version: "3"\n' + "kafka:\n" + " clusters:\n" + " main:\n" + " brokers:\n" + " - host:9092\n" ) cfg.write_text(original) @@ -310,7 +482,7 @@ def test_config_migrate_already_v2(tmp_path): assert result.exit_code == 0 payload = json.loads(result.output) - assert payload["result"]["already_v2"] is True + assert payload["result"]["already_current"] is True assert payload["result"]["rewritten"] == [] assert not (tmp_path / "agctl.yaml.bak").exists() assert cfg.read_text() == original @@ -327,7 +499,7 @@ def test_config_migrate_result_carries_cli_flags_note(tmp_path): " stubs:\n" " s:\n" " match:\n" - " jq: \".amount > 1000\"\n" + ' jq: ".amount > 1000"\n' ) result = _migrate(tmp_path, ["--config", str(cfg), "--dry-run"]) @@ -408,9 +580,9 @@ def test_config_migrate_result_carries_formatting_note(tmp_path): assert ".bak" in note -def test_config_migrate_formatting_note_null_on_dry_run_and_already_v2(tmp_path): +def test_config_migrate_formatting_note_null_on_dry_run_and_already_v3(tmp_path): """``formatting_note`` is ``null`` when nothing is reformatted (--dry-run - or already_v2) — the caveat would be noise in those cases.""" + or already_current) -- the caveat would be noise in those cases.""" # --dry-run: no write. cfg = tmp_path / "agctl.yaml" cfg.write_text( @@ -425,23 +597,21 @@ def test_config_migrate_formatting_note_null_on_dry_run_and_already_v2(tmp_path) dry = _migrate(tmp_path, ["--config", str(cfg), "--dry-run"]) assert json.loads(dry.output)["result"]["formatting_note"] is None - # already_v2: no write. + # already_current (v3): no write. cfg2 = tmp_path / "agctl2.yaml" cfg2.write_text( - 'version: "2"\n' - "mocks:\n" - " http:\n" - " stubs:\n" - " s:\n" - " match:\n" - ' jq: ".body | .amount > 1000"\n' + 'version: "3"\n' + "kafka:\n" + " clusters:\n" + " main:\n" + " brokers: [host:9092]\n" ) already = _migrate(tmp_path, ["--config", str(cfg2)]) assert json.loads(already.output)["result"]["formatting_note"] is None def test_migrate_preserves_env_interpolation(tmp_path): - """``${VAR:-default}`` tokens are resolved at LOAD time, not migrate time — + """``${VAR:-default}`` tokens are resolved at LOAD time, not migrate time -- the rewritten file on disk must still contain them verbatim. Do NOT call ``load_config`` here (env unresolved).""" cfg = tmp_path / "agctl.yaml" diff --git a/tests/unit/test_mock_commands.py b/tests/unit/test_mock_commands.py index 0ec0d81..7e41820 100644 --- a/tests/unit/test_mock_commands.py +++ b/tests/unit/test_mock_commands.py @@ -34,7 +34,7 @@ class TestMockRunCommand: def test_only_http_with_stubs(self, temp_config, fake_engine): """--only http with 2-stub mocks.http -> run_http=True, run_kafka=False, kafka_client=None.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -82,10 +82,12 @@ def test_only_kafka_with_reactors(self, temp_config, fake_engine): pytest.importorskip("confluent_kafka") config_content = """ -version: "2.0" +version: "3" kafka: - brokers: - - "localhost:9092" + clusters: + default: + brokers: + - "localhost:9092" mocks: kafka: reactors: @@ -120,7 +122,7 @@ def test_only_kafka_with_reactors(self, temp_config, fake_engine): def test_only_http_without_mocks_http(self, temp_config): """--only http with no mocks.http -> exit 2, ConfigError envelope.""" config_content = """ -version: "2.0" +version: "3" mocks: kafka: reactors: @@ -151,7 +153,7 @@ def test_only_http_without_mocks_http(self, temp_config): def test_no_mocks_section_no_only(self, temp_config, fake_engine): """No mocks section, no --only -> exit 0, no-op engine (run_http=False, run_kafka=False).""" config_content = """ -version: "2.0" +version: "3" kafka: brokers: - "localhost:9092" @@ -179,11 +181,15 @@ def test_no_mocks_section_no_only(self, temp_config, fake_engine): fake_engine.shutdown.assert_called_once() def test_kafka_reactors_without_brokers(self, temp_config): - """mocks.kafka reactors present but no kafka.brokers -> exit 2, ConfigError envelope.""" + """mocks.kafka reactors present but resolved cluster has empty brokers -> + exit 2, ConfigError envelope.""" config_content = """ -version: "2.0" +version: "3" kafka: - brokers: [] + clusters: + default: + brokers: [] + default_cluster: default mocks: kafka: reactors: @@ -209,12 +215,12 @@ def test_kafka_reactors_without_brokers(self, temp_config): assert envelope["ok"] is False assert envelope["command"] == "mock.run" assert envelope["error"]["type"] == "ConfigError" - assert "kafka.brokers" in envelope["error"]["message"] + assert "brokers" in envelope["error"]["message"] def test_duration_and_until_stopped_mutually_exclusive(self, temp_config): """--duration and --until-stopped together -> exit 2, ConfigError envelope.""" config_content = """ -version: "2.0" +version: "3" """ temp_config.write_text(config_content) @@ -237,7 +243,7 @@ def test_duration_and_until_stopped_mutually_exclusive(self, temp_config): def test_engine_start_raises_connection_failure(self, temp_config): """Engine start() raises ConnectionFailure -> mock.run envelope with ConnectionError, exit 2.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -278,7 +284,7 @@ def failing_start(): def test_http_listen_override(self, temp_config, fake_engine): """--http-listen 127.0.0.1:9999 overrides config listen -> engine called with http_listen.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -309,7 +315,7 @@ def test_http_listen_override(self, temp_config, fake_engine): def test_http_listen_bad_value(self, temp_config): """--http-listen 'bad:no-port' -> exit 2 (parse_listen fails).""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -341,7 +347,7 @@ def test_http_listen_bad_value(self, temp_config): def test_started_line_emitted(self, temp_config, fake_engine): """Config with mocks.http, --only http -> stdout's first line is the engine's started.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -385,7 +391,7 @@ def fake_run(): def test_fail_fast_flag_passed(self, temp_config, fake_engine): """--fail-fast flag is passed to the engine.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -416,7 +422,7 @@ def test_fail_fast_flag_passed(self, temp_config, fake_engine): def test_duration_flag_passed(self, temp_config, fake_engine): """--duration flag is passed to the engine.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -447,7 +453,7 @@ def test_duration_flag_passed(self, temp_config, fake_engine): def test_exit_code_from_engine(self, temp_config): """Engine returns non-zero exit code -> process exits with that code.""" config_content = """ -version: "2.0" +version: "3" mocks: http: listen: "0.0.0.0:18080" @@ -478,7 +484,7 @@ def test_exit_code_from_engine(self, temp_config): def test_mock_start_includes_overlay_in_daemon_argv(tmp_path, monkeypatch): """mock start includes --overlay in the daemon argv so the spawned daemon loads the overlay.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 diff --git a/tests/unit/test_mock_lifecycle.py b/tests/unit/test_mock_lifecycle.py index c56acd2..72044ad 100644 --- a/tests/unit/test_mock_lifecycle.py +++ b/tests/unit/test_mock_lifecycle.py @@ -11,8 +11,8 @@ from agctl.cli import cli from agctl.mock.daemon import is_alive -# Test fixture config (minimal, version 2.0) -MINIMAL_CONFIG = """version: "2.0" +# Test fixture config (minimal, version 3) +MINIMAL_CONFIG = """version: "3" mocks: http: listen: "127.0.0.1:18080" @@ -26,7 +26,12 @@ """ # Config without http section -NO_HTTP_CONFIG = """version: "2.0" +NO_HTTP_CONFIG = """version: "3" +kafka: + clusters: + default: + brokers: + - "localhost:9092" mocks: kafka: reactors: diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index 9e38e91..c6df330 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -5,6 +5,11 @@ Config, DatabaseConnection, DatabaseTemplate, + KafkaCluster, + KafkaConfig, + KafkaPattern, + KafkaReactor, + KafkaReaction, LogSource, LogsConfig, LogsDefaults, @@ -13,17 +18,19 @@ def _full_config_dict(): return { - "version": "1", + "version": "3", "services": { "order-service": {"base_url": "http://localhost:8081", "health_path": "/health"} }, "kafka": { - "brokers": ["localhost:9092"], - "default_consumer_group": "agctl-consumer", + "clusters": { + "main": {"brokers": ["localhost:9092"]}, + }, + "default_cluster": "main", "patterns": { "order-created": { "topic": "orders.created", - "match": '.eventType == "ORDER_CREATED"', + "match": '.value.eventType == "ORDER_CREATED"', } }, }, @@ -52,11 +59,13 @@ def test_full_config_validates_with_connections_and_templates(): def test_empty_sections_default(): - cfg = Config.model_validate({"version": "1"}) + cfg = Config.model_validate({"version": "3"}) assert cfg.services == {} assert cfg.database.connections == {} assert cfg.database.templates == {} - assert cfg.kafka.brokers == [] + assert cfg.kafka.clusters == {} + assert cfg.kafka.default_cluster is None + assert cfg.kafka.patterns == {} def test_http_template_requires_method_service_path(): @@ -173,3 +182,69 @@ def test_config_has_logs_field(): ) assert cfg2.logs.sources["svc"].path == "/tmp/x.log" assert cfg2.logs.sources["svc"].type == "file" # default applied + + +# --- v3 kafka clusters schema ------------------------------------------------- + + +def test_kafka_config_clusters_shape_parses(): + """A KafkaConfig built from a clusters dict parses; the cluster's brokers, + default_cluster, and patterns are all reachable.""" + cfg = Config.model_validate( + { + "version": "3", + "kafka": { + "clusters": {"main": {"brokers": ["h:9092"]}}, + "default_cluster": "main", + "patterns": {"p": {"topic": "t"}}, + }, + } + ) + assert cfg.kafka.clusters["main"].brokers == ["h:9092"] + assert cfg.kafka.default_cluster == "main" + assert cfg.kafka.patterns["p"].topic == "t" + + +def test_kafka_cluster_carries_per_cluster_fields(): + """KafkaCluster carries brokers/ssl/timeout_seconds/default_consumer_group/ + schema_registry_url (the fields formerly on KafkaConfig).""" + cluster = KafkaCluster( + brokers=["h:9092"], + timeout_seconds=30, + default_consumer_group="g", + schema_registry_url="http://sr:8081", + ) + assert cluster.brokers == ["h:9092"] + assert cluster.timeout_seconds == 30 + assert cluster.default_consumer_group == "g" + assert cluster.schema_registry_url == "http://sr:8081" + assert cluster.ssl is None + + +def test_kafka_config_default_is_empty(): + """KafkaConfig() default: empty clusters, default_cluster None, empty patterns.""" + k = KafkaConfig() + assert k.clusters == {} + assert k.default_cluster is None + assert k.patterns == {} + + +def test_kafka_pattern_accepts_cluster_field(): + """KafkaPattern gains an optional cluster field (consumed in Task 2).""" + pat = KafkaPattern(topic="t", cluster="analytics") + assert pat.cluster == "analytics" + # defaults to None when omitted + assert KafkaPattern(topic="t").cluster is None + + +def test_kafka_reactor_accepts_cluster_field(): + """KafkaReactor gains an optional cluster field (consumed in Task 3).""" + reactor = KafkaReactor( + topic="t", + cluster="analytics", + reaction=KafkaReaction(topic="out", value={}), + ) + assert reactor.cluster == "analytics" + # defaults to None when omitted + plain = KafkaReactor(topic="t", reaction=KafkaReaction(topic="out", value={})) + assert plain.cluster is None diff --git a/tests/unit/test_overlay.py b/tests/unit/test_overlay.py index 7afe7e5..07406bf 100644 --- a/tests/unit/test_overlay.py +++ b/tests/unit/test_overlay.py @@ -16,9 +16,9 @@ def test_partial_config_accepts_empty_dict(): def test_partial_config_accepts_version(): - """PartialConfig.model_validate({"version": "2"}) returns version="2".""" - result = PartialConfig.model_validate({"version": "2"}) - assert result.version == "2" + """PartialConfig.model_validate({"version": "3"}) returns version="3".""" + result = PartialConfig.model_validate({"version": "3"}) + assert result.version == "3" def test_partial_config_validates_sections_without_version(): @@ -130,7 +130,7 @@ def test_deep_merge_dotted_path_nesting(): def test_compose_config_overlay_adds_template(tmp_path): """Overlay adds a template → config has both; overrides empty.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -157,7 +157,7 @@ def test_compose_config_overlay_adds_template(tmp_path): def test_compose_config_override_recorded(tmp_path): """Base and overlay both define templates.create-order → overlay wins; override recorded.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -183,9 +183,9 @@ def test_compose_config_override_recorded(tmp_path): def test_compose_config_version_inherited(tmp_path): - """Overlay has no version → no error; config.version == '2'.""" + """Overlay has no version → no error; config.version == '3'.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -203,13 +203,14 @@ def test_compose_config_version_inherited(tmp_path): path: /api/v1/orders/{id} """) result = compose_config(str(base), [str(ov)]) - assert result.config.version == "2" + assert result.config.version == "3" def test_compose_config_overlay_version_mismatch(tmp_path): - """Overlay has version: '3' → ConfigError with detail['overlay'] set.""" + """Overlay has a stale version ('2' under the v3 tool) → ConfigError with + detail['overlay'] set.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -220,7 +221,7 @@ def test_compose_config_overlay_version_mismatch(tmp_path): path: /api/v1/orders """) ov = tmp_path / "overlay.yaml" - ov.write_text("""version: "3" + ov.write_text("""version: "2" templates: extra: method: GET @@ -231,13 +232,13 @@ def test_compose_config_overlay_version_mismatch(tmp_path): compose_config(str(base), [str(ov)]) assert "version mismatch" in exc_info.value.message.lower() assert exc_info.value.detail["overlay"] == str(ov) - assert exc_info.value.detail["found"] == "3" + assert exc_info.value.detail["found"] == "2" def test_compose_config_bad_overlay_fragment(tmp_path): """Overlay has templates.bad missing required method → ConfigError with detail['overlay'] set.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -263,7 +264,7 @@ def test_compose_config_bad_overlay_fragment(tmp_path): def test_compose_config_type_clash_final_validate(tmp_path): """Base templates.x is valid; overlay replaces with scalar → caught at PartialConfig validation.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -287,7 +288,7 @@ def test_compose_config_type_clash_final_validate(tmp_path): def test_compose_config_env_wins_over_overlay(tmp_path): """Base sets path=/a, overlay sets /b, env sets /env → final value is /env.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -311,7 +312,7 @@ def test_compose_config_env_wins_over_overlay(tmp_path): def test_compose_config_missing_overlay_file(tmp_path): """Overlay file not found → ConfigError with detail['path'] set.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -330,7 +331,7 @@ def test_compose_config_missing_overlay_file(tmp_path): def test_load_config_forwards_overlays(tmp_path): """load_config(path, overlays=[...]) returns Config with overlay's added template.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -356,7 +357,7 @@ def test_load_config_forwards_overlays(tmp_path): def test_load_config_no_overlay_back_compat(tmp_path): """load_config(path) and load_config(path, env={}) work as before (back-compat).""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -383,7 +384,7 @@ def test_load_config_no_overlay_back_compat(tmp_path): def test_global_overlay_flag_threads_to_show(tmp_path): """Scenario 1: Global --overlay flag threads to config show.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -418,7 +419,7 @@ def test_global_overlay_flag_threads_to_show(tmp_path): def test_override_surfaced_in_show(tmp_path): """Scenario 2: Override surfaced in show result.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -453,7 +454,7 @@ def test_override_surfaced_in_show(tmp_path): def test_no_overlay_shape_unchanged(tmp_path): """Scenario 3: No --overlay shape unchanged (back-compat).""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -476,7 +477,7 @@ def test_no_overlay_shape_unchanged(tmp_path): def test_post_command_overlay_form(tmp_path): """Scenario 4: Post-command --overlay form (own option precedence).""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -507,7 +508,7 @@ def test_post_command_overlay_form(tmp_path): def test_compose_config_overlay_version_only_no_override_recorded(tmp_path): """Overlay with only version: '2' on v2 base → no override recorded, version inherited.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -518,20 +519,20 @@ def test_compose_config_overlay_version_only_no_override_recorded(tmp_path): path: /api/v1/orders """) ov = tmp_path / "overlay.yaml" - ov.write_text("""version: "2" + ov.write_text("""version: "3" """) result = compose_config(str(base), [str(ov)]) # No overrides should be recorded for version assert result.overrides == [] # Version should be inherited from base - assert result.config.version == "2" + assert result.config.version == "3" # Fix 2: multiple overlays later wins test def test_compose_config_multiple_overlays_later_wins(tmp_path): """Two overlays both setting templates.x.path → later wins, single override record per path.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -576,7 +577,7 @@ def test_compose_config_multiple_overlays_later_wins(tmp_path): def test_http_call_forwards_overlay(tmp_path, monkeypatch): """http call forwards overlay_paths from ctx.obj to load_config_or_raise.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -636,7 +637,7 @@ def fake_load_config(config_path, overlay_paths=None): def test_db_query_forwards_overlay(tmp_path, monkeypatch): """db query forwards overlay_paths from ctx.obj to load_config_or_raise.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -680,7 +681,7 @@ def fake_load_config(config_path, overlay_paths=None): def test_kafka_produce_forwards_overlay(tmp_path, monkeypatch): """kafka produce forwards overlay_paths from ctx.obj to load_config_or_raise.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -700,11 +701,13 @@ def test_kafka_produce_forwards_overlay(tmp_path, monkeypatch): def fake_load_config(config_path, overlay_paths=None): captured_overlays.append(overlay_paths) - from agctl.config.models import KafkaConfig + from agctl.config.models import KafkaCluster, KafkaConfig return Config( - version="2", + version="3", services={"orders": ServiceConfig(base_url="http://localhost:8081")}, - kafka=KafkaConfig(brokers=["localhost:9092"]) + kafka=KafkaConfig( + clusters={"default": KafkaCluster(brokers=["localhost:9092"])} + ), ) monkeypatch.setattr("agctl.commands.kafka_commands.load_config_or_raise", fake_load_config) @@ -721,7 +724,7 @@ def fake_load_config(config_path, overlay_paths=None): def test_check_ready_forwards_overlay(tmp_path, monkeypatch): """check ready forwards overlay_paths from ctx.obj to load_config_or_raise.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -756,7 +759,7 @@ def fake_load_config(config_path, overlay_paths=None): def test_mock_run_forwards_overlay(tmp_path, monkeypatch): """mock run forwards overlay_paths from ctx.obj to load_config_or_raise.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 @@ -801,7 +804,7 @@ def fake_load_config(config_path, overlay_paths=None): def test_http_ping_forwards_overlay(tmp_path, monkeypatch): """http ping forwards overlay_paths from ctx.obj to load_config_or_raise.""" base = tmp_path / "agctl.yaml" - base.write_text("""version: "2" + base.write_text("""version: "3" services: orders: base_url: http://localhost:8081 diff --git a/tests/unit/test_validator.py b/tests/unit/test_validator.py index 592fdbd..137a0bd 100644 --- a/tests/unit/test_validator.py +++ b/tests/unit/test_validator.py @@ -11,6 +11,7 @@ HttpResponse, HttpStub, HttpTemplate, + KafkaCluster, KafkaConfig, KafkaMockConfig, KafkaPattern, @@ -275,8 +276,9 @@ def test_read_mode_template_with_read_only_connection_no_error(): # --- mock server validation --------------------------------------------------- -def test_mock_kafka_requires_kafka_brokers_error(): - """mocks.kafka.reactors non-empty but no kafka.brokers → error.""" +def test_mock_kafka_requires_resolvable_default_cluster_error(): + """mocks.kafka.reactors non-empty but no resolvable cluster -> error at + mocks.kafka (no default/single cluster resolves).""" cfg = _cfg( mocks=MocksConfig( kafka=KafkaMockConfig( @@ -290,16 +292,16 @@ def test_mock_kafka_requires_kafka_brokers_error(): } ) ), - kafka=KafkaConfig(brokers=[]), # Empty brokers list + kafka=KafkaConfig(), # no clusters at all ) errors, warnings = validate_config(cfg) assert len(errors) == 1 assert errors[0]["path"] == "mocks.kafka" - assert errors[0]["message"] == "kafka mocks require top-level kafka.brokers" + assert errors[0]["message"] == "reactors require a resolvable default cluster" -def test_mock_kafka_with_brokers_no_error(): - """mocks.kafka.reactors with kafka.brokers present → no error.""" +def test_mock_kafka_with_cluster_no_error(): + """mocks.kafka.reactors with a single resolvable cluster -> no error.""" cfg = _cfg( mocks=MocksConfig( kafka=KafkaMockConfig( @@ -313,13 +315,68 @@ def test_mock_kafka_with_brokers_no_error(): } ) ), - kafka=KafkaConfig(brokers=["localhost:9092"]), + kafka=KafkaConfig( + clusters={"default": KafkaCluster(brokers=["localhost:9092"])} + ), ) errors, warnings = validate_config(cfg) - # Should not have the kafka.brokers error + # Should not have the mocks.kafka error assert not any(e["path"] == "mocks.kafka" for e in errors) +# --- v3 cluster cross-ref validation ---------------------------------------- + + +def test_reactor_default_cluster_missing_brokers_errors(): + """A reactor whose resolved default cluster exists but has empty brokers -> + error at mocks.kafka naming the cluster.""" + cfg = _cfg( + mocks=MocksConfig( + kafka=KafkaMockConfig( + reactors={ + "r": KafkaReactor( + topic="t", + reaction=KafkaReaction(topic="out", value={}), + ) + } + ) + ), + kafka=KafkaConfig( + clusters={"default": KafkaCluster(brokers=[])}, + default_cluster="default", + ), + ) + errors, warnings = validate_config(cfg) + mocks_errors = [e for e in errors if e["path"] == "mocks.kafka"] + assert len(mocks_errors) == 1 + assert "kafka.clusters.default.brokers" in mocks_errors[0]["message"] + + +def test_default_cluster_dangling_ref_errors(): + """cfg.kafka.default_cluster names a cluster absent from clusters -> error + at kafka.default_cluster.""" + cfg = _cfg( + kafka=KafkaConfig(clusters={}, default_cluster="ghost"), + ) + errors, warnings = validate_config(cfg) + paths = [e["path"] for e in errors] + assert "kafka.default_cluster" in paths + + +def test_pattern_cluster_dangling_ref_errors(): + """A KafkaPattern whose cluster names an unknown cluster -> error at + kafka.patterns..cluster.""" + cfg = _cfg( + kafka=KafkaConfig( + clusters={"main": KafkaCluster(brokers=["h:9092"])}, + patterns={"p": KafkaPattern(topic="t", cluster="ghost")}, + ), + ) + errors, warnings = validate_config(cfg) + paths = [e["path"] for e in errors] + assert "kafka.patterns.p.cluster" in paths + + def test_mock_http_stub_missing_description_warns(): """HTTP stub without description → warning.""" cfg = _cfg( @@ -357,7 +414,9 @@ def test_mock_kafka_reactor_missing_description_warns(): } ) ), - kafka=KafkaConfig(brokers=["localhost:9092"]), + kafka=KafkaConfig( + clusters={"default": KafkaCluster(brokers=["localhost:9092"])} + ), ) errors, warnings = validate_config(cfg) assert errors == [] From 4fdf00e75089c0048cf40e9bdcd4ba8f6d6f363f Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 00:41:36 +0300 Subject: [PATCH 03/12] feat(kafka): add --cluster flag and per-pattern cluster binding Co-Authored-By: Claude --- agctl/commands/kafka_commands.py | 54 ++++++---- tests/integration/conftest.py | 28 +++++ tests/integration/test_kafka_commands.py | 82 ++++++++++++++ tests/unit/test_kafka_commands.py | 131 +++++++++++++++++++++++ 4 files changed, 277 insertions(+), 18 deletions(-) diff --git a/agctl/commands/kafka_commands.py b/agctl/commands/kafka_commands.py index 1f8daa8..4962a15 100644 --- a/agctl/commands/kafka_commands.py +++ b/agctl/commands/kafka_commands.py @@ -158,16 +158,17 @@ def _kafka_produce_core( message: str, key: str | None, header: tuple[str, ...], + cluster: str | None = None, overlay_paths: list[str] | None = None, ) -> dict: cfg = load_config_or_raise(config_path, overlay_paths=overlay_paths) value = json.loads(message) headers = parse_params(header) if header else None - # Resolve the cluster (Task 1: no flag/binding yet -> default/single-cluster). - name = resolve_cluster_name(cfg.kafka, None) - cluster = cfg.kafka.clusters[name] - client = new_kafka_client(cluster) + # Resolve the cluster: --cluster (explicit) > default > single-cluster. + name = resolve_cluster_name(cfg.kafka, explicit=cluster) + resolved = cfg.kafka.clusters[name] + client = new_kafka_client(resolved) return client.produce(topic, value, key=key, headers=headers or None) @@ -176,6 +177,7 @@ def _kafka_produce_core( @click.option("--message", "message", required=True, help="JSON message body") @click.option("--key", "key", default=None, help="Message key") @click.option("--header", "header", multiple=True, help="k=v message header") +@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)") @click.pass_context def kafka_produce( ctx: click.Context, @@ -183,11 +185,12 @@ def kafka_produce( message: str, key: str | None, header: tuple[str, ...], + cluster: str | None, ) -> None: """Produce one message to a Kafka topic.""" config_path = ctx.obj.get("config_path") if ctx.obj else None ovs = ctx.obj.get("overlay_paths") if ctx.obj else None - _kafka_produce_envelope(config_path, topic, message, key, header, overlay_paths=list(ovs) if ovs else None) + _kafka_produce_envelope(config_path, topic, message, key, header, cluster, overlay_paths=list(ovs) if ovs else None) _kafka_produce_envelope = envelope("kafka.produce")(_kafka_produce_core) @@ -208,6 +211,7 @@ def _kafka_consume_core( expect_count: int | None, from_beginning: bool, consumer_group: str | None, + cluster: str | None = None, overlay_paths: list[str] | None = None, ) -> dict: cfg = load_config_or_raise(config_path, overlay_paths=overlay_paths) @@ -221,11 +225,12 @@ def _kafka_consume_core( ) match_expr = match if match is not None else filter_key - # Resolve the cluster (Task 1: no flag/binding yet -> default/single-cluster). - name = resolve_cluster_name(cfg.kafka, None) - cluster = cfg.kafka.clusters[name] + # Resolve the cluster: --cluster (explicit) > default > single-cluster. + # No pattern binding on consume (no --pattern option) -> binding_cluster None. + name = resolve_cluster_name(cfg.kafka, explicit=cluster) + resolved = cfg.kafka.clusters[name] - resolved_timeout = _resolve_timeout(timeout, cluster.timeout_seconds) + resolved_timeout = _resolve_timeout(timeout, resolved.timeout_seconds) if resolved_timeout <= 0: # timeout <= 0 makes the poll deadline already-passed, so the loop never # runs and consume returns ok:0 — a silent no-op masquerading as success. @@ -235,7 +240,7 @@ def _kafka_consume_core( ) # D6: default lookback = resolved timeout. resolved_lookback = float(lookback) if lookback is not None else resolved_timeout - group = _resolve_group(consumer_group, cluster) + group = _resolve_group(consumer_group, resolved) # Build the optional jq filter as an inline predicate so consume_window can # apply it incrementally AND short-circuit as soon as --expect-count matching @@ -251,7 +256,7 @@ def _kafka_consume_core( def predicate(msg, _expr=match_expr): return jq_bool(msg, _expr) - client = new_kafka_client(cluster, group_id=group) + client = new_kafka_client(resolved, group_id=group) matched = client.consume_window( topic, lookback_seconds=resolved_lookback, @@ -304,6 +309,7 @@ def predicate(msg, _expr=match_expr): @click.option("--expect-count", "expect_count", type=int, default=None, help="Min expected count") @click.option("--from-beginning", "from_beginning", is_flag=True, default=False) @click.option("--consumer-group", "consumer_group", default=None, help="Consumer group override") +@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)") @click.pass_context def kafka_consume( ctx: click.Context, @@ -315,6 +321,7 @@ def kafka_consume( expect_count: int | None, from_beginning: bool, consumer_group: str | None, + cluster: str | None, ) -> None: """Consume messages from a Kafka topic window.""" config_path = ctx.obj.get("config_path") if ctx.obj else None @@ -329,6 +336,7 @@ def kafka_consume( expect_count, from_beginning, consumer_group, + cluster, overlay_paths=list(ovs) if ovs else None, ) @@ -419,6 +427,7 @@ def _kafka_assert_core( from_beginning: bool, consumer_group: str | None, assertion: str | None, + cluster: str | None = None, overlay_paths: list[str] | None = None, ) -> dict: cfg = load_config_or_raise(config_path, overlay_paths=overlay_paths) @@ -436,6 +445,9 @@ def _kafka_assert_core( pattern_match: str | None = None inferred_topic: str | None = topic + # The pattern's cluster is the binding value for cluster resolution + # (explicit --cluster, if given, wins — see resolve_cluster_name). + binding_cluster: str | None = None if pattern is not None: if pattern not in cfg.kafka.patterns: raise TemplateNotFound( @@ -444,6 +456,7 @@ def _kafka_assert_core( ) pat = cfg.kafka.patterns[pattern] pattern_match = pat.match + binding_cluster = pat.cluster # Topic inferred from the pattern when --topic is omitted. if inferred_topic is None: inferred_topic = pat.topic @@ -460,15 +473,17 @@ def _kafka_assert_core( {"timeout": resolved_timeout}, ) resolved_lookback = float(lookback) if lookback is not None else resolved_timeout - # Resolve the cluster (Task 1: no flag/binding yet -> default/single-cluster; - # Task 2 passes the pattern's cluster as binding_cluster). - name = resolve_cluster_name(cfg.kafka, None) - cluster = cfg.kafka.clusters[name] - group = _resolve_group(consumer_group, cluster) + # Resolve the cluster: --cluster (explicit) > pattern.cluster (binding) > + # default > single-cluster. + name = resolve_cluster_name( + cfg.kafka, explicit=cluster, binding_cluster=binding_cluster + ) + resolved = cfg.kafka.clusters[name] + group = _resolve_group(consumer_group, resolved) # DESIGN §9.3: a custom assertion mode evaluates the full consumed window. if assertion is not None: - client = new_kafka_client(cluster, group_id=group) + client = new_kafka_client(resolved, group_id=group) messages = client.consume_window( inferred_topic, lookback_seconds=resolved_lookback, @@ -509,7 +524,7 @@ def _kafka_assert_core( filled_pattern_match=filled_pattern_match, ) - client = new_kafka_client(cluster, group_id=group) + client = new_kafka_client(resolved, group_id=group) start = time.monotonic() matched, scanned = client.find_in_window( inferred_topic, @@ -593,6 +608,7 @@ def _kafka_assert_core( default=None, help="Named custom assertion mode", ) +@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)") @click.pass_context def kafka_assert( ctx: click.Context, @@ -607,6 +623,7 @@ def kafka_assert( from_beginning: bool, consumer_group: str | None, assertion: str | None, + cluster: str | None, ) -> None: """Assert a matching message exists in a Kafka window.""" config_path = ctx.obj.get("config_path") if ctx.obj else None @@ -624,6 +641,7 @@ def kafka_assert( from_beginning, consumer_group, assertion, + cluster, overlay_paths=list(ovs) if ovs else None, ) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 1c5ffdd..665f686 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -278,3 +278,31 @@ def require_kafka(): except Exception as exc: # noqa: BLE001 - any failure means no service pytest.skip(f"Kafka broker at {broker} unavailable: {exc}") return broker + + +@pytest.fixture +def require_kafka_second_broker(): + """Skip if a SECOND live Kafka broker is unreachable. + + Yields the second broker address (``$AGCTL_TEST_KAFKA_BROKER_2``). Used by + the multi-cluster integration test, which needs two independent brokers so + it can prove ``--cluster`` routes produce/assert to the named cluster. + Unset (the common single-broker dev/CI case) => skip, never fail. + """ + broker = os.environ.get("AGCTL_TEST_KAFKA_BROKER_2") + if not broker: + pytest.skip( + "AGCTL_TEST_KAFKA_BROKER_2 not set; skipping multi-cluster Kafka test" + ) + + try: + from confluent_kafka import admin + except ImportError: + pytest.skip("confluent_kafka not installed; skipping live Kafka test") + + try: + client = admin.AdminClient({"bootstrap.servers": broker}) + client.list_topics(timeout=2) + except Exception as exc: # noqa: BLE001 - any failure means no service + pytest.skip(f"Second Kafka broker at {broker} unavailable: {exc}") + return broker diff --git a/tests/integration/test_kafka_commands.py b/tests/integration/test_kafka_commands.py index 039c119..aec9823 100644 --- a/tests/integration/test_kafka_commands.py +++ b/tests/integration/test_kafka_commands.py @@ -89,3 +89,85 @@ def test_kafka_produce_then_assert(require_kafka): envelope = json.loads(assert_env.output) assert envelope["ok"] is True assert envelope["result"]["matched"] is True + + +def test_kafka_multi_cluster_produce_then_assert( + require_kafka, require_kafka_second_broker, tmp_path +): + """Multi-cluster round-trip: produce to cluster ``two``, then assert on + cluster ``two`` -- proving ``--cluster`` routes both sides to the same + (second) broker, distinct from the default cluster ``one``. + + Self-skips when ``AGCTL_TEST_KAFKA_BROKER_2`` is unset (the common + single-broker case), mirroring the ``require_kafka`` skip pattern. + """ + broker1 = require_kafka + broker2 = require_kafka_second_broker + + # Two-cluster config with literal broker addresses (no env interpolation). + cfg = tmp_path / "agctl.yaml" + cfg.write_text( + 'version: "3"\n' + "kafka:\n" + " clusters:\n" + " one:\n" + f" brokers: ['{broker1}']\n" + " default_consumer_group: agctl-consumer\n" + " two:\n" + f" brokers: ['{broker2}']\n" + " default_consumer_group: agctl-consumer\n" + " default_cluster: one\n" + ) + env = _env(broker1) + topic = "agctl-it-multi" + body = json.dumps({"event": "MC", "nonce": "multi-cluster-789"}) + runner = CliRunner() + + # 1. Produce to cluster `two`. + produce = runner.invoke( + cli, + [ + "--config", + str(cfg), + "kafka", + "produce", + "--topic", + topic, + "--message", + body, + "--cluster", + "two", + ], + env=env, + ) + assert produce.exit_code == 0, produce.output + prod_env = json.loads(produce.output) + assert prod_env["ok"] is True + + # Allow the broker a moment to make the produced message readable. + time.sleep(1) + + # 2. Assert the message on cluster `two`. + assert_env = runner.invoke( + cli, + [ + "--config", + str(cfg), + "kafka", + "assert", + "--topic", + topic, + "--contains", + body, + "--timeout", + "10", + "--from-beginning", + "--cluster", + "two", + ], + env=env, + ) + assert assert_env.exit_code == 0, assert_env.output + envelope = json.loads(assert_env.output) + assert envelope["ok"] is True + assert envelope["result"]["matched"] is True diff --git a/tests/unit/test_kafka_commands.py b/tests/unit/test_kafka_commands.py index 89f76d7..e65ce67 100644 --- a/tests/unit/test_kafka_commands.py +++ b/tests/unit/test_kafka_commands.py @@ -321,6 +321,9 @@ def _install(messages, producer=None): def factory(cluster, group_id=None): # Preserve the test's canned messages; honor a caller group_id by # simply returning the same client (group has no effect on fakes). + # Capture the resolved cluster so multi-cluster tests can assert + # which named cluster the flag/binding resolved to. + captured["cluster"] = cluster return client monkeypatch.setattr(kafka_commands, "new_kafka_client", factory) @@ -1199,3 +1202,131 @@ def test_resolve_cluster_name_no_cluster_specified_errors(): kafka_commands.resolve_cluster_name(k, None) assert exc.value.message == "No kafka cluster specified" assert exc.value.detail == {} + + +# --------------------------------------------------------------------------- # +# Task 2: --cluster flag + per-pattern cluster binding +# Two-cluster configs: main (broker-a, default) + analytics (broker-b). The +# `install_fake` seam captures the resolved KafkaCluster passed to +# new_kafka_client, so tests assert which cluster was selected by inspecting +# ``captured["cluster"].brokers``. +# --------------------------------------------------------------------------- # + + +def _write_two_cluster_cfg(tmp_path, *, pattern_cluster=None): + """Write a two-cluster v3 config: main (broker-a, default) + analytics + (broker-b). When ``pattern_cluster`` is given, add a pattern ``ord`` + (topic=orders, match=.value.x) bound to that cluster.""" + lines = [ + 'version: "3"', + "kafka:", + " clusters:", + " main:", + " brokers: [broker-a:9092]", + " analytics:", + " brokers: [broker-b:9092]", + " default_cluster: main", + ] + if pattern_cluster is not None: + lines += [ + " patterns:", + " ord:", + " topic: orders", + " match: '.value.x'", + f" cluster: {pattern_cluster}", + ] + cfg = tmp_path / "agctl.yaml" + cfg.write_text("\n".join(lines) + "\n") + return cfg + + +def test_kafka_produce_explicit_cluster(install_fake, tmp_path): + """`--cluster analytics` selects the analytics cluster: the fake client is + built from analytics's brokers (broker-b), not the default main (broker-a).""" + cap = install_fake([]) + cfg = _write_two_cluster_cfg(tmp_path) + result = _run( + [ + "--config", str(cfg), + "kafka", "produce", + "--topic", "t", + "--message", '{"a":1}', + "--cluster", "analytics", + ] + ) + payload = _payload(result) + assert result.exit_code == 0 + assert payload["ok"] is True + # The factory received the analytics cluster (broker-b), not main (broker-a). + assert cap["cluster"].brokers == ["broker-b:9092"] + + +def test_kafka_assert_pattern_resolves_cluster(install_fake, tmp_path): + """`assert --pattern ord` (no --cluster, no --topic) resolves BOTH the topic + and the cluster from the pattern binding (analytics). The fake client is + built from analytics and consumes the pattern's topic `orders`.""" + cap = install_fake([_msg("orders", {"x": 1}, "k1")]) + cfg = _write_two_cluster_cfg(tmp_path, pattern_cluster="analytics") + result = _run( + [ + "--config", str(cfg), + "kafka", "assert", + "--pattern", "ord", + "--timeout", "2", + ] + ) + payload = _payload(result) + assert result.exit_code == 0 + assert payload["ok"] is True + # Topic inferred from the pattern. + assert payload["result"]["topic"] == "orders" + # Cluster resolved from the pattern's binding (analytics / broker-b). + assert cap["cluster"].brokers == ["broker-b:9092"] + + +def test_kafka_assert_cluster_flag_overrides_pattern(install_fake, tmp_path): + """Precedence: `--cluster main` (explicit) beats the pattern's + `cluster: analytics` (binding). Captured cluster is main (broker-a). + + Note: override of a pattern binding is only exercisable on `kafka assert`, + the sole command that carries a pattern binding (produce/consume have no + --pattern, so binding_cluster is always None there per the Task 2 spec).""" + cap = install_fake([_msg("orders", {"x": 1}, "k1")]) + cfg = _write_two_cluster_cfg(tmp_path, pattern_cluster="analytics") + result = _run( + [ + "--config", str(cfg), + "kafka", "assert", + "--pattern", "ord", + "--cluster", "main", + "--timeout", "2", + ] + ) + payload = _payload(result) + assert result.exit_code == 0 + assert payload["ok"] is True + # Explicit --cluster main wins over the pattern's analytics binding. + assert cap["cluster"].brokers == ["broker-a:9092"] + + +def test_kafka_assert_unknown_cluster_flag_error(install_fake, tmp_path): + """`--cluster ghost` surfaces as a ConfigError at the CLI (flag path). The + helper-level contract is covered by test_resolve_cluster_name_unknown_cluster_errors; + this adds the flag-path coverage Task 1 deferred.""" + install_fake([]) + cfg = _write_two_cluster_cfg(tmp_path) + result = _run( + [ + "--config", str(cfg), + "kafka", "assert", + "--topic", "t", + "--contains", '{"a":1}', + "--timeout", "2", + "--cluster", "ghost", + ] + ) + payload = _payload(result) + assert result.exit_code == 2 + assert payload["error"]["type"] == "ConfigError" + assert "Unknown kafka cluster: ghost" in payload["error"]["message"] + assert payload["error"]["detail"]["cluster"] == "ghost" From 0384ed526c8ce9546f9c3da4b4df08a27b179c64 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 01:03:19 +0300 Subject: [PATCH 04/12] feat(mock): per-cluster Kafka reactors via per-reactor client map Co-Authored-By: Claude --- agctl/commands/mock_commands.py | 31 +++---- agctl/config/validator.py | 69 ++++++++++------ agctl/mock/engine.py | 18 ++-- tests/unit/test_mock_commands.py | 92 +++++++++++++++++++-- tests/unit/test_mock_engine.py | 137 ++++++++++++++++++++++++++----- tests/unit/test_validator.py | 61 ++++++++++++-- 6 files changed, 330 insertions(+), 78 deletions(-) diff --git a/agctl/commands/mock_commands.py b/agctl/commands/mock_commands.py index 8f7447a..b51f230 100644 --- a/agctl/commands/mock_commands.py +++ b/agctl/commands/mock_commands.py @@ -117,7 +117,7 @@ def new_mock_engine( run_http: bool, run_kafka: bool, http_listen: str, - kafka_client: KafkaClient | None, + kafka_clients: dict[str, "KafkaClient"] | None = None, fail_fast: bool = False, duration: float | None = None, until_stopped: bool = True, @@ -130,7 +130,7 @@ def new_mock_engine( run_http=run_http, run_kafka=run_kafka, http_listen=http_listen, - kafka_client=kafka_client, + kafka_clients=kafka_clients, fail_fast=fail_fast, duration=duration, until_stopped=until_stopped, @@ -469,20 +469,21 @@ def mock_run( # Guard 2+3: Resolve engines to run run_http, run_kafka = _resolve_engines(only, cfg.mocks) - # Guard 5: If run_kafka, resolve a default cluster and require its brokers. - # (Task 1: all reactors share the single default client; per-reactor - # cluster selection lands in Task 3.) - kafka_client = None + # Guard 5: If run_kafka, resolve each reactor's cluster and build one + # KafkaClient per DISTINCT cluster (reactors sharing a cluster reuse the + # same client). The per-reactor client map is keyed by reactor name. + kafka_clients: dict[str, KafkaClient] | None = None if run_kafka: - cluster_name = resolve_cluster_name(cfg.kafka, None) - cluster = cfg.kafka.clusters[cluster_name] - if not cluster.brokers: - raise ConfigError( - "kafka.clusters..brokers is required when running Kafka reactors", - {"cluster": cluster_name}, + kafka_clients = {} + clients_by_cluster: dict[str, KafkaClient] = {} + for reactor_name, reactor in cfg.mocks.kafka.reactors.items(): + cluster_name = resolve_cluster_name( + cfg.kafka, None, binding_cluster=reactor.cluster ) - # Build KafkaClient (may raise ConfigError if kafka extra missing) - kafka_client = new_kafka_client(cluster) + if cluster_name not in clients_by_cluster: + cluster = cfg.kafka.clusters[cluster_name] + clients_by_cluster[cluster_name] = new_kafka_client(cluster) + kafka_clients[reactor_name] = clients_by_cluster[cluster_name] # Guard 6: Resolve http_listen if http_listen is not None: @@ -503,7 +504,7 @@ def mock_run( run_http=run_http, run_kafka=run_kafka, http_listen=http_listen, - kafka_client=kafka_client, + kafka_clients=kafka_clients, fail_fast=fail_fast, duration=duration, until_stopped=until_stopped, diff --git a/agctl/config/validator.py b/agctl/config/validator.py index 90f4a46..4aad701 100644 --- a/agctl/config/validator.py +++ b/agctl/config/validator.py @@ -122,35 +122,56 @@ def validate_config(cfg: Config) -> tuple[list[dict], list[dict]]: # --- mock server validation ----------------------------------------------- - # Check 1: mocks.kafka reactors require a resolvable default cluster with - # non-empty brokers. Resolution mirrors resolve_cluster_name (default_cluster - # -> single-cluster auto-default) but is inlined here so config/ stays free - # of a commands/ import. (Reactor.cluster dangling-ref is deferred to Task 3 - # where it is consumed.) + # Check 1: each mocks.kafka reactor must bind a resolvable cluster with + # non-empty brokers. Resolution mirrors resolve_cluster_name + # (reactor.cluster -> default_cluster -> single-cluster auto-default) but is + # inlined here so config/ stays free of a commands/ import. if ( cfg.mocks is not None and cfg.mocks.kafka is not None and cfg.mocks.kafka.reactors ): - reactor_cluster = cfg.kafka.default_cluster - if reactor_cluster is None and len(cfg.kafka.clusters) == 1: - reactor_cluster = next(iter(cfg.kafka.clusters)) - if reactor_cluster is None or reactor_cluster not in cfg.kafka.clusters: - errors.append( - { - "path": "mocks.kafka", - "message": "reactors require a resolvable default cluster", - } - ) - elif not cfg.kafka.clusters[reactor_cluster].brokers: - errors.append( - { - "path": "mocks.kafka", - "message": ( - f"kafka mocks require kafka.clusters.{reactor_cluster}.brokers" - ), - } - ) + for reactor_name, reactor in cfg.mocks.kafka.reactors.items(): + # (a) reactor.cluster dangling ref -> error at the .cluster field. + if ( + reactor.cluster is not None + and reactor.cluster not in cfg.kafka.clusters + ): + errors.append( + { + "path": f"mocks.kafka.reactors.{reactor_name}.cluster", + "message": ( + f"Reactor references unknown cluster " + f"'{reactor.cluster}'" + ), + } + ) + continue # cannot check brokers for an unknown cluster + + # (b) resolve cluster name: reactor.cluster -> default -> single. + resolved = reactor.cluster + if resolved is None: + resolved = cfg.kafka.default_cluster + if resolved is None and len(cfg.kafka.clusters) == 1: + resolved = next(iter(cfg.kafka.clusters)) + + # (c) resolvable + non-empty brokers. + if resolved is None or resolved not in cfg.kafka.clusters: + errors.append( + { + "path": f"mocks.kafka.reactors.{reactor_name}", + "message": "reactor requires a resolvable cluster", + } + ) + elif not cfg.kafka.clusters[resolved].brokers: + errors.append( + { + "path": f"mocks.kafka.reactors.{reactor_name}", + "message": ( + f"reactor requires kafka.clusters.{resolved}.brokers" + ), + } + ) # Check 2: Missing description warnings for stubs and reactors if cfg.mocks is not None: diff --git a/agctl/mock/engine.py b/agctl/mock/engine.py index 555a3e9..b319797 100644 --- a/agctl/mock/engine.py +++ b/agctl/mock/engine.py @@ -58,7 +58,7 @@ def __init__( run_http: bool, run_kafka: bool, http_listen: str, - kafka_client: Any, # KafkaClient or None (only used if run_kafka=True) + kafka_clients: dict[str, Any] | None = None, # reactor name -> KafkaClient (None if run_kafka=False) fail_fast: bool = False, duration: float | None = None, until_stopped: bool = True, @@ -72,7 +72,9 @@ def __init__( run_http: If True, start HTTP mock server. run_kafka: If True, start Kafka reactors. http_listen: HTTP listen address string (host:port). - kafka_client: KafkaClient instance (required if run_kafka=True). + kafka_clients: Mapping of reactor name -> KafkaClient (required if + run_kafka=True). Each reactor is wired to its own client so it can + target its own cluster. None when run_kafka=False. fail_fast: If True, reactor STOP → immediate exit 1. duration: If set, stop after this many seconds. until_stopped: Ignored (kept for API compatibility). @@ -83,7 +85,7 @@ def __init__( self._run_http = run_http self._run_kafka = run_kafka self._http_listen = http_listen - self._kafka_client = kafka_client + self._kafka_clients = kafka_clients self._fail_fast = fail_fast self._duration = duration self._emit_fn = emit_fn @@ -196,15 +198,17 @@ def start(self) -> None: if self._mocks is None or self._mocks.kafka is None: raise ConfigError("run_kafka=True but no Kafka mocks configured", {}) - if self._kafka_client is None: - raise ConfigError("run_kafka=True but kafka_client is None", {}) + if self._kafka_clients is None: + raise ConfigError("run_kafka=True but kafka_clients is None", {}) - # Build reactors and prepare (probe) each + # Build reactors, each wired to its own client (keyed by reactor + # name) so a reactor can target its own cluster. The reactor + # runtime class itself still takes a single ``client``. for name, reactor_config in self._mocks.kafka.reactors.items(): reactor = KafkaReactorClass( name=name, config=reactor_config, - client=self._kafka_client, + client=self._kafka_clients[name], emit_event=self.emit_event, stop_event=self._stop, fail_fast=self._fail_fast, diff --git a/tests/unit/test_mock_commands.py b/tests/unit/test_mock_commands.py index 7e41820..b4f6a34 100644 --- a/tests/unit/test_mock_commands.py +++ b/tests/unit/test_mock_commands.py @@ -32,7 +32,7 @@ class TestMockRunCommand: """Test the mock run command with various scenarios.""" def test_only_http_with_stubs(self, temp_config, fake_engine): - """--only http with 2-stub mocks.http -> run_http=True, run_kafka=False, kafka_client=None.""" + """--only http with 2-stub mocks.http -> run_http=True, run_kafka=False, kafka_clients=None.""" config_content = """ version: "3" mocks: @@ -69,7 +69,7 @@ def test_only_http_with_stubs(self, temp_config, fake_engine): call_kwargs = mock_factory.call_args.kwargs assert call_kwargs["run_http"] is True assert call_kwargs["run_kafka"] is False - assert call_kwargs["kafka_client"] is None + assert call_kwargs["kafka_clients"] is None # Verify engine lifecycle fake_engine.start.assert_called_once() @@ -115,9 +115,79 @@ def test_only_kafka_with_reactors(self, temp_config, fake_engine): assert call_kwargs["run_http"] is False assert call_kwargs["run_kafka"] is True - # Verify kafka_client is a KafkaClient instance + # Verify kafka_clients is a dict mapping reactor name -> KafkaClient from agctl.clients.kafka_client import KafkaClient - assert isinstance(call_kwargs["kafka_client"], KafkaClient) + kafka_clients = call_kwargs["kafka_clients"] + assert isinstance(kafka_clients, dict) + assert "reactor1" in kafka_clients + assert isinstance(kafka_clients["reactor1"], KafkaClient) + + def test_mock_run_builds_per_cluster_clients(self, temp_config, fake_engine): + """mock run with two reactors binding two distinct clusters -> new_kafka_client + called once per distinct cluster, and kafka_clients maps each reactor name + to the client built for its resolved cluster.""" + config_content = """ +version: "3" +kafka: + clusters: + main: + brokers: + - "main:9092" + analytics: + brokers: + - "analytics:9092" + default_cluster: main +mocks: + kafka: + reactors: + rA: + topic: topicA + cluster: main + reaction: + topic: out + value: {} + rB: + topic: topicB + cluster: analytics + reaction: + topic: out + value: {} +""" + temp_config.write_text(config_content) + + recorded_clusters = [] + + def fake_client_factory(cluster, group_id=None): + recorded_clusters.append(cluster) + return MagicMock() # fake client; engine is also faked + + with patch( + "agctl.commands.mock_commands.new_kafka_client", + side_effect=fake_client_factory, + ), patch( + "agctl.commands.mock_commands.new_mock_engine", + return_value=fake_engine, + ) as mock_factory: + result = CliRunner().invoke( + cli, + ["--config", str(temp_config), "mock", "run", "--only", "kafka"], + catch_exceptions=False, + ) + + assert result.exit_code == 0 + + # One new_kafka_client call per distinct cluster (main, analytics). + assert len(recorded_clusters) == 2 + brokers_seen = {tuple(c.brokers) for c in recorded_clusters} + assert brokers_seen == {("main:9092",), ("analytics:9092",)} + + # kafka_clients maps each reactor name to a client. + call_kwargs = mock_factory.call_args.kwargs + kafka_clients = call_kwargs["kafka_clients"] + assert set(kafka_clients.keys()) == {"rA", "rB"} + # Reactors sharing a cluster would reuse the same instance; here each + # has its own cluster, so the two client objects differ. + assert kafka_clients["rA"] is not kafka_clients["rB"] def test_only_http_without_mocks_http(self, temp_config): """--only http with no mocks.http -> exit 2, ConfigError envelope.""" @@ -181,8 +251,16 @@ def test_no_mocks_section_no_only(self, temp_config, fake_engine): fake_engine.shutdown.assert_called_once() def test_kafka_reactors_without_brokers(self, temp_config): - """mocks.kafka reactors present but resolved cluster has empty brokers -> - exit 2, ConfigError envelope.""" + """mocks.kafka reactors whose resolved cluster has empty brokers -> exit 2. + + The runtime brokers guard was dropped in Task 3 (per-reactor resolution + replaced it); empty-brokers validation now lives in the validator's + per-reactor check. At ``mock run`` time the empty broker list surfaces as + a probe failure (ConnectionError when confluent_kafka is installed, or a + ConfigError for the missing extra when it is not). Either way the run + fails fast with exit 2 and a single error envelope — the contract this + test pins. + """ config_content = """ version: "3" kafka: @@ -214,8 +292,6 @@ def test_kafka_reactors_without_brokers(self, temp_config): envelope = json.loads(output_lines[0]) assert envelope["ok"] is False assert envelope["command"] == "mock.run" - assert envelope["error"]["type"] == "ConfigError" - assert "brokers" in envelope["error"]["message"] def test_duration_and_until_stopped_mutually_exclusive(self, temp_config): """--duration and --until-stopped together -> exit 2, ConfigError envelope.""" diff --git a/tests/unit/test_mock_engine.py b/tests/unit/test_mock_engine.py index 5f1fb35..8392ede 100644 --- a/tests/unit/test_mock_engine.py +++ b/tests/unit/test_mock_engine.py @@ -64,12 +64,14 @@ def __init__(self, probe_raises=None, consume_loop_returns_immediately=True): self._probe_raises = probe_raises self._consume_loop_returns_immediately = consume_loop_returns_immediately self.probe_called = False + self.probed_topic = None self.consume_loop_called = False self._stop_signal = None def probe(self, topic, group_id): """Fake probe - can raise if configured.""" self.probe_called = True + self.probed_topic = topic # records which reactor's topic was probed if self._probe_raises: raise self._probe_raises @@ -249,7 +251,7 @@ def capture_emit(line): run_http=False, run_kafka=False, http_listen="127.0.0.1:18080", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-123", ) @@ -304,7 +306,7 @@ def capture_emit(line): run_http=True, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-456", ) @@ -367,7 +369,7 @@ def make_fake_http(*args, **kwargs): run_http=False, # Only test kafka probe failure run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"reactor1": fake_client}, emit_fn=capture_emit, run_id="test-run-789", ) @@ -417,7 +419,7 @@ def capture_emit(line): run_http=True, run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"reactor1": fake_client}, emit_fn=capture_emit, run_id="test-run-abc", ) @@ -467,7 +469,7 @@ def capture_emit(line): run_http=False, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-concurrent", ) @@ -525,7 +527,7 @@ def capture_emit(line): run_http=False, run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"reactor1": fake_client}, emit_fn=capture_emit, run_id="test-run-failfast", fail_fast=True, @@ -574,7 +576,7 @@ def capture_emit(line): run_http=False, run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"reactor1": fake_client}, emit_fn=capture_emit, run_id="test-run-nonfatal", fail_fast=False, # default continue mode: non-fatal error → COMMIT, not STOP @@ -628,7 +630,7 @@ def capture_emit(line): run_http=False, run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"reactor1": fake_client}, emit_fn=capture_emit, run_id="test-run-winddown", fail_fast=False, @@ -698,7 +700,7 @@ def capture_emit(line): run_http=False, run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"failing-reactor": fake_client, "healthy-reactor": fake_client}, emit_fn=capture_emit, run_id="test-run-thread-death", fail_fast=False, # default: other reactors must continue @@ -748,7 +750,7 @@ def capture_emit(line): run_http=False, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-tally", ) @@ -804,7 +806,7 @@ def make_http_server_that_raises(*args, **kwargs): run_http=True, run_kafka=False, http_listen="127.0.0.1:18080", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-portinuse", ) @@ -830,7 +832,7 @@ def capture_emit(line): run_http=False, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-duration", duration=0.1, # 100ms @@ -874,7 +876,7 @@ def capture_emit(line): run_http=False, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-signals", ) @@ -946,7 +948,7 @@ def capture_emit(line): run_http=False, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id=None, # Should default to PID ) @@ -995,7 +997,7 @@ def capture_emit(line): run_http=True, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-step0-bad-http", ) @@ -1041,7 +1043,7 @@ def capture_emit(line): run_http=False, run_kafka=True, http_listen="127.0.0.1:0", - kafka_client=fake_client, + kafka_clients={"r1": fake_client}, emit_fn=capture_emit, run_id="test-run-step0-bad-kafka", ) @@ -1089,7 +1091,7 @@ def capture_emit(line): run_http=True, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-step0-no-jq", ) @@ -1145,7 +1147,7 @@ def capture_emit(line): run_http=True, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-step0-body-only", ) @@ -1202,7 +1204,7 @@ def capture_emit(line): run_http=True, run_kafka=False, http_listen="127.0.0.1:0", - kafka_client=None, + kafka_clients=None, emit_fn=capture_emit, run_id="test-run-step0-object-placement", ) @@ -1229,3 +1231,100 @@ def test_default_emit_renders_non_ascii_utf8(capsys): assert "Иван" in out assert "\\u" not in out assert json.loads(out)["body"] == {"name": "Иван"} + + +# ============================================================================= +# Per-cluster reactor wiring (Task 3) +# ============================================================================= + + +def test_reactors_use_per_cluster_clients(): + """Each reactor is wired to its own client from ``kafka_clients`` keyed by + reactor name. Two reactors (rA, rB) each get a distinct FakeKafkaClient; + after ``start()`` each client must have probed ITS reactor's topic (proving + the reactor→client map was honored, not a single shared client).""" + captured_lines = [] + + def capture_emit(line): + captured_lines.append(line.copy()) + + clientA = FakeKafkaClient() + clientB = FakeKafkaClient() + + mocks = MocksConfig( + kafka=KafkaMockConfig( + reactors={ + "rA": KafkaReactor( + topic="topicA", + reaction=KafkaReaction(topic="outA", value={}), + ), + "rB": KafkaReactor( + topic="topicB", + reaction=KafkaReaction(topic="outB", value={}), + ), + } + ) + ) + + engine = MockEngine( + mocks=mocks, + run_http=False, + run_kafka=True, + http_listen="127.0.0.1:0", + kafka_clients={"rA": clientA, "rB": clientB}, + emit_fn=capture_emit, + run_id="test-run-per-cluster", + ) + + engine.start() + + # Both reactors appear in the started line. + started = [l for l in captured_lines if l.get("event") == "started"] + assert len(started) == 1 + reactor_names = {r["name"] for r in started[0]["kafka"]["reactors"]} + assert reactor_names == {"rA", "rB"} + + # Each client probed its own reactor's topic — proving the per-reactor + # client map was honored (a single shared client would leave one un-probed). + assert clientA.probe_called is True + assert clientB.probe_called is True + assert clientA.probed_topic == "topicA" + assert clientB.probed_topic == "topicB" + + # Cleanup + engine._stop.set() + engine.run() + engine.shutdown() + + +def test_run_kafka_without_kafka_clients_raises(): + """run_kafka=True but kafka_clients is None -> ConfigError (guard holds).""" + captured_lines = [] + + def capture_emit(line): + captured_lines.append(line.copy()) + + mocks = MocksConfig( + kafka=KafkaMockConfig( + reactors={ + "r1": KafkaReactor( + topic="t", + reaction=KafkaReaction(topic="out", value={}), + ) + } + ) + ) + + engine = MockEngine( + mocks=mocks, + run_http=False, + run_kafka=True, + http_listen="127.0.0.1:0", + kafka_clients=None, + emit_fn=capture_emit, + run_id="test-run-no-clients", + ) + + with pytest.raises(ConfigError): + engine.start() + diff --git a/tests/unit/test_validator.py b/tests/unit/test_validator.py index 137a0bd..a85dbc7 100644 --- a/tests/unit/test_validator.py +++ b/tests/unit/test_validator.py @@ -296,8 +296,8 @@ def test_mock_kafka_requires_resolvable_default_cluster_error(): ) errors, warnings = validate_config(cfg) assert len(errors) == 1 - assert errors[0]["path"] == "mocks.kafka" - assert errors[0]["message"] == "reactors require a resolvable default cluster" + assert errors[0]["path"] == "mocks.kafka.reactors.r" + assert "cluster" in errors[0]["message"] def test_mock_kafka_with_cluster_no_error(): @@ -320,8 +320,8 @@ def test_mock_kafka_with_cluster_no_error(): ), ) errors, warnings = validate_config(cfg) - # Should not have the mocks.kafka error - assert not any(e["path"] == "mocks.kafka" for e in errors) + # Should not have the mocks.kafka.reactors error + assert not any(e["path"].startswith("mocks.kafka.reactors") for e in errors) # --- v3 cluster cross-ref validation ---------------------------------------- @@ -347,11 +347,62 @@ def test_reactor_default_cluster_missing_brokers_errors(): ), ) errors, warnings = validate_config(cfg) - mocks_errors = [e for e in errors if e["path"] == "mocks.kafka"] + mocks_errors = [e for e in errors if e["path"] == "mocks.kafka.reactors.r"] assert len(mocks_errors) == 1 assert "kafka.clusters.default.brokers" in mocks_errors[0]["message"] +def test_reactor_cluster_dangling_ref_errors(): + """A KafkaReactor whose ``cluster`` names an unknown cluster -> error at + mocks.kafka.reactors..cluster.""" + cfg = _cfg( + mocks=MocksConfig( + kafka=KafkaMockConfig( + reactors={ + "r": KafkaReactor( + topic="t", + cluster="ghost", + reaction=KafkaReaction(topic="out", value={}), + ) + } + ) + ), + kafka=KafkaConfig( + clusters={"main": KafkaCluster(brokers=["h:9092"])}, + default_cluster="main", + ), + ) + errors, warnings = validate_config(cfg) + paths = [e["path"] for e in errors] + assert "mocks.kafka.reactors.r.cluster" in paths + + +def test_reactor_resolved_cluster_missing_brokers_errors(): + """A reactor binding ``cluster="main"`` where main.brokers is empty -> error + at mocks.kafka.reactors., message names the cluster.""" + cfg = _cfg( + mocks=MocksConfig( + kafka=KafkaMockConfig( + reactors={ + "r": KafkaReactor( + topic="t", + cluster="main", + reaction=KafkaReaction(topic="out", value={}), + ) + } + ) + ), + kafka=KafkaConfig( + clusters={"main": KafkaCluster(brokers=[])}, + default_cluster="main", + ), + ) + errors, warnings = validate_config(cfg) + reactor_errors = [e for e in errors if e["path"] == "mocks.kafka.reactors.r"] + assert len(reactor_errors) == 1 + assert "kafka.clusters.main.brokers" in reactor_errors[0]["message"] + + def test_default_cluster_dangling_ref_errors(): """cfg.kafka.default_cluster names a cluster absent from clusters -> error at kafka.default_cluster.""" From 04c88303841795f88b9f1df2601e237f7f20d51c Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 01:08:21 +0300 Subject: [PATCH 05/12] docs(arch): sync mock-kafka validator bullet to per-reactor check Co-Authored-By: Claude --- docs/ARCHITECTURE.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 4cfeafc..1d963c8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -286,7 +286,12 @@ catching what Pydantic cannot: - `defaults.database_connection` → unknown connection → **error**. - Any template/pattern/stub/reactor missing `description` → **warning** (discovery degrades). -- `mocks.kafka` with reactors but no top-level `kafka.brokers` → **error**. +- Each `mocks.kafka.reactors.` must resolve a cluster + (`reactor.cluster` → `kafka.default_cluster` → single-cluster auto-default, + mirroring `resolve_cluster_name`); a dangling `reactor.cluster` → **error** at + `…reactors..cluster`, and a resolved cluster with empty `brokers` → + **error** at `…reactors.`. (Inlined in the validator so `config/` stays + free of a `commands/` import.) - Path-template shadowing: a `{name}` segment ahead of a literal at the same position (`/orders/{id}` before `/orders/bulk`) → **warning** (first-match-wins; the literal stub silently never fires). Method-agnostic — known limitation. From 2ae8ff04c273c7edc6daee190170126ba78ac785 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 01:18:54 +0300 Subject: [PATCH 06/12] fix(mock): per-reactor brokers guard at mock run + restore test Co-Authored-By: Claude --- agctl/commands/mock_commands.py | 13 ++++- tests/unit/test_mock_commands.py | 86 ++++++++++++++++++++++++++++---- 2 files changed, 89 insertions(+), 10 deletions(-) diff --git a/agctl/commands/mock_commands.py b/agctl/commands/mock_commands.py index b51f230..e034777 100644 --- a/agctl/commands/mock_commands.py +++ b/agctl/commands/mock_commands.py @@ -480,8 +480,19 @@ def mock_run( cluster_name = resolve_cluster_name( cfg.kafka, None, binding_cluster=reactor.cluster ) + cluster = cfg.kafka.clusters[cluster_name] + # Per-reactor brokers guard (spec §11): a reactor whose resolved + # cluster has empty brokers must fail fast with a clear + # ConfigError at mock run, BEFORE any client build/probe. This + # restores the runtime guarantee dropped in Task 3 — but per + # reactor (honoring the brief's per-reactor resolution), not via + # the old single-cluster guard. + if not cluster.brokers: + raise ConfigError( + f"kafka.clusters.{cluster_name}.brokers is required when running Kafka reactors", + {"reactor": reactor_name, "cluster": cluster_name}, + ) if cluster_name not in clients_by_cluster: - cluster = cfg.kafka.clusters[cluster_name] clients_by_cluster[cluster_name] = new_kafka_client(cluster) kafka_clients[reactor_name] = clients_by_cluster[cluster_name] diff --git a/tests/unit/test_mock_commands.py b/tests/unit/test_mock_commands.py index b4f6a34..65cb4ec 100644 --- a/tests/unit/test_mock_commands.py +++ b/tests/unit/test_mock_commands.py @@ -189,6 +189,69 @@ def fake_client_factory(cluster, group_id=None): # has its own cluster, so the two client objects differ. assert kafka_clients["rA"] is not kafka_clients["rB"] + def test_mock_run_reuses_client_for_shared_cluster(self, temp_config, fake_engine): + """Two reactors bound to the SAME cluster -> new_kafka_client called + exactly once, and both reactors share the same client instance. + + Pins the DRY/reuse path that ``test_mock_run_builds_per_cluster_clients`` + (2 reactors / 2 clusters) does not cover. + """ + config_content = """ +version: "3" +kafka: + clusters: + shared: + brokers: + - "shared:9092" + default_cluster: shared +mocks: + kafka: + reactors: + rA: + topic: topicA + cluster: shared + reaction: + topic: out + value: {} + rB: + topic: topicB + cluster: shared + reaction: + topic: out + value: {} +""" + temp_config.write_text(config_content) + + call_count = {"n": 0} + + def fake_client_factory(cluster, group_id=None): + call_count["n"] += 1 + return MagicMock() # fake client; engine is also faked + + with patch( + "agctl.commands.mock_commands.new_kafka_client", + side_effect=fake_client_factory, + ), patch( + "agctl.commands.mock_commands.new_mock_engine", + return_value=fake_engine, + ) as mock_factory: + result = CliRunner().invoke( + cli, + ["--config", str(temp_config), "mock", "run", "--only", "kafka"], + catch_exceptions=False, + ) + + assert result.exit_code == 0 + + # The factory is called exactly once for the single shared cluster. + assert call_count["n"] == 1 + + # Both reactors map to the SAME client instance. + call_kwargs = mock_factory.call_args.kwargs + kafka_clients = call_kwargs["kafka_clients"] + assert set(kafka_clients.keys()) == {"rA", "rB"} + assert kafka_clients["rA"] is kafka_clients["rB"] + def test_only_http_without_mocks_http(self, temp_config): """--only http with no mocks.http -> exit 2, ConfigError envelope.""" config_content = """ @@ -251,15 +314,12 @@ def test_no_mocks_section_no_only(self, temp_config, fake_engine): fake_engine.shutdown.assert_called_once() def test_kafka_reactors_without_brokers(self, temp_config): - """mocks.kafka reactors whose resolved cluster has empty brokers -> exit 2. - - The runtime brokers guard was dropped in Task 3 (per-reactor resolution - replaced it); empty-brokers validation now lives in the validator's - per-reactor check. At ``mock run`` time the empty broker list surfaces as - a probe failure (ConnectionError when confluent_kafka is installed, or a - ConfigError for the missing extra when it is not). Either way the run - fails fast with exit 2 and a single error envelope — the contract this - test pins. + """mocks.kafka reactors whose resolved cluster has empty brokers -> + exit 2 with a clear ConfigError naming the reactor + cluster. + + The per-reactor brokers guard in ``mock_run`` (spec §11) fires BEFORE + any confluent_kafka import/probe, so this guarantee holds regardless of + whether the extra is installed. """ config_content = """ version: "3" @@ -293,6 +353,14 @@ def test_kafka_reactors_without_brokers(self, temp_config): assert envelope["ok"] is False assert envelope["command"] == "mock.run" + # Clear ConfigError naming the offending reactor + cluster (spec §11). + error = envelope["error"] + assert error["type"] == "ConfigError" + assert "brokers" in error["message"] + detail = error["detail"] + assert detail["reactor"] == "reactor1" + assert detail["cluster"] == "default" + def test_duration_and_until_stopped_mutually_exclusive(self, temp_config): """--duration and --until-stopped together -> exit 2, ConfigError envelope.""" config_content = """ From 06a730290957e98bc9c16e776c27932fcbc57813 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 01:32:09 +0300 Subject: [PATCH 07/12] feat(discover): surface kafka pattern resolved cluster Co-Authored-By: Claude --- agctl/commands/discover_commands.py | 6 +++ tests/unit/test_discover_command.py | 75 +++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/agctl/commands/discover_commands.py b/agctl/commands/discover_commands.py index 29ff568..09f63e1 100644 --- a/agctl/commands/discover_commands.py +++ b/agctl/commands/discover_commands.py @@ -28,6 +28,7 @@ from ..command import envelope, load_config_or_raise from ..config.models import parse_listen from ..errors import ConfigError, TemplateNotFound +from .kafka_commands import resolve_cluster_name __all__ = ["discover"] @@ -323,6 +324,11 @@ def _item_core(config_path: str | None, category: str, name: str, overlay_paths: "topic": pat.topic, "params": params, "example": _kafka_example(name, params), + # Resolved cluster name (DESIGN §6): pattern.cluster > default_cluster + # > single-cluster auto-default. Surfaces where this pattern asserts. + "cluster": resolve_cluster_name( + cfg.kafka, None, binding_cluster=pat.cluster + ), } if pat.match is not None: item["match"] = pat.match diff --git a/tests/unit/test_discover_command.py b/tests/unit/test_discover_command.py index d15da1b..96752eb 100644 --- a/tests/unit/test_discover_command.py +++ b/tests/unit/test_discover_command.py @@ -191,6 +191,81 @@ def test_item_kafka_pattern(monkeypatch): ) +_KAFKA_TWO_CLUSTER_CONFIG = ( + 'version: "3"\n' + "services:\n" + " demo:\n" + ' base_url: "http://localhost:9999"\n' + "kafka:\n" + " clusters:\n" + " main:\n" + " brokers:\n" + ' - "localhost:9092"\n' + " analytics:\n" + " brokers:\n" + ' - "analytics-host:9092"\n' + " default_cluster: main\n" + " patterns:\n" + " evt:\n" + ' description: "An analytics event"\n' + " topic: events\n" + ' match: \'.value.eventType == "EVT"\'\n' + " cluster: analytics\n" +) + + +def test_kafka_pattern_item_shows_cluster(tmp_path, monkeypatch): + """Multi-cluster: discover surfaces the pattern's bound cluster name.""" + result = _run_with( + ["--category", "kafka-patterns", "--name", "evt"], + _KAFKA_TWO_CLUSTER_CONFIG, + tmp_path, + monkeypatch, + ) + assert result.exit_code == 0 + payload = _payload(result) + assert payload["command"] == "discover.item" + res = payload["result"] + assert res["name"] == "evt" + # The pattern binds cluster="analytics" — that wins over default_cluster. + assert res["cluster"] == "analytics" + + +_KAFKA_DEFAULT_CLUSTER_CONFIG = ( + 'version: "3"\n' + "services:\n" + " demo:\n" + ' base_url: "http://localhost:9999"\n' + "kafka:\n" + " clusters:\n" + " main:\n" + " brokers:\n" + ' - "localhost:9092"\n' + " default_cluster: main\n" + " patterns:\n" + " evt:\n" + ' description: "A default-cluster event"\n' + " topic: events\n" + ' match: \'.value.eventType == "EVT"\'\n' +) + + +def test_kafka_pattern_item_cluster_defaults(tmp_path, monkeypatch): + """A pattern with no cluster field resolves to default_cluster.""" + result = _run_with( + ["--category", "kafka-patterns", "--name", "evt"], + _KAFKA_DEFAULT_CLUSTER_CONFIG, + tmp_path, + monkeypatch, + ) + assert result.exit_code == 0 + payload = _payload(result) + res = payload["result"] + assert res["name"] == "evt" + # No pattern cluster + default_cluster="main" → resolved cluster "main". + assert res["cluster"] == "main" + + def test_item_service(monkeypatch): result = _run(["--category", "services", "--name", "order-service"], monkeypatch) assert result.exit_code == 0 From 6e78dcd2c21c58755f19b38fe777aaf4a055cb99 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 01:50:29 +0300 Subject: [PATCH 08/12] docs(kafka): sync DESIGN/ARCHITECTURE/skills to multi-cluster v3 Co-Authored-By: Claude --- README.md | 2 +- docs/ARCHITECTURE.md | 30 ++++- docs/DESIGN.md | 117 +++++++++++------- skills/agctl-config/SKILL.md | 7 +- skills/agctl-config/reference/init-config.md | 11 +- .../agctl-config/reference/kafka-pattern.md | 28 +++-- skills/agctl-config/reference/mocks.md | 38 +++--- skills/agctl/SKILL.md | 26 ++-- 8 files changed, 168 insertions(+), 91 deletions(-) diff --git a/README.md b/README.md index 01e12cb..80fc04c 100644 --- a/README.md +++ b/README.md @@ -187,7 +187,7 @@ interpolation, with highest precedence: ```bash AGCTL_DEFAULTS__TIMEOUT_SECONDS=30 -AGCTL_KAFKA__DEFAULT_CONSUMER_GROUP=ci-consumer +AGCTL_KAFKA__CLUSTERS__DEFAULT__DEFAULT_CONSUMER_GROUP=ci-consumer AGCTL_DATABASE__CONNECTIONS__MAIN_DB__HOST=localhost AGCTL_DATABASE__CONNECTIONS__MAIN_DB__PASSWORD=supersecret AGCTL_SERVICES__ORDER_SERVICE__BASE_URL=http://order-svc:8080 diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 1d963c8..1dbabdc 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -220,7 +220,7 @@ discovers, parses, and validates `agctl.yaml` from scratch. This is what makes `config/loader.py::load_config` → `compose_config`, fixed order; any stage may fail the load with a `ConfigError`: ``` -discover_config_path → yaml.safe_load → interpolate → _check_version (base must be v2) +discover_config_path → yaml.safe_load → interpolate → _check_version (base must be v3) ↓ for each overlay (in flag order): discover_config_path (explicit only) → yaml.safe_load → interpolate @@ -266,13 +266,17 @@ deep-merged with highest precedence. Two refinements beyond the spec: - `database.connections.*.writable` — write safety gate must be set explicitly in config - `database.templates.*.mode` — read/write mode is a template authoring intent, not runtime config -**Version guard** — major only; `_check_version` (`loader.py`) rejects `major != TOOL_MAJOR_VERSION` (`"2"`) with a `ConfigError` whose message points at `agctl config migrate`. The version is the jq-dialect switch: `"2"` = envelope-rooted `match` (`.body.x` for HTTP, `.value.x` for Kafka); `"1"` (the legacy payload-rooted dialect) is rejected, not silently re-interpreted. `agctl config migrate` performs the file-level rewrite (`.body | ` / `.value | ` prefix on the three `match`-site families — see `config/migrate.py::migrate_match_exprs`); CLI `--match` flags in scripts/prompts are out of its scope and must be prefixed manually. +**Version guard** — major only; `_check_version` (`loader.py`) rejects `major != TOOL_MAJOR_VERSION` (`"3"`) with a `ConfigError` whose message points at `agctl config migrate`. The version is the **config schema version**: `"3"` restructured `Config.kafka` from a single flat object into a named map `kafka.clusters.` (mirroring `database.connections`) plus `default_cluster`. The `match` envelope-rooting introduced under `"2"` (`.body.x` for HTTP, `.value.x` for Kafka) is unchanged in v3; `"1"` and `"2"` (the legacy flat-`kafka:` schemas, `"1"` additionally payload-rooted) are rejected, not silently re-interpreted. `agctl config migrate` performs a STRUCTURAL lift (v1/v2 → v3: a flat `kafka:` block moves into `kafka.clusters.default` + `default_cluster: default` — see `config/migrate.py::migrate_config`/`_lift_kafka_clusters`); v1 sources additionally get the `.body | ` / `.value | ` prefix on the three `match`-site families (v2 exprs are already envelope-rooted and are not re-prefixed). CLI `--match` flags in scripts/prompts are out of its scope and must be prefixed manually (v1 inputs only). **Typed validation** (`Config.model_validate`, `models.py`) — Pydantic v2 tree -(`Config` → `ServiceConfig`, `KafkaConfig`/`KafkaSSL`, `DatabaseConfig`/…, -`HttpTemplate`, `Defaults`). Notable: `KafkaSSL.security_protocol` is +(`Config` → `ServiceConfig`, `KafkaConfig`/`KafkaCluster`/`KafkaSSL`, +`DatabaseConfig`/…, `HttpTemplate`, `Defaults`). Notable: `KafkaSSL.security_protocol` is upper-cased and restricted to `PLAINTEXT|SSL|SASL_SSL|SASL_PLAINTEXT` at load, so an invalid protocol fails fast rather than surfacing as an opaque broker error. +Under v3 `KafkaConfig` is a named map (`clusters: dict[str, KafkaCluster]`, +`default_cluster`, `patterns`), mirroring `DatabaseConfig.connections`; +`KafkaCluster` owns the per-cluster knobs formerly on `KafkaConfig` (brokers / ssl / +timeout / consumer group / schema registry). **Overlay types** — `PartialConfig` (a `Config` subclass with `version: str | None = None`) represents a fragment; version is inherited from the base at merge time. `ComposedConfig` is a `NamedTuple(config: Config, overrides: list[dict])`, where each override record is `{"path": "", "overlay": ""}` surfacing which overlay won which leaf. @@ -461,7 +465,21 @@ Exception mapping: `ConnectError`/`ConnectTimeout` → `ConnectionFailure`; confluent-kafka wrapper, transport-agnostic — it takes an `extra_conf` dict (`security.protocol`/`ssl.*`); the typed→librdkafka translation is owned by the -command layer (`_kafka_ssl_conf`). +command layer (`_kafka_ssl_conf`, which now takes the resolved `KafkaCluster`). + +**Cluster resolution + client seam (v3).** Multi-cluster selection is decoupled +from client construction: `kafka_commands.resolve_cluster_name(cfg.kafka, +explicit, binding_cluster)` returns the cluster **name** to use (precedence: +`--cluster` flag > a pattern/reactor `.cluster` binding > `kafka.default_cluster` +> the single defined cluster when exactly one exists), mirroring +`db_commands.resolve_connection_name`. It raises `ConfigError` (exit 2) when no +name resolves (>1 cluster, no default) or the resolved name is absent from +`kafka.clusters`. Callers then index `cfg.kafka.clusters[name]` and build the +client via `new_kafka_client(cluster, group_id=None)` (test seam — tests +monkeypatch it). `_kafka_ssl_conf(cluster)` and `_resolve_timeout`/`_resolve_group` +read the resolved cluster's knobs. The mock engine receives one pre-built +`KafkaClient` per reactor (`kafka_clients: dict[reactor_name → KafkaClient]`; +reactors sharing a cluster reuse a single client built via `clients_by_cluster`). - **`produce`** — JSON-encodes the value, registers a delivery-report callback, `flush(timeout=30)`, returns the `kafka.produce` shape. A delivery error **or** @@ -905,6 +923,6 @@ What the system does **not** do today (as-built; see DESIGN §10 for the roadmap - **TLS / HTTPS-pinned or `https://`-hardcoded SUT clients** — cannot intercept HTTPS → integration untested → false green. - **Cross-transport sagas** (Kafka trigger → HTTP callback) — no causal linkage → false green. - **Non-JSON Kafka values** (Avro/Protobuf/schema-registry) — emitted as `kafka.skipped` → false green. -- **`match` is envelope-rooted under dialect `"2"`** (was: payload-rooted under `"1"`). The five `match` eval sites (HTTP stub `match.jq`, Kafka reactor `match`, `kafka.patterns[].match`, `kafka assert/consume --match`, `http call`/`request --match`) feed the whole envelope; `capture.*.from` shares the same root. `match.body` / `--contains` / `--path` / `--jq-path`+`--equals` / `--status` remain payload-rooted. A v1 config is rejected by `_check_version`; rewrite with `agctl config migrate`. +- **`match` is envelope-rooted under dialect `"2"`+ (was: payload-rooted under `"1"`; unchanged by the v3 named-cluster schema lift). The five `match` eval sites (HTTP stub `match.jq`, Kafka reactor `match`, `kafka.patterns[].match`, `kafka assert/consume --match`, `http call`/`request --match`) feed the whole envelope; `capture.*.from` shares the same root. `match.body` / `--contains` / `--path` / `--jq-path`+`--equals` / `--status` remain payload-rooted. A v1/v2 config is rejected by `_check_version`; rewrite with `agctl config migrate`. - **Containerized SUT topology** — operator must target `host.docker.internal` / host LAN IP and avoid SUT that swallows connection errors → false green. - **Shared broker + pinned `consumer_group` reused across runs/devs** — partition split or resume-past-messages → silently missing/old reactions → false green (mitigated by unique-per-run default). diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 24bf347..5d93496 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -51,10 +51,13 @@ ```yaml # agctl.yaml # --------------- -# Version is the jq-dialect switch: "2" = envelope-rooted `match` (`.body.x` -# for HTTP, `.value.x` for Kafka). A major-version mismatch is a ConfigError -# (exit 2) pointing at `agctl config migrate`; minor/patch are not tracked. -version: "2" +# Version is the config schema version. "3" = named `kafka.clusters` (the flat +# `kafka:` block was lifted into `kafka.clusters.` + `default_cluster`). +# The `match` envelope-rooting introduced under "2" (`.body.x` for HTTP, +# `.value.x` for Kafka) is unchanged in v3. A major-version mismatch is a +# ConfigError (exit 2) pointing at `agctl config migrate`; minor/patch are not +# tracked. +version: "3" # --------------------------------------------------------------------------- # services — named HTTP base URLs for services under test. @@ -72,30 +75,46 @@ services: timeout_seconds: 15 # --------------------------------------------------------------------------- -# kafka — broker configuration. +# kafka — named clusters + patterns (v3). Mirrors database.connections: +# clusters.: a named broker profile (brokers, TLS, timeout, group, schema registry) +# default_cluster: cluster used when no --cluster flag / pattern.cluster selects one +# (required only when >1 cluster is defined; a single cluster auto-defaults) +# Each cluster's fields support ${ENV_VAR} interpolation (see §2.2). # --------------------------------------------------------------------------- kafka: - brokers: - - "${KAFKA_BROKER_HOST}:9092" - default_consumer_group: "agctl-consumer" # optional - schema_registry_url: "${SCHEMA_REGISTRY_URL}" # optional; omit if not used - timeout_seconds: 30 # default consume/assert timeout - - # kafka.ssl — optional TLS/mTLS settings for brokers that require SSL. - # Setting any field (to a non-empty value) enables TLS; security.protocol - # defaults to "SSL" unless overridden. Hostname verification stays on - # unless endpoint_identification_algorithm is set to "none" (self-signed/dev). - # ca_location is optional: when unset, librdkafka falls back to the system - # trust store (use it for publicly-trusted brokers like Confluent Cloud; pin - # a CA for private-PKI brokers). All values support ${ENV_VAR} interpolation - # and AGCTL_KAFKA__SSL__* overrides; an empty string counts as unset. - ssl: - ca_location: "${KAFKA_SSL_CA:-}" # path to CA certificate (PEM); optional - certificate_location: "${KAFKA_SSL_CERT:-}" # path to client certificate (mTLS) - key_location: "${KAFKA_SSL_KEY:-}" # path to client private key (mTLS) - key_password: "${KAFKA_SSL_KEY_PASSWORD:-}" # optional private-key password - # endpoint_identification_algorithm: "none" # uncomment to disable hostname verification - # security_protocol: "SSL" # defaults to SSL; set SASL_SSL when adding SASL later + clusters: + default: + brokers: + - "${KAFKA_BROKER_HOST}:9092" + default_consumer_group: "agctl-consumer" # optional + schema_registry_url: "${SCHEMA_REGISTRY_URL}" # optional; omit if not used + timeout_seconds: 30 # default consume/assert timeout + + # kafka.clusters..ssl — optional TLS/mTLS settings, per cluster. + # Setting any field (to a non-empty value) enables TLS; security.protocol + # defaults to "SSL" unless overridden. Hostname verification stays on + # unless endpoint_identification_algorithm is set to "none" (self-signed/dev). + # ca_location is optional: when unset, librdkafka falls back to the system + # trust store (use it for publicly-trusted brokers like Confluent Cloud; pin + # a CA for private-PKI brokers). All values support ${ENV_VAR} interpolation + # and AGCTL_KAFKA__CLUSTERS____SSL__* overrides; an empty string counts as unset. + ssl: + ca_location: "${KAFKA_SSL_CA:-}" # path to CA certificate (PEM); optional + certificate_location: "${KAFKA_SSL_CERT:-}" # path to client certificate (mTLS) + key_location: "${KAFKA_SSL_KEY:-}" # path to client private key (mTLS) + key_password: "${KAFKA_SSL_KEY_PASSWORD:-}" # optional private-key password + # endpoint_identification_algorithm: "none" # uncomment to disable hostname verification + # security_protocol: "SSL" # defaults to SSL; set SASL_SSL when adding SASL later + + # A second named cluster (optional). Patterns and reactors bind a cluster by name; + # the CLI `--cluster` flag overrides any binding. Omit `default_cluster` only when + # exactly one cluster is defined (it auto-defaults); with >1 cluster, `default_cluster` + # is required. + # analytics: + # brokers: + # - "${ANALYTICS_KAFKA_BROKER_HOST}:9092" + + default_cluster: default # --------------------------------------------------------------------------- # kafka.patterns — named Kafka filter patterns, analogous to HTTP templates. @@ -104,6 +123,8 @@ kafka: # ({key, value, partition, offset, timestamp, headers}); so .value.eventType, # .value.payload.orderId, .key, .headers.. Supports {placeholder} # substitution via --param at assert time. +# cluster: optional named cluster this pattern binds to (default: default_cluster, +# or the single defined cluster). The CLI `--cluster` flag overrides it. # --------------------------------------------------------------------------- patterns: order-created: @@ -172,6 +193,8 @@ mocks: description: "Mock the service that consumes order commands" topic: orders.commands consumer_group: agctl-mock-order-handler # OPTIONAL; omit → unique per-run group + # cluster: analytics # OPTIONAL named cluster this reactor binds to + # (default: kafka.default_cluster / single-cluster auto-default) match: '.value.command == "CREATE_ORDER"' # envelope-rooted (whole Kafka message) # capture: same CaptureSpec shape as HTTP stubs; `from` shares the `match` # root — the Kafka message envelope ({key, value, partition, offset, @@ -547,7 +570,7 @@ Consume messages from a topic. Returns as soon as `--expect-count` matching mess ``` agctl kafka consume --topic - [--timeout ] # default: kafka.timeout_seconds from config + [--timeout ] # default: resolved cluster's timeout_seconds from config [--lookback ] # seek to now - lookback before reading; default: = --timeout [--match ] # jq boolean predicate over the message envelope # ({key, value, partition, offset, timestamp, headers}); only @@ -557,6 +580,7 @@ agctl kafka consume # messages are received within the window [--from-beginning] # seek to earliest offset (default: seek to now - lookback) [--consumer-group ] # override default consumer group (default: agctl-consumer) + [--cluster ] # named kafka cluster (default: kafka.default_cluster / single-cluster) ``` `--match` is a jq boolean expression evaluated against each message **envelope** (`{key, value, partition, offset, timestamp, headers}`) — so `.value.eventType`, `.key`, `.headers.rqUID`. Header keys are case-sensitive (as-produced). A **malformed expression** (syntax error) raises `ConfigError` (exit 2) before any polling; messages where the expression evaluates to `false` or raises a **runtime error** against that specific message are silently skipped. This enables partial matching — you do not need to know the full message structure. @@ -594,6 +618,7 @@ agctl kafka produce --message '{...}' # JSON string; value is published as-is [--key ] # optional Kafka message key [--header key=value] # repeatable; Kafka message headers + [--cluster ] # named kafka cluster (default: kafka.default_cluster / single-cluster) ``` **Example:** @@ -628,6 +653,8 @@ agctl kafka assert --timeout # assert fails (AssertionError) if no match within this window [--from-beginning] # seek to earliest offset (default: seek to now - lookback) [--consumer-group ] # override default consumer group (default: agctl-consumer) + [--cluster ] # named kafka cluster; overrides the pattern's bound cluster + # (default: pattern.cluster > kafka.default_cluster / single-cluster) ``` **Examples:** @@ -659,7 +686,9 @@ agctl kafka assert \ **Offset & timing model (consume and assert):** By default the consumer seeks each partition to the timestamp `now - --lookback` (via `offsets_for_times`) and reads forward, rather than subscribing at "latest". This makes the common send-then-assert pattern reliable: an event published a moment before the command starts still falls inside the window. `--lookback` defaults to the resolved `--timeout` (look back as far as you wait forward); `--from-beginning` overrides to the earliest offset. For `assert`, committed offsets are ignored — each invocation re-seeks by time, so repeated asserts are independent and deterministic. On high-volume topics, narrow with `--match`/`--contains` to avoid matching stale events from prior runs. -**TLS / transport model (produce, consume, assert):** All three commands connect to brokers using the `kafka.ssl` block (see §2.1). Setting any field to a non-empty value enables TLS and defaults `security.protocol` to `SSL` (mTLS); `ca_location` is optional and falls back to the system trust store when unset. Hostname verification is **on** by default (librdkafka default) — set `endpoint_identification_algorithm: "none"` only for self-signed or dev brokers. An empty string (e.g. an unresolved `${VAR:-}`) is treated as unset, so a partially-configured `ssl:` block never silently downgrades to plaintext nor disables verification. TLS configuration is unit-tested only; the live integration suite runs a plaintext broker. +**TLS / transport model (produce, consume, assert):** All three commands connect to brokers using the resolved cluster's `ssl` block (see §2.1). Setting any field to a non-empty value enables TLS and defaults `security.protocol` to `SSL` (mTLS); `ca_location` is optional and falls back to the system trust store when unset. Hostname verification is **on** by default (librdkafka default) — set `endpoint_identification_algorithm: "none"` only for self-signed or dev brokers. An empty string (e.g. an unresolved `${VAR:-}`) is treated as unset, so a partially-configured `ssl:` block never silently downgrades to plaintext nor disables verification. TLS configuration is unit-tested only; the live integration suite runs a plaintext broker. + +**Cluster resolution (produce, consume, assert):** All three commands target a single named cluster per invocation. Resolution precedence: `--cluster` (explicit) > a pattern's bound `cluster` (`assert --pattern` only) > `kafka.default_cluster` > the single defined cluster when exactly one is configured. An unresolvable name (`>1 cluster` and no `--cluster`/`default_cluster`), or a name absent from `kafka.clusters`, raises `ConfigError` (exit 2). This mirrors DB connection resolution (`--connection` > template's `connection` > `defaults.database_connection`). --- @@ -1093,7 +1122,7 @@ agctl mock run **Lifecycle:** - `--http-listen` is a **literal** `host:port` string (CLI args are not `${}`-interpolated, only YAML values). -- `--only` restricts to one engine; `kafka`-extra / `kafka.brokers` checks are gated on engines actually started. +- `--only` restricts to one engine; `kafka`-extra / per-reactor cluster `brokers` checks are gated on engines actually started. - **No `mocks` section** → emits `started` + `summary` with zero counts and exits `0` (idempotent no-op). - **`--only ` with that engine absent** → `ConfigError` (exit 2). - **Startup hazard:** when backgrounding, wrap with `nohup`/`setsid` so the mock is not killed by `SIGHUP` when the launching shell exits, and capture the PID. @@ -1357,7 +1386,7 @@ agctl config init #### `agctl config migrate` -Rewrite a dialect-`"1"` config to dialect `"2"` (envelope-rooted `match`). Backs up the original to `.bak` and writes the rewritten config back to ``. A config already at `"2"` is a clean no-op (`already_v2: true`, `rewrites: []`). Refuses to clobber an existing `.bak` (`ConfigError`, exit 2 — remove or rename it first); the backup is the only safety net for the reformat the rewrite performs. +Rewrite a v1/v2 config to dialect `"3"` (named `kafka.clusters`). v3 restructured `Config.kafka` from a single flat object into a named map `kafka.clusters.` (mirroring `database.connections`) plus `default_cluster`, and bumped the version. v1 configs additionally need the v2 jq-dialect rewrite (every `match` expression envelope-rooted: HTTP `.body`, Kafka `.value`); a v1 input is carried through BOTH the jq rewrite and the structural lift in one pass, a v2 input through the lift only. Backs up the original to `.bak` and writes the rewritten config back to ``. A config already at `"3"` is a clean no-op (`already_current: true`, `rewrites: []`). Refuses to clobber an existing `.bak` (`ConfigError`, exit 2 — remove or rename it first); the backup is the only safety net for the reformat the rewrite performs. ``` agctl config migrate @@ -1365,15 +1394,17 @@ agctl config migrate [--dry-run] # preview the rewrite; do not write ``` -The rewrite walks the three `match`-site families and prepends the envelope prefix: +The structural lift runs for any non-current source (v1 and v2 both may carry a flat `kafka:` block): when `kafka:` holds any of the five flat keys (`brokers`/`ssl`/`timeout_seconds`/`default_consumer_group`/`schema_registry_url`) and no `clusters` key, those keys move into `kafka.clusters.default` and `default_cluster: default` is set. A missing or already-clustered `kafka` contributes no rewrites and never raises. + +Additionally, for **v1 sources only** (v2 exprs are already envelope-rooted and would be double-prefixed), the rewrite walks the three `match`-site families and prepends the envelope prefix: - `mocks.http.stubs..match.jq` → prefix `.body | ` (e.g. `.amount > 1000` → `.body | .amount > 1000`). - `mocks.kafka.reactors..match` → prefix `.value | `. - `kafka.patterns..match` → prefix `.value | `. -Then bumps `version` to `"2"`. `capture.*.from` and `match.body` are **not** visited (out of scope). Idempotent — expressions that already start with the prefix are not double-prefixed; an already-`"2"` config is returned unchanged. +Then bumps `version` to `"3"`. `capture.*.from` and `match.body` are **not** visited (out of scope). Idempotent — jq-prefix expressions that already start with the prefix are not double-prefixed; the structural lift never double-lifts an already-clustered `kafka`; an already-`"3"` config is returned unchanged. -**`cli_flags_note` (load-bearing caveat):** CLI `--match` flags (and the deprecated `--filter-key` alias) passed to `agctl http` / `agctl kafka` in shell scripts, agent prompts, or runbooks are **not** rewritten by this command (it walks the config file only). Prefix them manually: `.body | ` for HTTP, `.value | ` for Kafka. (`agctl mock run` has no `--match` CLI flag — mock matchers are config-file only, and ARE rewritten.) +**`cli_flags_note` (load-bearing caveat):** CLI `--match` flags (and the deprecated `--filter-key` alias) passed to `agctl http` / `agctl kafka` in shell scripts, agent prompts, or runbooks are **not** rewritten by this command (it walks the config file only). The `.body | ` / `.value | ` prefix guidance applies only to **v1** inputs being lifted to v3 — v2/v3 exprs are already envelope-rooted. (`agctl mock run` has no `--match` CLI flag — mock matchers are config-file only, and ARE rewritten on v1 sources.) **`formatting_note`:** the rewritten file is emitted via `yaml.safe_dump`, which normalizes indentation/quotes and drops comments; the original is preserved verbatim in `.bak`. Review the full diff before committing. @@ -1385,13 +1416,14 @@ Then bumps `version` to `"2"`. `capture.*.from` and `match.body` are **not** vis "command": "config.migrate", "result": { "path": "./agctl.yaml", - "already_v2": false, - "from_version": "1", - "to_version": "2", + "already_current": false, + "from_version": "2", + "to_version": "3", "rewritten": [ - {"path": "mocks.http.stubs.create-order.match.jq", "before": ".amount > 1000", "after": ".body | .amount > 1000"} + {"path": "kafka.clusters.default.brokers", "before": ["host:9092"], "after": ["host:9092"]}, + {"path": "kafka.default_cluster", "before": null, "after": "default"} ], - "cli_flags_note": "CLI --match flags (and the deprecated --filter-key alias) on `agctl http` / `agctl kafka` … must be prefixed manually: `.body | ` for HTTP, `.value | ` for Kafka.", + "cli_flags_note": "CLI --match flags (and the deprecated --filter-key alias) on `agctl http` / `agctl kafka` … the `.body | ` / `.value | ` prefix applies only to v1 inputs; v2/v3 exprs are already envelope-rooted.", "formatting_note": "yaml.safe_dump reformats the file and drops comments; the original is preserved in .bak …" }, "duration_ms": 3 @@ -1520,6 +1552,7 @@ agctl discover --category --name [--overlay ] "name": "order-created", "description": "An ORDER_CREATED event for a specific order", "topic": "orders.created", + "cluster": "default", "params": ["orderId"], "example": "agctl kafka assert --pattern order-created --param orderId=X --timeout 10" }, @@ -1614,7 +1647,7 @@ Every invocation writes exactly one JSON object to stdout (the sole exception is | Type | Exit code | Applies when | |---|---|---| | `AssertionError` | 1 | An assertion was evaluated and failed — including `kafka assert` timing out (no matching message within the window), `kafka consume --expect-count` receiving fewer than expected, and `http call`/`http request` response assertions (`--status`/`--contains`/`--match`/`--jq-path`/`--equals`) evaluating false | -| `ConfigError` | 2 | Config missing/invalid, an unresolvable **required** env var, or a major-version (jq-dialect) mismatch — a v1 config under the v2 tool is rejected with a pointer to `agctl config migrate` | +| `ConfigError` | 2 | Config missing/invalid, an unresolvable **required** env var, or a major-version (config-schema) mismatch — a v1/v2 config under the v3 tool is rejected with a pointer to `agctl config migrate` | | `ConnectionError` | 2 | Could not reach a service, broker, or database | | `TimeoutError` | 1 | A non-assertion operation exceeded its time budget (e.g. a slow HTTP request or a hung DB query) | | `TemplateNotFound` | 2 | Named template/pattern/connection does not exist in config | @@ -1904,7 +1937,7 @@ Every service entry always includes `response_time_ms` (an integer on success, ` #### `discover.item` -Shape varies by category. All items share `name`, `description`, `params[]`, and `example`. HTTP templates add `method`, `service`, `path`; Kafka patterns add `topic` and `match`; DB templates add `connection` and `sql` (so an agent can read a query's result columns before writing a `--path` value assertion). +Shape varies by category. All items share `name`, `description`, `params[]`, and `example`. HTTP templates add `method`, `service`, `path`; Kafka patterns add `topic`, `cluster` (the resolved cluster name), and `match`; DB templates add `connection` and `sql` (so an agent can read a query's result columns before writing a `--path` value assertion). #### `discover.search` @@ -2027,7 +2060,7 @@ Refused to overwrite (without `--force`): ``` AGCTL_DEFAULTS__TIMEOUT_SECONDS=30 -AGCTL_KAFKA__DEFAULT_CONSUMER_GROUP=my-group +AGCTL_KAFKA__CLUSTERS__DEFAULT__DEFAULT_CONSUMER_GROUP=my-group AGCTL_DATABASE__CONNECTIONS__MAIN_DB__PASSWORD=s3cr3t AGCTL_SERVICES__ORDER_SERVICE__BASE_URL=http://order-svc:8080 ``` @@ -2404,7 +2437,7 @@ Full examples: ```bash AGCTL_DEFAULTS__TIMEOUT_SECONDS=30 -AGCTL_KAFKA__DEFAULT_CONSUMER_GROUP=ci-consumer +AGCTL_KAFKA__CLUSTERS__DEFAULT__DEFAULT_CONSUMER_GROUP=ci-consumer AGCTL_DATABASE__CONNECTIONS__MAIN_DB__HOST=localhost AGCTL_DATABASE__CONNECTIONS__MAIN_DB__PASSWORD=supersecret AGCTL_SERVICES__ORDER_SERVICE__BASE_URL=http://order-svc:8080 diff --git a/skills/agctl-config/SKILL.md b/skills/agctl-config/SKILL.md index 60a1b9f..788c97f 100644 --- a/skills/agctl-config/SKILL.md +++ b/skills/agctl-config/SKILL.md @@ -126,13 +126,14 @@ live validation was skipped. **Never** declare done on config that doesn't valid ### Structural checklist (fallback when agctl is absent) - [ ] YAML parses. -- [ ] `version` is present, major part `"2"` (the jq-dialect switch — `"2"` = envelope-rooted `match`). +- [ ] `version` is present, major part `"3"` (the config schema version — `"3"` = named `kafka.clusters`; the `match` envelope-rooting from `"2"` is unchanged). - [ ] Every `templates.*.service` ∈ `services`. - [ ] Every `database.templates.*.connection` (if set) ∈ `database.connections`. - [ ] `defaults.database_connection` (if set) ∈ `database.connections`. -- [ ] `kafka.ssl.security_protocol` (if set) ∈ {PLAINTEXT, SSL, SASL_SSL, SASL_PLAINTEXT}. +- [ ] Every `kafka.clusters..ssl.security_protocol` (if set) ∈ {PLAINTEXT, SSL, SASL_SSL, SASL_PLAINTEXT}. +- [ ] `kafka.default_cluster` (if set, or when >1 cluster is defined) ∈ `kafka.clusters`; each `kafka.patterns..cluster` (if set) ∈ `kafka.clusters`. - [ ] Every `templates` / `database.templates` / `kafka.patterns` entry has a non-empty `description`. -- [ ] If `mocks.kafka.reactors` is set, `kafka.brokers` is non-empty (required at `mock run` startup). +- [ ] If `mocks.kafka.reactors` is set, each reactor's resolved cluster (`reactor.cluster` → `default_cluster` → single-cluster auto-default) has non-empty `kafka.clusters..brokers` (required at `mock run` startup). - [ ] `mocks.http.listen` (if set) parses as `host:port` (IPv6 hosts bracketed, e.g. `[::1]:18080`). - [ ] Every `mocks.http.stubs` / `mocks.kafka.reactors` entry has a non-empty `description`. - [ ] Every `mocks.kafka.reactors.*.reaction.headers` value (if set) is a string. diff --git a/skills/agctl-config/reference/init-config.md b/skills/agctl-config/reference/init-config.md index 750691d..f79894f 100644 --- a/skills/agctl-config/reference/init-config.md +++ b/skills/agctl-config/reference/init-config.md @@ -24,8 +24,11 @@ scan plan. 2. **HTTP templates** → `templates:` — run the `http` extraction (see `http-template.md`) over controllers / OpenAPI. In a monorepo, emit one `services:` key per distinct `base_url`; controllers sharing a port / base URL fold into one service. -3. **Kafka** → `kafka:` — brokers from compose / props → `kafka.brokers`; producers / topics → - `kafka.patterns:` (see `kafka-pattern.md`). +3. **Kafka** → `kafka:` — brokers from compose / props → `kafka.clusters.` (a named + map mirroring `database.connections`); set `default_cluster` to one of the cluster names + (or omit it when only one cluster is defined — it auto-defaults). Producers / topics → + `kafka.patterns:` (see `kafka-pattern.md`); a pattern may bind a `cluster` if its event + lives on a non-default cluster. 4. **Database** → `database:` — datasource config (Spring `spring.datasource`, `DATABASE_URL`, a compose `postgres` service) → `database.connections:` (mark one `default: true`); queries / repos → `database.templates:` (see `db-template.md`). @@ -58,11 +61,11 @@ Never put real secret values in it. - Which DB connection is the default. - `health_path` when it's not Spring / Actuator. - Env-var names for secrets if the repo doesn't already define them. -- Whether to include the optional `kafka.ssl` block (only if brokers need TLS). +- Whether to include the optional per-cluster `kafka.clusters..ssl` block (only if brokers need TLS). ## Close-out -- Put `version: "2"` at the top. +- Put `version: "3"` at the top. - Write `.env.example` (above), then have the user copy & fill it: `cp .env.example .env` (edit the secrets). - Source it before validating, so required `${VAR}`s resolve: `set -a; . ./.env; set +a`. diff --git a/skills/agctl-config/reference/kafka-pattern.md b/skills/agctl-config/reference/kafka-pattern.md index 68d5b0b..fd1985b 100644 --- a/skills/agctl-config/reference/kafka-pattern.md +++ b/skills/agctl-config/reference/kafka-pattern.md @@ -18,8 +18,13 @@ this file is the extraction detail. `.value.` (e.g. `.value.eventType`, `.value.payload.orderId == "{orderId}"`). Reach the message key / a header with `.key` / `.headers.` (header keys are **case-sensitive** — use the producer's exact name). Use `{placeholder}` for the value that varies per assert. -3. **description** — one line. -4. **name** — kebab-case from the event (`ORDER_CREATED` → `order-created`; +3. **cluster** *(optional)* — the named cluster this pattern binds to (a key under + `kafka.clusters`). Omit to fall back to `kafka.default_cluster`, or to the single + defined cluster when exactly one exists. Set it only when the event lives on a different + cluster than the default; a dangling name is a `config validate` error. The CLI + `kafka assert --cluster ` overrides whatever the pattern binds. +4. **description** — one line. +5. **name** — kebab-case from the event (`ORDER_CREATED` → `order-created`; `PAYMENT_FAILED` → `payment-failed`). A pattern answers "what does the event I care about look like?" — narrow enough to not match @@ -55,13 +60,16 @@ stale events from prior runs on busy topics. - Prefer a sharp `--match` predicate over `--contains` (subset) for large/variable payloads — patterns live in config precisely so you can write one. -## Migrating from dialect `"1"` +## Migrating from dialect `"1"` / `"2"` -If the repo's `agctl.yaml` is still at `version: "1"`, `agctl` rejects it (exit 2) with a -pointer to `agctl config migrate`. Run that to rewrite every `kafka.patterns..match` -(and reactor `match`) by prepending `.value | ` and bumping `version` to `"2"` — backups and -`--dry-run` are supported; CLI `--match` flags in scripts/prompts are **not** rewritten -(prefix those by hand). +If the repo's `agctl.yaml` is still at `version: "1"` or `"2"`, `agctl` rejects it (exit 2) +with a pointer to `agctl config migrate`. Run that to lift a flat `kafka:` block into +`kafka.clusters.default` + `default_cluster: default` and bump `version` to `"3"`; a v1 +source additionally gets every `kafka.patterns..match` (and reactor `match`) rewritten +by prepending `.value | ` (v2 exprs are already envelope-rooted and are left alone). +Backups and `--dry-run` are supported; the result reports `already_current: true` for an +already-v3 config. CLI `--match` flags in scripts/prompts are **not** rewritten — prefix +those by hand, and only for v1 inputs (v2/v3 exprs are already envelope-rooted). ## What to clarify @@ -81,5 +89,5 @@ Under `kafka.patterns:` (nested under `kafka:`). - The pattern's `topic` is what `kafka assert --pattern` uses (then omit `--topic`). - `kafka assert` reads a **window** (default lookback = `--timeout`); narrow with `match` so you don't match a stale event from a previous run. -- If you also touch `kafka.ssl`, `security_protocol` (if set) must be one of - PLAINTEXT / SSL / SASL_SSL / SASL_PLAINTEXT. +- If you also touch a cluster's `ssl` block (`kafka.clusters..ssl`), + `security_protocol` (if set) must be one of PLAINTEXT / SSL / SASL_SSL / SASL_PLAINTEXT. diff --git a/skills/agctl-config/reference/mocks.md b/skills/agctl-config/reference/mocks.md index 6ceb048..98229be 100644 --- a/skills/agctl-config/reference/mocks.md +++ b/skills/agctl-config/reference/mocks.md @@ -90,16 +90,22 @@ jq-shadowing warning (see Gotchas). (`{key, value, partition, offset, timestamp, headers}`), so `.value.command == "CREATE_ORDER"`, `.key`, `.headers.rqUID` (header keys are **case-sensitive**). Omit = match all. Non-JSON / non-object values never match — they're visibly skipped, not silently dropped. -4. **reaction** — what to **produce** back: `topic` (the **event** topic), `key` (optional), +4. **cluster** *(optional)* — the named cluster this reactor binds to (a key under + `kafka.clusters`). Omit to fall back to `kafka.default_cluster` or the single defined + cluster. Set it when the reactor consumes from a non-default cluster; a dangling name is + a `config validate` error. Each reactor gets its own `KafkaClient` built from the + resolved cluster (reactors sharing a cluster reuse one client). +5. **reaction** — what to **produce** back: `topic` (the **event** topic), `key` (optional), `value` (JSON-serializable), `headers` (optional; **string values only** — a non-string value is a config error). Render `{name}` against the message-value context. -5. **name** — kebab-case from the consumer/event. -6. **description** — one line. +6. **name** — kebab-case from the consumer/event. +7. **description** — one line. -**Requires top-level `kafka.brokers`.** `mocks.kafka` joins the SUT's *real* broker — it is -not a broker itself. `mock run` enforces this at startup (exit 2 if `kafka.brokers` is -absent), and needs the `kafka` extra installed (`pip install 'agctl[kafka]'`). HTTP-only -mocks need neither. +**Requires a resolved cluster with brokers.** `mocks.kafka` joins the SUT's *real* broker — it is +not a broker itself. Each reactor resolves a cluster (`reactor.cluster` → `kafka.default_cluster` +→ single-cluster auto-default); the resolved cluster must have non-empty `brokers`, or +`mock run` fails fast at startup (exit 2). `mock run` also needs the `kafka` extra installed +(`pip install 'agctl[kafka]'`). HTTP-only mocks need neither brokers nor the extra. ## Capture value coercion (load-bearing) @@ -138,9 +144,10 @@ capture: ``` - **`from`** — a jq path evaluated against the **envelope** (not the payload). Under - dialect `"2"` `match` shares this root (envelope-rooted); `capture.from` and `match` + dialect `"2"`+ `match` shares this root (envelope-rooted); `capture.from` and `match` reach the same fields. (Under dialect `"1"` `match` was payload-rooted — a `.` - divergence unified by #22 and the v2 dialect switch.) + divergence unified by #22 and the v2 dialect switch; the v3 schema lift left this + rooting unchanged.) - **`type`** — `scalar` (default) / `object` / `json`. See "Capture value coercion" above for what each renders. `object` is the only way to produce a real JSON object/array field; it requires the placeholder to occupy the **whole field** (`ctx: "{ctx}"`, not @@ -290,8 +297,8 @@ them (full list + failure modes in DESIGN §10 "Known-wrong-result / Not Covered - `{name}` = capture-from-trigger here — **never** `${}` in a path/reaction template, and there is no `--param` for mocks. `${ENV}` is fine in `listen`/`consumer_group`/topics. -- `mocks.kafka` requires top-level `kafka.brokers` (and the `kafka` extra); HTTP-only mocks - don't. +- `mocks.kafka` requires each reactor's resolved cluster to have non-empty + `kafka.clusters..brokers` (and the `kafka` extra); HTTP-only mocks don't. - `reaction.headers` values must be strings — a non-string is a config error. - `description` is optional but effectively required (contract #4); its absence degrades `discover` and earns a validate warning. @@ -303,11 +310,12 @@ them (full list + failure modes in DESIGN §10 "Known-wrong-result / Not Covered (exit 2); a jq **eval error** (or a `from` resolving to `null`) against a particular request/message is a soft non-match / `capture.missing` (falls through, empty string). Two different guards for two different error classes — see "jq match semantics" above. -- Under dialect `"2"`, **`match` and `capture.from` share an envelope root** — `.body.amount` +- Under dialect `"2"`+, **`match` and `capture.from` share an envelope root** — `.body.amount` (HTTP) / `.value.command` (Kafka) on both sides. Under dialect `"1"` `match` was - payload-rooted; `agctl config migrate` rewrites a v1 config (`.body | ` / `.value | ` - prefix on the three match-site families) but does **not** touch CLI `--match` flags in - shell scripts / prompts. + payload-rooted; `agctl config migrate` lifts a v1/v2 config to v3 (structural + `kafka.clusters` lift for both; `.body | ` / `.value | ` prefix on the three match-site + families for v1 only) but does **not** touch CLI `--match` flags in shell scripts / + prompts. - HTTP `headers` in the capture envelope are **lowercased** (`.headers.authorization`); Kafka `headers` are **case-sensitive as-produced** (`.headers.rqUID`, exact producer casing). Don't lowercase Kafka header names. diff --git a/skills/agctl/SKILL.md b/skills/agctl/SKILL.md index 888f244..31ade3b 100644 --- a/skills/agctl/SKILL.md +++ b/skills/agctl/SKILL.md @@ -24,10 +24,10 @@ required args, even `--lookback`/`--consumer-group`/`--assertion`); this skill keeps only what `--help` won't tell you — semantics, roots, traps. **Pin the version you target.** `--match`/`--jq-path` roots and failure shapes -moved in agctl ≥1.0 (dialect v2). A stale global install (e.g. `0.1.0`, where -`--match` was body-rooted) silently contradicts this skill. After upgrading, -reinstall in every env that runs it (`pip install -U 'agctl[jq]'`) so `--help` -and behavior match. +moved in agctl ≥1.0 (dialect v2), and the config schema moved to v3 (named +`kafka.clusters`). A stale global install (e.g. `0.1.0`, where `--match` was +body-rooted) silently contradicts this skill. After upgrading, reinstall in every +env that runs it (`pip install -U 'agctl[jq]'`) so `--help` and behavior match. ## Orient first: `agctl discover` @@ -48,10 +48,14 @@ flags: see `--help`.) | Publish a message | `kafka produce --topic T --message '{…}'` | | DB write / rows / value / inspect / schema | `db execute --write` / `db assert --expect-rows N` / `db assert --expect-value --path .x --equals v` / `db query` / `db schema` | | Impersonate a dependency | `mock run` (foreground) / `mock start\|stop\|status` (daemon) | -| Are services up? / validate config / migrate v1→v2 | `check ready --all` / `config validate` / `config migrate` | +| Are services up? / validate config / migrate v1/v2→v3 | `check ready --all` / `config validate` / `config migrate` | `--config ` and `--overlay ` (repeatable) are global; **`--timeout` is not global**. `` for kafka = `--contains '{…}' | --match '' | --pattern `. +`kafka produce|consume|assert` take `--cluster ` (default: the pattern's bound +cluster for `assert --pattern`, else `kafka.default_cluster`, else the single defined +cluster; `--cluster` always wins) — set it only when a command must target a non-default +cluster. ## Flag semantics (`--help` shows flags; these are how they behave) @@ -75,8 +79,9 @@ not global**. `` for kafka = `--contains '{…}' | --match '' | --patt *result*, not an error. Add `--status`/`--contains`/`--match`/`--jq-path`/ `--equals` to `http call`/`request` to flip a wrong response into `AssertionError` (exit 1); zero assertion flags leaves the result path unchanged. -3. **`--match` is envelope-rooted under dialect `"2"` (not payload-rooted).** `--help` - names the envelope; the trap is the migration: +3. **`--match` is envelope-rooted (dialect `"2"`+; not payload-rooted).** `--help` + names the envelope; the trap is the migration. (v3 only changed the kafka config + shape to named clusters — the `match` rooting is unchanged.) - **HTTP** `--match` → response envelope `{status_code, response_time_ms, headers (lowercased), body, url, method}` ⇒ `.body.order_id`, `.status_code`, `.headers.x`. Prefix legacy body-form exprs with `.body | ` (`.status == "X"` → `.body | .status == "X"`). @@ -86,9 +91,10 @@ not global**. `` for kafka = `--contains '{…}' | --match '' | --patt value-form with `.value | `. - **Unchanged:** `match.body` (json_subset), `--contains`, `--path`, `--jq-path`/`--equals` (still body-rooted), `--status`. - - A v1 `agctl.yaml` is rejected (exit 2) → `config migrate` rewrites the three - `match`-site families in-file. **CLI `--match` flags in scripts/prompts are - NOT rewritten** — prefix them by hand. + - A v1 or v2 `agctl.yaml` is rejected (exit 2) → `config migrate` lifts it to v3 + (structural `kafka.clusters` lift for v1/v2; the three `match`-site families are + `.body | ` / `.value | `-prefixed for **v1 only**). **CLI `--match` flags in + scripts/prompts are NOT rewritten** — prefix them by hand, and only for v1 inputs. 4. **Three placeholder syntaxes — don't mix:** `${VAR}` env, resolved at config load (required → exit 2 if unset; `${VAR:-default}` optional; `${VAR:-}` optional/empty); `{name}` HTTP path/body & Kafka patterns, filled at call time From 55e606c90b697232bf0310b91ff804250a3baed3 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 02:08:32 +0300 Subject: [PATCH 09/12] fix(config): gate migrate cli-flags note on v1; tidy v3 test fixtures Co-Authored-By: Claude --- agctl/commands/config_commands.py | 46 +++++++++++++++++++------- agctl/config/loader.py | 3 +- tests/unit/test_kafka_commands.py | 3 ++ tests/unit/test_migrate.py | 54 +++++++++++++++++++++++++++++++ tests/unit/test_mock_commands.py | 11 +++++-- tests/unit/test_overlay.py | 7 ++-- 6 files changed, 106 insertions(+), 18 deletions(-) diff --git a/agctl/commands/config_commands.py b/agctl/commands/config_commands.py index d00143c..7c3398c 100644 --- a/agctl/commands/config_commands.py +++ b/agctl/commands/config_commands.py @@ -287,18 +287,32 @@ def config_init(ctx: click.Context, output: str | None, force: bool) -> None: # --- config migrate -------------------------------------------------------- -#: Fixed operator-facing reminder that CLI ``--match`` flags are NOT rewritten -#: by ``agctl config migrate`` (the migration only walks the config file). Note -#: ``agctl mock run`` has NO ``--match`` CLI flag (mock matchers are config-file -#: only — and ARE rewritten); the deprecated ``--filter-key`` alias on -#: ``agctl kafka consume`` is, like ``--match``, envelope-rooted under v2 and -#: equally out of this command's reach. -_CLI_FLAGS_NOTE = ( +#: Base operator-facing reminder, always emitted: CLI ``--match`` flags are +#: NOT rewritten by ``agctl config migrate`` (the migration only walks the +#: config file). Note ``agctl mock run`` has NO ``--match`` CLI flag (mock +#: matchers are config-file only — and ARE rewritten); the deprecated +#: ``--filter-key`` alias on ``agctl kafka consume`` is, like ``--match``, +#: envelope-rooted under v2 and equally out of this command's reach. This base +#: text contains NO prefix instruction — that is appended (see below) only for +#: v1 sources, whose CLI flags genuinely still need a manual envelope prefix. +_CLI_FLAGS_NOTE_BASE = ( "CLI --match flags (and the deprecated --filter-key alias) on " "`agctl http` / `agctl kafka` live in shell scripts and agent prompts — " - "this command cannot reach them. Prefix those manually with `.body | ` " - "(HTTP) or `.value | ` (Kafka). Mock `match` expressions live in the " - "config file and ARE rewritten by this command." + "this command cannot reach them (it rewrites the config file only). Mock " + "`match` expressions live in the config file and ARE rewritten by this " + "command." +) + +#: Appended to :data:`_CLI_FLAGS_NOTE_BASE` ONLY for v1 sources. ``migrate`` +#: jq-prefixes match expressions solely for v1 inputs (v2/v3 exprs are already +#: envelope-rooted); telling a v2->v3 migrator to prefix would double-prefix +#: working scripts. Composed in :func:`config_migrate` gated on +#: ``from_version.split(".")[0] == "1"`` (mirroring ``migrate_config``'s own +#: ``source_major == "1"`` gate). +_CLI_FLAGS_NOTE_V1_PREFIX = ( + " For v1 sources being lifted to v3, prefix those CLI flags manually with " + "`.body | ` (HTTP) or `.value | ` (Kafka); v2/v3 exprs are already " + "envelope-rooted and need no prefix." ) #: yaml.safe_dump reformats the file and drops comments; the original is @@ -332,13 +346,23 @@ def config_migrate( result = migrate_config(parsed) # Write only when migrating for real (not already_current, not --dry-run). will_write = not result.already_current and not dry_run + # The manual-prefix instruction applies ONLY to v1 sources (those are + # the inputs whose match expressions migrate_config actually jq-prefixed). + # For v2->v3 the exprs are already envelope-rooted, so emitting the + # prefix guidance would steer operators into double-prefixing working + # scripts. Gate on the from_version major, exactly as migrate_config + # gates its own jq-prefix walkers on source_major == "1". + is_v1_source = str(result.from_version).split(".")[0] == "1" + cli_flags_note = _CLI_FLAGS_NOTE_BASE + ( + _CLI_FLAGS_NOTE_V1_PREFIX if is_v1_source else "" + ) base_result = { "path": str(path), "already_current": result.already_current, "from_version": result.from_version, "to_version": result.to_version, "rewritten": result.rewrites, - "cli_flags_note": _CLI_FLAGS_NOTE, + "cli_flags_note": cli_flags_note, # Surfaced only when the file is actually rewritten — on --dry-run # and already_current nothing is reformatted, so the note would be noise. "formatting_note": _FORMATTING_NOTE if will_write else None, diff --git a/agctl/config/loader.py b/agctl/config/loader.py index 63254f6..ffceb7b 100644 --- a/agctl/config/loader.py +++ b/agctl/config/loader.py @@ -343,8 +343,7 @@ def _check_version(data: dict) -> None: if not version: message = ( f"Config is missing a `version`. agctl speaks dialect v{TOOL_MAJOR_VERSION}; " - f"add `version: \"{TOOL_MAJOR_VERSION}\"` (or run `agctl config migrate` " - f"on a v1 config)." + f"add `version: \"{TOOL_MAJOR_VERSION}\"` (or run `agctl config migrate`)." ) else: message = ( diff --git a/tests/unit/test_kafka_commands.py b/tests/unit/test_kafka_commands.py index e65ce67..32cc91a 100644 --- a/tests/unit/test_kafka_commands.py +++ b/tests/unit/test_kafka_commands.py @@ -1129,6 +1129,9 @@ def test_kafka_produce_single_cluster_no_flag(install_fake): assert payload["ok"] is True # The fake producer received the produce call. assert cap["producer"].calls[0]["topic"] == "t" + # The fixture's single default cluster was selected (broker resolved from + # KAFKA_BROKER=localhost) — genuinely exercising single-cluster resolution. + assert cap["cluster"].brokers == ["localhost"] def test_kafka_consume_no_default_multi_cluster_error(install_fake, tmp_path): diff --git a/tests/unit/test_migrate.py b/tests/unit/test_migrate.py index 428e42b..c940fcf 100644 --- a/tests/unit/test_migrate.py +++ b/tests/unit/test_migrate.py @@ -511,6 +511,60 @@ def test_config_migrate_result_carries_cli_flags_note(tmp_path): assert "--match" in note +def test_config_migrate_cli_flags_note_v1_includes_prefix_instruction(tmp_path): + """A v1->v3 migration DID jq-prefix the config's match expressions, so the + ``cli_flags_note`` must carry the manual ``.body |`` / ``.value |`` prefix + guidance for the CLI flags it cannot reach.""" + cfg = tmp_path / "agctl.yaml" + cfg.write_text( + 'version: "1"\n' + "mocks:\n" + " http:\n" + " stubs:\n" + " s:\n" + " match:\n" + ' jq: ".amount > 1000"\n' + ) + + result = _migrate(tmp_path, ["--config", str(cfg), "--dry-run"]) + + assert result.exit_code == 0 + note = json.loads(result.output)["result"]["cli_flags_note"] + assert "--match" in note + assert ".body | " in note + assert ".value | " in note + + +def test_config_migrate_cli_flags_note_v2_omits_prefix_instruction(tmp_path): + """A v2->v3 migration does NOT jq-prefix (v2 exprs are already + envelope-rooted), so the ``cli_flags_note`` must NOT tell the operator to + prefix — that would steer them into double-prefixing working scripts. The + base reminder (flags live in scripts/prompts, not rewritten) is still + present. Uses a flat-kafka v2 config so a real structural lift runs (i.e. + this is a genuine migration, not already_current).""" + cfg = tmp_path / "agctl.yaml" + cfg.write_text( + 'version: "2"\n' + "kafka:\n" + " brokers:\n" + " - host:9092\n" + ) + + result = _migrate(tmp_path, ["--config", str(cfg), "--dry-run"]) + + assert result.exit_code == 0 + payload = json.loads(result.output) + assert payload["result"]["already_current"] is False + # A structural rewrite ran -> this was a real v2->v3 migration. + assert payload["result"]["rewritten"] + note = payload["result"]["cli_flags_note"] + # Base reminder present. + assert "--match" in note + # Prefix instruction MUST be absent for v2->v3. + assert ".body | " not in note + assert ".value | " not in note + + def test_config_migrate_round_trip_proves_behavior(tmp_path): """After migrating a v1 config and loading the result, the rewritten HTTP stub ``match.jq`` actually matches an envelope with a qualifying body and diff --git a/tests/unit/test_mock_commands.py b/tests/unit/test_mock_commands.py index 65cb4ec..f204729 100644 --- a/tests/unit/test_mock_commands.py +++ b/tests/unit/test_mock_commands.py @@ -77,7 +77,9 @@ def test_only_http_with_stubs(self, temp_config, fake_engine): fake_engine.shutdown.assert_called_once() def test_only_kafka_with_reactors(self, temp_config, fake_engine): - """--only kafka with reactors + kafka.brokers -> run_kafka=True, kafka_client is KafkaClient.""" + """--only kafka with reactors resolved to the default v3 cluster + (kafka.clusters.default.brokers) -> run_kafka=True, kafka_clients maps + the reactor name to a KafkaClient built for that resolved cluster.""" # Skip if confluent_kafka is not installed pytest.importorskip("confluent_kafka") @@ -288,8 +290,11 @@ def test_no_mocks_section_no_only(self, temp_config, fake_engine): config_content = """ version: "3" kafka: - brokers: - - "localhost:9092" + clusters: + default: + brokers: + - "localhost:9092" + default_cluster: default """ temp_config.write_text(config_content) diff --git a/tests/unit/test_overlay.py b/tests/unit/test_overlay.py index 07406bf..5cdad85 100644 --- a/tests/unit/test_overlay.py +++ b/tests/unit/test_overlay.py @@ -686,8 +686,11 @@ def test_kafka_produce_forwards_overlay(tmp_path, monkeypatch): orders: base_url: http://localhost:8081 kafka: - brokers: - - localhost:9092 + clusters: + default: + brokers: + - localhost:9092 + default_cluster: default """) ov = tmp_path / "overlay.yaml" ov.write_text("""kafka: From f6901d0c995470e3f59c330cf94ce9852b85102e Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 10:31:08 +0300 Subject: [PATCH 10/12] fix(kafka): discover cluster resilience + resolution/error polish (code review) Batched application of 8 fixes from the 4-reviewer fan-out for the Multi-Cluster Kafka feature (PR #45). 1. discover no longer hard-fails on an unresolvable cluster: the kafka-patterns item-detail resolve_cluster_name call is wrapped so a ConfigError yields cluster=null (the item still renders topic/match/ params/example). A cluster-agnostic pattern with >1 cluster and no default_cluster passes validation, so an inspection command must not exit 2 on it. 2. resolve_cluster_name is now keyword-only (*, explicit=None, binding_cluster=None), mirroring the DB analog resolve_connection_name; all call sites updated to keyword form. 3. --cluster help shortened to "Cluster name override" across produce/consume/assert (the old text over-promised a single default source). 4. mock_run re-raises resolve_cluster_name failures with reactor context, so a dangling reactor.cluster / unresolvable cluster names the reactor. 5. migrate: comment at the v1 gate now states the missing/legacy-version assumption explicitly (do not broaden the gate). 6. migrate: _migrate_* helper rewrites param widened to list[dict[str, Any]] (structural rewrites carry non-str before/after). 7. Renamed test_mock_kafka_requires_resolvable_default_cluster_error to test_mock_kafka_reactor_no_clusters_error (accurate: builds no clusters). 8. Added test_kafka_consume_explicit_cluster (mirrors the produce test). Tests: covering (discover/kafka/mock/migrate/validator) 160 passed; full unit suite 964 passed (+2). Co-Authored-By: Claude --- agctl/commands/discover_commands.py | 21 ++++++++++--- agctl/commands/kafka_commands.py | 9 +++--- agctl/commands/mock_commands.py | 15 +++++++-- agctl/config/migrate.py | 11 ++++--- tests/unit/test_discover_command.py | 49 +++++++++++++++++++++++++++++ tests/unit/test_kafka_commands.py | 34 ++++++++++++++++---- tests/unit/test_validator.py | 2 +- 7 files changed, 118 insertions(+), 23 deletions(-) diff --git a/agctl/commands/discover_commands.py b/agctl/commands/discover_commands.py index 09f63e1..cdcaf64 100644 --- a/agctl/commands/discover_commands.py +++ b/agctl/commands/discover_commands.py @@ -317,6 +317,21 @@ def _item_core(config_path: str | None, category: str, name: str, overlay_paths: ) pat = cfg.kafka.patterns[name] params = _kafka_params(pat) + # Resolved cluster name (DESIGN §6): pattern.cluster > default_cluster + # > single-cluster auto-default. Surfaces where this pattern asserts. + # A pattern is legitimately cluster-agnostic (disambiguated via + # ``--cluster`` at ``kafka assert`` time), and a config with >1 cluster + # and no ``default_cluster`` passes validation — so an inspection command + # must NOT hard-fail on the unresolvable case. On ConfigError, set + # ``cluster = None``: the item still renders its topic/match/params/ + # example, and ``cluster: null`` signals "no implicit cluster — pass + # ``--cluster`` at assert time". + try: + cluster = resolve_cluster_name( + cfg.kafka, binding_cluster=pat.cluster + ) + except ConfigError: + cluster = None item = { "category": "kafka-patterns", "name": name, @@ -324,11 +339,7 @@ def _item_core(config_path: str | None, category: str, name: str, overlay_paths: "topic": pat.topic, "params": params, "example": _kafka_example(name, params), - # Resolved cluster name (DESIGN §6): pattern.cluster > default_cluster - # > single-cluster auto-default. Surfaces where this pattern asserts. - "cluster": resolve_cluster_name( - cfg.kafka, None, binding_cluster=pat.cluster - ), + "cluster": cluster, } if pat.match is not None: item["match"] = pat.match diff --git a/agctl/commands/kafka_commands.py b/agctl/commands/kafka_commands.py index 4962a15..4ac7b9a 100644 --- a/agctl/commands/kafka_commands.py +++ b/agctl/commands/kafka_commands.py @@ -44,7 +44,8 @@ def resolve_cluster_name( cfg_kafka: "KafkaConfig", - explicit: str | None, + *, + explicit: str | None = None, binding_cluster: str | None = None, ) -> str: """Resolve the Kafka cluster **name** to use (DESIGN §6, D2/D3). @@ -177,7 +178,7 @@ def _kafka_produce_core( @click.option("--message", "message", required=True, help="JSON message body") @click.option("--key", "key", default=None, help="Message key") @click.option("--header", "header", multiple=True, help="k=v message header") -@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)") +@click.option("--cluster", "cluster", default=None, help="Cluster name override") @click.pass_context def kafka_produce( ctx: click.Context, @@ -309,7 +310,7 @@ def predicate(msg, _expr=match_expr): @click.option("--expect-count", "expect_count", type=int, default=None, help="Min expected count") @click.option("--from-beginning", "from_beginning", is_flag=True, default=False) @click.option("--consumer-group", "consumer_group", default=None, help="Consumer group override") -@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)") +@click.option("--cluster", "cluster", default=None, help="Cluster name override") @click.pass_context def kafka_consume( ctx: click.Context, @@ -608,7 +609,7 @@ def _kafka_assert_core( default=None, help="Named custom assertion mode", ) -@click.option("--cluster", "cluster", default=None, help="Named kafka cluster (default: kafka.default_cluster)") +@click.option("--cluster", "cluster", default=None, help="Cluster name override") @click.pass_context def kafka_assert( ctx: click.Context, diff --git a/agctl/commands/mock_commands.py b/agctl/commands/mock_commands.py index e034777..2bf0da6 100644 --- a/agctl/commands/mock_commands.py +++ b/agctl/commands/mock_commands.py @@ -477,9 +477,18 @@ def mock_run( kafka_clients = {} clients_by_cluster: dict[str, KafkaClient] = {} for reactor_name, reactor in cfg.mocks.kafka.reactors.items(): - cluster_name = resolve_cluster_name( - cfg.kafka, None, binding_cluster=reactor.cluster - ) + try: + cluster_name = resolve_cluster_name( + cfg.kafka, binding_cluster=reactor.cluster + ) + except ConfigError as e: + # load_config does NOT run validate_config, so mock_run is + # the primary error surface for `agctl mock run`. Re-raise + # with reactor context so a dangling reactor.cluster or + # unresolvable (no default/single) cluster names the reactor. + raise ConfigError( + e.message, {**e.detail, "reactor": reactor_name} + ) from e cluster = cfg.kafka.clusters[cluster_name] # Per-reactor brokers guard (spec §11): a reactor whose resolved # cluster has empty brokers must fail fast with a clear diff --git a/agctl/config/migrate.py b/agctl/config/migrate.py index 9a4135e..a0b9a66 100644 --- a/agctl/config/migrate.py +++ b/agctl/config/migrate.py @@ -115,7 +115,7 @@ def _prepend(expr: str, prefix: str) -> str: return expr if expr.startswith(prefix) else prefix + expr -def _migrate_http_stubs(config: dict[str, Any], rewrites: list[dict[str, str]]) -> None: +def _migrate_http_stubs(config: dict[str, Any], rewrites: list[dict[str, Any]]) -> None: """Walk ``mocks.http.stubs..match.jq`` and prepend the HTTP prefix.""" stubs = ( config.get("mocks", {}).get("http", {}).get("stubs", {}) @@ -138,7 +138,7 @@ def _migrate_http_stubs(config: dict[str, Any], rewrites: list[dict[str, str]]) def _migrate_kafka_reactors( - config: dict[str, Any], rewrites: list[dict[str, str]] + config: dict[str, Any], rewrites: list[dict[str, Any]] ) -> None: """Walk ``mocks.kafka.reactors..match`` and prepend the Kafka prefix.""" reactors = ( @@ -159,7 +159,7 @@ def _migrate_kafka_reactors( def _migrate_kafka_patterns( - config: dict[str, Any], rewrites: list[dict[str, str]] + config: dict[str, Any], rewrites: list[dict[str, Any]] ) -> None: """Walk ``kafka.patterns..match`` and prepend the Kafka prefix.""" patterns = config.get("kafka", {}).get("patterns", {}) @@ -250,7 +250,10 @@ def migrate_config(config: dict[str, Any]) -> MigrateResult: rewrites: list[dict[str, Any]] = [] # jq-prefix walkers run ONLY on v1 sources: v2+ exprs are already - # envelope-rooted, so force-prepending would double-prefix them. + # envelope-rooted, so force-prepending would double-prefix them. A + # missing/legacy (source_major "" or "0") version is assumed already + # envelope-rooted (v2-style) and intentionally NOT jq-prefixed — do NOT + # broaden this gate to include them. if source_major == "1": # Traversal order per the brief: HTTP stubs -> Kafka reactors -> kafka.patterns. _migrate_http_stubs(new_config, rewrites) diff --git a/tests/unit/test_discover_command.py b/tests/unit/test_discover_command.py index 96752eb..be7aff8 100644 --- a/tests/unit/test_discover_command.py +++ b/tests/unit/test_discover_command.py @@ -266,6 +266,55 @@ def test_kafka_pattern_item_cluster_defaults(tmp_path, monkeypatch): assert res["cluster"] == "main" +_KAFKA_TWO_CLUSTER_NO_DEFAULT_CONFIG = ( + 'version: "3"\n' + "services:\n" + " demo:\n" + ' base_url: "http://localhost:9999"\n' + "kafka:\n" + " clusters:\n" + " main:\n" + " brokers:\n" + ' - "localhost:9092"\n' + " analytics:\n" + " brokers:\n" + ' - "analytics-host:9092"\n' + # NO default_cluster: validation passes (a pattern is cluster-agnostic). + " patterns:\n" + " evt:\n" + ' description: "A cluster-agnostic event"\n' + " topic: events\n" + ' match: \'.value.eventType == "EVT"\'\n' + # NO cluster field — disambiguated via --cluster at kafka assert time. +) + + +def test_kafka_pattern_item_unresolvable_cluster_is_null(tmp_path, monkeypatch): + """Two clusters, no default_cluster, pattern with no cluster field: discover + is resilient — it returns ok:true with cluster: null (still rendering topic/ + match/params/example), NOT a hard ConfigError. The cluster is disambiguated + via --cluster at ``kafka assert`` time.""" + result = _run_with( + ["--category", "kafka-patterns", "--name", "evt"], + _KAFKA_TWO_CLUSTER_NO_DEFAULT_CONFIG, + tmp_path, + monkeypatch, + ) + assert result.exit_code == 0 + payload = _payload(result) + assert payload["ok"] is True + res = payload["result"] + assert res["name"] == "evt" + # No resolvable cluster (no pattern cluster, no default, >1 cluster) → null. + assert res["cluster"] is None + # Item still renders its full detail. + assert res["topic"] == "events" + assert res["match"] == '.value.eventType == "EVT"' + # No {brace} tokens in the match → empty params, simple example. + assert res["params"] == [] + assert res["example"] == "agctl kafka assert --pattern evt --timeout 10" + + def test_item_service(monkeypatch): result = _run(["--category", "services", "--name", "order-service"], monkeypatch) assert result.exit_code == 0 diff --git a/tests/unit/test_kafka_commands.py b/tests/unit/test_kafka_commands.py index 32cc91a..628158f 100644 --- a/tests/unit/test_kafka_commands.py +++ b/tests/unit/test_kafka_commands.py @@ -1167,17 +1167,17 @@ def test_resolve_cluster_name_explicit_wins(): }, default_cluster="a", ) - assert kafka_commands.resolve_cluster_name(k, "b") == "b" + assert kafka_commands.resolve_cluster_name(k, explicit="b") == "b" # binding_cluster beats default - assert kafka_commands.resolve_cluster_name(k, None, "b") == "b" + assert kafka_commands.resolve_cluster_name(k, binding_cluster="b") == "b" # default when no explicit/binding - assert kafka_commands.resolve_cluster_name(k, None) == "a" + assert kafka_commands.resolve_cluster_name(k) == "a" def test_resolve_cluster_name_single_cluster_auto_default(): """One cluster defined, no default_cluster -> that cluster auto-resolves.""" k = KafkaConfig(clusters={"only": KafkaCluster(brokers=["h:9092"])}) - assert kafka_commands.resolve_cluster_name(k, None) == "only" + assert kafka_commands.resolve_cluster_name(k) == "only" def test_resolve_cluster_name_unknown_cluster_errors(): @@ -1188,7 +1188,7 @@ def test_resolve_cluster_name_unknown_cluster_errors(): default_cluster="a", ) with pytest.raises(ConfigError) as exc: - kafka_commands.resolve_cluster_name(k, "ghost") + kafka_commands.resolve_cluster_name(k, explicit="ghost") assert "Unknown kafka cluster: ghost" in exc.value.message assert exc.value.detail["cluster"] == "ghost" @@ -1202,7 +1202,7 @@ def test_resolve_cluster_name_no_cluster_specified_errors(): } ) with pytest.raises(ConfigError) as exc: - kafka_commands.resolve_cluster_name(k, None) + kafka_commands.resolve_cluster_name(k) assert exc.value.message == "No kafka cluster specified" assert exc.value.detail == {} @@ -1264,6 +1264,28 @@ def test_kafka_produce_explicit_cluster(install_fake, tmp_path): assert cap["cluster"].brokers == ["broker-b:9092"] +def test_kafka_consume_explicit_cluster(install_fake, tmp_path): + """`consume --cluster analytics` selects the analytics cluster: the fake + client is built from analytics's brokers (broker-b), not the default main + (broker-a). Mirrors ``test_kafka_produce_explicit_cluster``.""" + cap = install_fake([]) + cfg = _write_two_cluster_cfg(tmp_path) + result = _run( + [ + "--config", str(cfg), + "kafka", "consume", + "--topic", "t", + "--cluster", "analytics", + "--timeout", "1", + ] + ) + payload = _payload(result) + assert result.exit_code == 0 + assert payload["ok"] is True + # The factory received the analytics cluster (broker-b), not main (broker-a). + assert cap["cluster"].brokers == ["broker-b:9092"] + + def test_kafka_assert_pattern_resolves_cluster(install_fake, tmp_path): """`assert --pattern ord` (no --cluster, no --topic) resolves BOTH the topic and the cluster from the pattern binding (analytics). The fake client is diff --git a/tests/unit/test_validator.py b/tests/unit/test_validator.py index a85dbc7..0712843 100644 --- a/tests/unit/test_validator.py +++ b/tests/unit/test_validator.py @@ -276,7 +276,7 @@ def test_read_mode_template_with_read_only_connection_no_error(): # --- mock server validation --------------------------------------------------- -def test_mock_kafka_requires_resolvable_default_cluster_error(): +def test_mock_kafka_reactor_no_clusters_error(): """mocks.kafka.reactors non-empty but no resolvable cluster -> error at mocks.kafka (no default/single cluster resolves).""" cfg = _cfg( From d087248160aed4675b3a10c65258cf3757f88da3 Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 10:38:49 +0300 Subject: [PATCH 11/12] docs(design): note discover kafka-patterns cluster field is nullable Following the discover cluster-resilience fix (f6901d0), the kafka-patterns discover.item now yields cluster: null (instead of exit 2) for a cluster-agnostic pattern with >1 cluster and no default_cluster. The discover.item shape contract now reflects this nullable case. Co-Authored-By: Claude --- docs/DESIGN.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 5d93496..de8f7b3 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -1937,7 +1937,7 @@ Every service entry always includes `response_time_ms` (an integer on success, ` #### `discover.item` -Shape varies by category. All items share `name`, `description`, `params[]`, and `example`. HTTP templates add `method`, `service`, `path`; Kafka patterns add `topic`, `cluster` (the resolved cluster name), and `match`; DB templates add `connection` and `sql` (so an agent can read a query's result columns before writing a `--path` value assertion). +Shape varies by category. All items share `name`, `description`, `params[]`, and `example`. HTTP templates add `method`, `service`, `path`; Kafka patterns add `topic`, `cluster` (the resolved cluster name, or `null` when no cluster resolves), and `match`; DB templates add `connection` and `sql` (so an agent can read a query's result columns before writing a `--path` value assertion). #### `discover.search` From 2a10f7fa175674a46e5bb1d8b4812c0df2e9550f Mon Sep 17 00:00:00 2001 From: Dmitry Teryaev Date: Fri, 10 Jul 2026 10:50:33 +0300 Subject: [PATCH 12/12] fix(test): migrate integration test configs to v3 (CI) The v2->v3 version bump (Task 1) migrated unit fixtures but missed the inline integration configs in test_logs_commands.py, test_mock_daemon.py, and test_mock_commands.py (_build_config + reactor e2e). They still had version: "2" / flat kafka, so the v3 guard rejected them at load -> "Config dialect v2 is no longer supported". CI (full suite) caught what unit-only review runs missed. Bump to v3 + convert flat kafka to clusters. Co-Authored-By: Claude --- tests/integration/test_logs_commands.py | 2 +- tests/integration/test_mock_commands.py | 14 ++++++++++---- tests/integration/test_mock_daemon.py | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/integration/test_logs_commands.py b/tests/integration/test_logs_commands.py index d43a56d..b318ef5 100644 --- a/tests/integration/test_logs_commands.py +++ b/tests/integration/test_logs_commands.py @@ -23,7 +23,7 @@ def _write_config(tmp_path, log_path): """Write a minimal agctl.yaml config with a logs source pointing at log_path.""" config_path = tmp_path / "agctl.yaml" - config_content = f"""version: "2" + config_content = f"""version: "3" services: demo: base_url: "http://localhost:9999" diff --git a/tests/integration/test_mock_commands.py b/tests/integration/test_mock_commands.py index 253e383..addec44 100644 --- a/tests/integration/test_mock_commands.py +++ b/tests/integration/test_mock_commands.py @@ -30,9 +30,12 @@ def _build_config(mocks_config: dict) -> str: """Build a minimal agctl.yaml with mocks section.""" base_config = { - "version": "2.0", + "version": "3", "kafka": { - "brokers": ["localhost:9092"] + "clusters": { + "default": {"brokers": ["localhost:9092"]} + }, + "default_cluster": "default" }, "mocks": mocks_config } @@ -600,9 +603,12 @@ def test_kafka_mock_run_with_react(self, require_kafka, tmp_path): # Create temp config with reactor # Build the config inline to ensure testcontainers broker is used config_content = json.dumps({ - "version": "2.0", + "version": "3", "kafka": { - "brokers": [broker], + "clusters": { + "default": {"brokers": [broker]} + }, + "default_cluster": "default", }, "mocks": { "kafka": { diff --git a/tests/integration/test_mock_daemon.py b/tests/integration/test_mock_daemon.py index ecd959b..12276de 100644 --- a/tests/integration/test_mock_daemon.py +++ b/tests/integration/test_mock_daemon.py @@ -41,7 +41,7 @@ def _allocate_free_port() -> int: def _write_test_config(config_path: Path, port: int) -> None: """Write a minimal agctl.yaml config with one HTTP stub.""" config_path.write_text( - f"""version: "2.0" + f"""version: "3" mocks: http: listen: "127.0.0.1:{port}"