diff --git a/README.md b/README.md index 2e3e126..15f4ecf 100644 --- a/README.md +++ b/README.md @@ -276,7 +276,7 @@ The `config` dict passed to `mount()` uses the same keys as the `overrides.hook- |---------|----------|---------|-------------| | `url` | yes | — | Base URL of the CI server for this destination. | | `api_key` | yes | — | Bearer token for this destination. Sent as `Authorization: Bearer ` on that destination's POSTs only — because each destination references its own `${VAR}`, distinct keys never cross between servers. | -| `include` | no | `["**"]` | `.gitignore`-style patterns matched against the session's working directory. The destination is a candidate when any pattern matches. | +| `include` | no | *(none — matches nothing)* | `.gitignore`-style patterns matched against the session's working directory. The destination is a candidate when any pattern matches; an omitted or empty `include` matches **nothing** (fail-closed) and the destination is inactive — set `include: ["**"]` explicitly to match every session. | | `exclude` | no | `[]` | `.gitignore`-style patterns; if any matches, the destination is dropped for that session (**exclude wins**). | ```yaml @@ -309,7 +309,7 @@ overrides: Prefer the trailing-slash directory form (e.g. `**/work/`) to mean "this project and all its sessions" — it matches whether the session is launched from the project **root** or any subdirectory. (A pattern that targets only contents, like `**/work/**`, still also matches the directory itself here, because the match key is a directory.) -**Defaults & validation.** Omitted `include` defaults to `["**"]` (match everything); omitted `exclude` defaults to none. After `${VAR}` expansion, a `destinations` entry whose `url` **or** `api_key` is empty is a **mount error** (fail-fast, naming the offending destination). With no `destinations` configured, the hook is local-JSONL-only. (The legacy scalar path is intentionally more lenient — see [Deprecated — legacy single-server scalars](#deprecated--legacy-single-server-scalars) below.) +**Defaults & validation.** An omitted or empty `include` matches **nothing** — the destination never receives events (fail-closed); to receive everything, set `include: ["**"]` explicitly. Validation logs a per-destination WARNING when this happens (an inactive destination is legal, just easy to configure by accident). Omitted `exclude` defaults to none. After `${VAR}` expansion, a `destinations` entry whose `url` **or** `api_key` is empty is a **mount error** (fail-fast, naming the offending destination). With no `destinations` configured, the hook is local-JSONL-only. (The legacy scalar path synthesizes its single `default` destination with `include: ["**"]` — see [Deprecated — legacy single-server scalars](#deprecated--legacy-single-server-scalars) below — and is intentionally more lenient on url/api_key.) **Per-project override.** Because `destinations` is keyed by name, a project `.amplifier/settings.yaml` can override a single destination's `include`/`exclude` (e.g. `destinations.team.include`) without restating the others — the app-cli deep-merges user → project settings. diff --git a/modules/hook-context-intelligence/amplifier_module_hook_context_intelligence/config_resolver.py b/modules/hook-context-intelligence/amplifier_module_hook_context_intelligence/config_resolver.py index b272484..bcb10dd 100644 --- a/modules/hook-context-intelligence/amplifier_module_hook_context_intelligence/config_resolver.py +++ b/modules/hook-context-intelligence/amplifier_module_hook_context_intelligence/config_resolver.py @@ -741,6 +741,20 @@ def validate_destinations(self) -> dict[str, Destination]: configured any destination at all (the latter is this method's ordinary, silent local-only case per the Returns note below). + Empty-include WARNING (fail-closed surprise, not a failure): a + destination that survives url/api_key/auth_mode validation but has + an empty ``include`` (omitted or ``[]`` \u2014 see the ``Destination`` + docstring above) is legal \u2014 it is simply permanently inactive, since + an empty pattern set matches no session (``fanout.py``'s + ``_matches`` returns ``False`` on empty patterns). That is easy to + configure by accident (a hand-typed destination with no ``include`` + line looks identical to one meant to match everything), so it gets + its own per-destination WARNING naming the destination \u2014 distinct + from the misconfigured-url/api_key path above, and it does NOT drop + the destination from the returned dict (it remains a legitimate, + just currently inactive, target \u2014 e.g. still selectable by name via + the query tools' ``source=`` override). + Returns: The validated (surviving) destinations dict \u2014 possibly empty, which is always OK (local-only, S4). Never raises. @@ -793,6 +807,25 @@ def validate_destinations(self) -> dict[str, Destination]: ) continue + if not dest.include: + # Fail-closed by design (see the Destination docstring and + # fanout.py's _matches): an empty include pattern set means + # this destination will never match any session. That is a + # legal configuration (e.g. a destination meant only to be + # reached explicitly via the query tools' source= override), + # but it is also easy to produce by accident, so it gets its + # own WARNING -- distinct from the misconfigured-url/api_key + # path above -- and does NOT drop the destination. + log.warning( + "context-intelligence destination %r: no include patterns " + "-- this destination will never match any session and " + 'will receive nothing; set include: ["**"] to receive ' + "all sessions. Fix under " + "overrides.hook-context-intelligence.config.destinations.%s.include.", + name, + name, + ) + valid[name] = dest if dests and not valid: diff --git a/modules/hook-context-intelligence/tests/test_destinations_validation.py b/modules/hook-context-intelligence/tests/test_destinations_validation.py index 2a121b8..e3300ff 100644 --- a/modules/hook-context-intelligence/tests/test_destinations_validation.py +++ b/modules/hook-context-intelligence/tests/test_destinations_validation.py @@ -250,3 +250,82 @@ def test_unknown_auth_mode_dropped_and_logged_as_error( assert result == {} errors = [rec for rec in caplog.records if rec.levelno == logging.ERROR] assert any("weird" in rec.message and "kerberos" in rec.message for rec in errors) + + +class TestEmptyIncludeWarning: + """A destination with an empty ``include`` (omitted or ``[]``) is fail-closed -- + it matches no session (see ``Destination`` docstring, ``fanout.py``'s ``_matches``) + -- but that is a LEGAL configuration, not a validation failure. It must still be + a WARNING (not an error), must NOT drop the destination from the returned dict, + and must not fire for a destination whose ``include`` is actually populated + (explicitly, or via the legacy-scalar synthesis, which always sets ``("**",)``).""" + + def test_omitted_include_warns_and_destination_survives( + self, caplog: pytest.LogCaptureFixture + ) -> None: + r = _resolver({"destinations": {"quiet": {"url": "http://q:8000", "api_key": "qk"}}}) + with caplog.at_level(logging.WARNING): + result = r.validate_destinations() + assert set(result.keys()) == {"quiet"} + warnings = [rec for rec in caplog.records if rec.levelno == logging.WARNING] + assert any( + "quiet" in rec.message and "no include patterns" in rec.message for rec in warnings + ) + + def test_explicit_empty_include_list_warns_and_destination_survives( + self, caplog: pytest.LogCaptureFixture + ) -> None: + r = _resolver( + {"destinations": {"quiet": {"url": "http://q:8000", "api_key": "qk", "include": []}}} + ) + with caplog.at_level(logging.WARNING): + result = r.validate_destinations() + assert set(result.keys()) == {"quiet"} + assert any( + "quiet" in rec.message and "no include patterns" in rec.message + for rec in caplog.records + ) + + def test_nonempty_include_does_not_warn(self, caplog: pytest.LogCaptureFixture) -> None: + r = _resolver( + { + "destinations": { + "active": { + "url": "http://a:8000", + "api_key": "ak", + "include": ["**"], + } + } + } + ) + with caplog.at_level(logging.WARNING): + result = r.validate_destinations() + assert set(result.keys()) == {"active"} + assert not any("no include patterns" in rec.message for rec in caplog.records) + + def test_legacy_scalar_synthesized_destination_does_not_warn( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """The legacy scalar path always synthesizes include=("**",) -- it must + never trigger the empty-include warning.""" + r = _resolver( + { + "context_intelligence_server_url": "http://legacy:8000", + "context_intelligence_api_key": "legacy-key", + } + ) + with caplog.at_level(logging.WARNING): + result = r.validate_destinations() + assert set(result.keys()) == {"default"} + assert not any("no include patterns" in rec.message for rec in caplog.records) + + def test_dropped_destination_does_not_also_get_include_warning( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """A destination dropped for a bad url/api_key must not ALSO emit the + empty-include warning -- it never reaches that check.""" + r = _resolver({"destinations": {"broken": {"url": "", "api_key": "k"}}}) + with caplog.at_level(logging.WARNING): + result = r.validate_destinations() + assert result == {} + assert not any("no include patterns" in rec.message for rec in caplog.records)