From 702cd09fcee1c361b90f1727e608a13be6c0aeb5 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 3 Aug 2026 15:37:47 -0500 Subject: [PATCH] fix(wiring): reject validate_directory on an outbound instead of silently ignoring it (BACKLOG #114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validate_directory is an inbound-only option: only SourceConnector carries the validate_startup hook the runner awaits at bind, and no destination reads the setting. But File()/Sftp()/Ftp() are single factories serving BOTH directions, so the option could be written onto an outbound, where it was accepted and silently ignored — an operator asking for fail-fast startup validation got none and saw no error. It is now a WiringError at bind, raised in build_outbound_connection. That is the single choke point both the code-first outbound() factory and the connections.toml loader (ADR 0007) pass through, so one guard covers both authoring surfaces — and a TOML test pins that rather than leaving it as an assertion in a commit message. The guard is truthy-only, so the False the factories always write into settings is unaffected and every outbound authored today builds byte-identically; the inbound half is untouched. Scope is the wiring rejection only, per the item's own stated MVP. The fuller build — an outbound DestinationConnector.validate_startup hook — stays deferred. While recording the remainder, one of #114's own premises turned out to be false and is corrected in the ledger and in ADR 0031's follow-on: the "clean workaround via the on-demand test probe" holds for the INBOUND only. Both destinations' test_connection *create* the target directory (FileDestination -> _probe_dir_writable, whose first line is mkdir(parents=True, exist_ok=True); RemoteFileDestination -> ensure_dir -> mkd), so on an outbound nothing shipped can distinguish "the directory exists" from "I just made it" — a typo'd target path is fabricated and every message reports delivered. That raises the outbound half's value above its recorded 5/10, and argues the hook should ship together with suppressing the mkdir-on-write rather than as a start-time-only check under a name that promises more. Tests: outbound rejection, inbound-still-builds, and outbound-without-the-flag across File/Sftp/Ftp (tests/test_wiring.py), plus the connections.toml desugar (tests/test_connections_file.py). --- docs/BACKLOG.md | 6 +-- docs/CONNECTIONS.md | 4 +- ...0031-startup-connection-fault-isolation.md | 12 +++++ messagefoundry/config/wiring.py | 30 ++++++++--- tests/test_connections_file.py | 19 +++++++ tests/test_wiring.py | 53 +++++++++++++++++++ 6 files changed, 113 insertions(+), 11 deletions(-) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 09439513..f1864eb2 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -247,7 +247,7 @@ Ordered by value descending, then difficulty ascending (cheapest first at equal | 44 | **#190** | PHI data-plane integrity defaults: JWS signing, GCM rekey counter, keyed audit chain | 6 | 7 | _big bet_ | P2 | Removes real audit-forgery and GCM-nonce integrity blind spots; cross-cutting crypto plus keying the persisted audit chain across three store backends. | | 45 | **#94** | External BLOB-server offload for embedded documents — replace inline base64 with a stored-object pointer (OBX-5 RP) (P2, on-trigger) | 6 | 5–6 | _big bet_ | DEMAND-GATE | **RE-PRICED 2026-07-28 (difficulty 8 → 5–6; row left in its original position, so ordering here is stale by one slot).** Still the strongest store-bloat lever, but ADR 0105/#149 shipped the substrate and **reserved the deref seam** (`parsing/binary.py:55-62`, `:252`), so the remainder sits behind an existing seam. Still ADR-first, still demand-gated. | | 46 | **#149** | Streaming path for very-large single messages | 6 | 9 | _big bet_ | ✅ SHIPPED | **COMPLETE 2026-07-13 (ADR 0105):** all phases shipped — substrate + ingress detach + delivery re-attach + retention decref + SS/PG parity + operator read/download surface, all three backends. Lifted the 16 MiB engine ceiling for monolithic bodies #94/split can't decompose. | -| 47 | **#114** | Directory validation toggle (perform vs suppress startup validation) | 5 | 2 | _fill-in_ | DEMAND-GATE | **PARTIAL (2026-07-28 sweep):** the INBOUND half is BUILT; the outbound half has no hook at all, and `File(validate_directory=True)` on an outbound is silently accepted-and-ignored with no `WiringError`. Corepoint-parity File toggle to fail-fast on an invalid startup directory; clean workaround via the on-demand test probe plus existing run-time deferral. | +| 47 | **#114** | Directory validation toggle (perform vs suppress startup validation) | 5 | 2 | _fill-in_ | DEMAND-GATE | **PARTIAL (2026-08-03):** the INBOUND half is BUILT, and the outbound now raises a `WiringError` instead of silently ignoring the option. **Remainder = the outbound validation hook**, whose score needs revisiting: the "clean workaround via the on-demand test probe" holds for the inbound only — the outbound probe *creates* the directory, so a typo'd target is fabricated and reports delivered. Corepoint-parity File toggle to fail-fast on an invalid startup directory. | | 48 | **#120** | Application log-file retention (auto-delete after N days) | 5 | 2 | _fill-in_ | DEMAND-GATE | Real gap: NSSM rotates by size but never deletes old log files, so disk grows unbounded; external log rotation is a clean workaround; additive sweep. | | 49 | **#132** | Fixed 'now' test-time override (frozen clock for reproducible transform tests) | 5 | 2 | _fill-in_ | DEMAND-GATE | Corepoint-parity frozen-clock aid for reproducible dry-run testing; route_message already accepts ingest_time, only a CLI --now flag is missing. | | 50 | **#146** | Per-rule alert recipients | 5 | 2 | _fill-in_ | ✅ SHIPPED | **SHIPPED (2026-07-28 sweep, ADR 0014 amendment):** per-rule `recipients` re-targeting the email transport, fail-closed on blanks. Email-only; addresses are never read back through the API. Corepoint-parity alert routing with a clean global-email_to workaround; small additive recipients field on the pure-data AlertRule. | @@ -4731,9 +4731,9 @@ sourced — **#1 (SQL Server concurrency)** and **#2 (console off-thread)** — > 🔢 **Re-scored 2026-07-10 → DEMAND-GATE.** Value **5/10** · Difficulty **2/10** · _fill-in_. Corepoint-parity File toggle to fail-fast on an invalid startup directory; clean workaround via the on-demand test probe plus existing run-time deferral. _(was DEMAND-GATE · V2/5 · D2/5)_ > **On-trigger / demand-gate.** Numbered for tracking only — build when the trigger below fires (“demand-gate, don’t schedule”). -> **AMENDED 2026-07-28 — the INBOUND half is fully BUILT; only the outbound half remains.** Adversarial verification refuted a full close. **BUILT:** `validate_directory` on the File source (`messagefoundry/transports/file.py:311`) with its opt-in at-start check (`:389-398`) — a no-mkdir probe that reports the connection `failed` at start rather than deferring to first poll (`:170`). +> **AMENDED 2026-08-03 — the INBOUND half and the outbound WIRING REJECTION are BUILT; only the outbound validation HOOK remains.** Adversarial verification refuted a full close. **BUILT 2026-07-28:** `validate_directory` on the File/RemoteFile source (`messagefoundry/transports/file.py:311`, `remotefile.py:735`) with its opt-in at-start check (`file.py:389-398`) — a no-mkdir probe that reports the connection `failed` at start rather than deferring to first poll (`file.py:170`). **BUILT 2026-08-03:** the option on an **outbound** is now a **`WiringError` at bind** (`build_outbound_connection`, `messagefoundry/config/wiring.py`) instead of being accepted and silently ignored. That is the single choke point both code-first `outbound()` and the `connections.toml` loader (ADR 0007) pass through, so one guard covers both authoring surfaces; it is truthy-only, so the `False` the factories always write into settings is unaffected and every outbound authored today builds byte-identically. > -> ⚠️ **The REMAINDER is the outbound half, and today it fails SILENTLY.** `DestinationConnector` has **no `validate_startup` hook at all**: `FileDestination` never reads the setting and still `mkdir`s on write, and the runner's outbound start path never validates. Worse, `File(validate_directory=True)` on an **outbound** is accepted and ignored with **no `WiringError`** — an operator can ask for fail-fast validation, get none, and see no error. ADR 0031's amendment scopes the outbound out as *"out of scope here"*, which is a **deferral, not a decline**. Minimum viable fix is the wiring rejection; the hook is the fuller build. +> ⚠️ **REMAINDER: the outbound validation HOOK — and this item's scoring rationale is WRONG for that direction.** `DestinationConnector` still has no `validate_startup` hook and `FileDestination` still `mkdir`s on write. The "clean workaround via the on-demand test probe" cited in the score above **does not exist on an outbound**: both destinations' `test_connection` *create* the target directory (see ADR 0031's 2026-08-03 follow-on for the call chain), so nothing shipped can tell "the directory exists" from "I just made it" — a typo'd target path is fabricated and every message reports delivered. **Re-score against that.** And if the hook is built, build it **together with** suppressing the mkdir-on-write under the flag: a start-time-only check leaves the run-time fabrication intact under a setting name that promises otherwise. **Cluster:** Connections & Transports. **Priority:** P3. **Verdict:** demand-gate. **Severity (vs Corepoint):** minor. diff --git a/docs/CONNECTIONS.md b/docs/CONNECTIONS.md index c855283f..0342d437 100644 --- a/docs/CONNECTIONS.md +++ b/docs/CONNECTIONS.md @@ -629,7 +629,7 @@ def route(msg): | `sort` | in | `name` | process order: `name` or `mtime` | | `recursive` | in | `false` | also scan subdirectories | | `max_file_bytes` | in | `16 MiB` | route files larger than this to the error dir instead of reading them into memory (OOM guard). `None`/`0` = unlimited. | -| `validate_directory` | in | `false` | validate the poll directory **at startup** (#114): a missing/unusable dir reports the connection **`failed`** (ADR 0031) instead of the default deferral to run time. No mkdir — a merely-missing dir fails. A `leave` source validates read-only (a read-only share passes); `move`/`delete` also require write. | +| `validate_directory` | in | `false` | validate the poll directory **at startup** (#114): a missing/unusable dir reports the connection **`failed`** (ADR 0031) instead of the default deferral to run time. No mkdir — a merely-missing dir fails. A `leave` source validates read-only (a read-only share passes); `move`/`delete` also require write. **Inbound only** — on an outbound it is a `WiringError` at bind (an outbound target directory is never validated at startup; it is `mkdir`ed on write). | | `processed_subdir` / `error_subdir` | in | `.processed` / `.error` | where read/failed files go | | `filename` | out | `{MSH-10}.hl7` | output name (supports `{HL7-path}` placeholders). Resolved values are sanitized to a **single safe filename** — path separators/unsafe chars stripped, leading dots removed, and `.`/`..`/reserved device names fall back — so a message field can never write outside the directory. | | `overwrite` | out | `false` | overwrite vs. uniquify a name collision (collisions are resolved by an **atomic** exclusive create, so concurrent writes never clobber) | @@ -850,7 +850,7 @@ poll/write shape against a remote server, selected by an internal `protocol` set | `min_age_seconds` | in | `0.0` | **accepted but not honoured on a remote source today** — the connector never reads it (a remote directory listing carries no reliable mtime). Only `File(...)` implements it; use `after_read`/the partner's own write-then-rename to avoid partial reads. | | `after_read` | in | `move` | `move` (→ `processed_subdir`), `delete`, or `leave` (process **in place**, #142 — a durable dedup ledger keyed on a hash of the **full remote path** + size ensures a left file is ingested once) | | `max_file_bytes` | in | `16 MiB` | move a file larger than this to `error_subdir` instead of retrieving it (OOM guard). `None`/`0` = unlimited. | -| `validate_directory` | in | `false` | validate `remote_dir` **at startup** (#114): unreachable/unusable reports the connection **`failed`** (ADR 0031) instead of deferring to run time | +| `validate_directory` | in | `false` | validate `remote_dir` **at startup** (#114): unreachable/unusable reports the connection **`failed`** (ADR 0031) instead of deferring to run time. **Inbound only** — on an outbound it is a `WiringError` at bind (the upload dir is `ensure_dir`ed on write, never validated at startup). | | `processed_subdir` / `error_subdir` | in | `.processed` / `.error` | where read / failed files go | | `filename` | out | `{MSH-10}.hl7` | upload name (supports `{HL7-path}` placeholders, sanitized to a **single safe filename** exactly as `File(...)`) | | `overwrite` | out | `false` | overwrite vs. uniquify a name collision (never a silent clobber) | diff --git a/docs/adr/0031-startup-connection-fault-isolation.md b/docs/adr/0031-startup-connection-fault-isolation.md index 861d2658..b3550d40 100644 --- a/docs/adr/0031-startup-connection-fault-isolation.md +++ b/docs/adr/0031-startup-connection-fault-isolation.md @@ -196,3 +196,15 @@ surfacing and the `connection_stopped` alert. Reload stays fail-fast for the res check only runs at bind, so a below-threshold / not-deployed / auto-start-off connection is unaffected. The equivalent outbound (FileDestination) is out of scope here — it already `mkdir`s on write and has the on-demand `POST /connections/{name}/test` probe. + +**Follow-on (2026-08-03, BACKLOG #114) — the outbound rejects the option rather than ignoring it.** +Because `File()`/`Sftp()`/`Ftp()` are single factories serving both directions, the option above could +be *written* onto an outbound, where nothing reads it — accepted and silently ignored. That is now a +**`WiringError` at bind** in `build_outbound_connection`, the one choke point both the code-first +`outbound()` and the `connections.toml` loader (ADR 0007) pass through. Truthy-only, so the `False` +the factories always write is unaffected and every outbound authored today builds byte-identically. +The outbound *validation hook* itself (`DestinationConnector.validate_startup`) remains **out of +scope** and deferred — note that the "on-demand test probe" workaround cited above is **inbound-only +in effect**: `FileDestination.test_connection` → `_probe_dir_writable` and +`RemoteFileDestination.test_connection` → `ensure_dir` both **create** the target directory, so on an +outbound no shipped mechanism can distinguish "the directory exists" from "I just made it." diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 18ebb76c..15e0f3f1 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -1455,7 +1455,7 @@ def File( sort: str = "name", # inbound: process order — "name" | "mtime" recursive: bool = False, # inbound: also scan subdirectories max_file_bytes: int | None = 16 * 1024 * 1024, # inbound: skip files over this (OOM guard) - validate_directory: bool = False, # inbound: fail-fast at start on a missing/unusable dir (#114); default defers to run time + validate_directory: bool = False, # inbound ONLY (a WiringError on an outbound, #114): fail-fast at start on a missing/unusable dir; default defers to run time overwrite: bool = False, # outbound: overwrite vs. uniquify a name collision processed_subdir: str = ".processed", error_subdir: str = ".error", @@ -1479,9 +1479,10 @@ def File( ``after_read`` (inbound) chooses the source-file disposition: ``move`` (→ ``processed_subdir``, the default), ``delete``, or ``leave`` — **process in place** for a read-only share / a directory another system owns (#142; a HASHED per-file ledger dedups so a left file is ingested once). - ``validate_directory`` (inbound, #114) makes a missing/unusable directory **fail startup** (the - connection is reported ``failed``) instead of the default deferral to run time; a ``leave`` source - validates read-only (a read-only share passes). + ``validate_directory`` (**inbound only**, #114) makes a missing/unusable directory **fail startup** + (the connection is reported ``failed``) instead of the default deferral to run time; a ``leave`` + source validates read-only (a read-only share passes). On an **outbound** it raises a + :class:`WiringError` at bind — no destination reads it, so accepting it would be a silent no-op. ``credential_username`` / ``credential_domain`` / ``credential_password`` (ADR 0132, #111) give the endpoint an **alternate Windows identity** for a UNC/SMB share, distinct from the engine service @@ -2433,7 +2434,7 @@ def Sftp( ] = "move", # inbound: "move" (to processed_subdir) | "delete" | "leave" (process in place, #142) min_age_seconds: float = 0.0, # inbound: skip files modified within this window (partial writes) max_file_bytes: int | None = 16 * 1024 * 1024, # inbound: skip files over this (OOM guard) - validate_directory: bool = False, # inbound: fail-fast at start on an unreachable remote dir (#114) + validate_directory: bool = False, # inbound ONLY (a WiringError on an outbound, #114): fail-fast at start on an unreachable remote dir overwrite: bool = False, # outbound: overwrite vs. uniquify a name collision processed_subdir: str = ".processed", error_subdir: str = ".error", @@ -2493,7 +2494,7 @@ def Ftp( ] = "move", # inbound: "move" (to processed_subdir) | "delete" | "leave" (process in place, #142) min_age_seconds: float = 0.0, # inbound: skip files modified within this window (partial writes) max_file_bytes: int | None = 16 * 1024 * 1024, # inbound: skip files over this (OOM guard) - validate_directory: bool = False, # inbound: fail-fast at start on an unreachable remote dir (#114) + validate_directory: bool = False, # inbound ONLY (a WiringError on an outbound, #114): fail-fast at start on an unreachable remote dir overwrite: bool = False, # outbound: overwrite vs. uniquify a name collision processed_subdir: str = ".processed", error_subdir: str = ".error", @@ -3782,6 +3783,23 @@ def build_outbound_connection( f"outbound connection {name!r}: {kind} outbound requires a host (the downstream peer), " f"e.g. {kind.title()}(host=..., port=...)." ) + if spec.type in (ConnectorType.FILE, ConnectorType.REMOTEFILE) and spec.settings.get( + "validate_directory" + ): + # BACKLOG #114: validate_directory is INBOUND-ONLY. Only SourceConnector carries the + # validate_startup hook the runner awaits at bind (pipeline/wiring_runner.py) — no destination + # reads the setting, and DestinationConnector has no equivalent hook. Set on an outbound it was + # accepted and silently ignored, so an operator asking for fail-fast validation got none and saw + # no error. It is caught HERE rather than in File()/Sftp()/Ftp() because those factories serve + # BOTH directions and cannot know which one they are — only the bind does. (Same choke-point + # reasoning as the capture_response guard below, which spells it out.) Truthy-only, so the + # `False` the factories always write into settings keeps building byte-identically. + kind = spec.type.value.upper() + raise WiringError( + f"outbound connection {name!r}: validate_directory is an inbound-only option — an outbound " + f"{kind} never reads it, so requesting it here would be a silent no-op. Remove it. " + "(Startup validation of an outbound target directory is not implemented; see BACKLOG #114.)" + ) _check_metadata(name, metadata) # ADR 0013 Increment 2: reingress_to (route this outbound's reply back as a new inbound message) # IMPLIES capture (the reply must be captured to re-ingress it). Force capture_response here so the diff --git a/tests/test_connections_file.py b/tests/test_connections_file.py index 84432ffc..60d5e070 100644 --- a/tests/test_connections_file.py +++ b/tests/test_connections_file.py @@ -303,6 +303,25 @@ def test_duplicate_name_across_file_and_code_fails(tmp_path: Path) -> None: load_config(cfg) +def test_outbound_validate_directory_is_rejected_from_toml(tmp_path: Path) -> None: + # #114: the guard lives in build_outbound_connection, the single choke point BOTH authoring + # surfaces pass through — so the data-authored outbound is rejected by the same rule as the + # code-first one, with no second check in the TOML reader. This test pins that claim. + cfg = _config( + tmp_path, + """ + [[outbound]] + name = "OB_FILE" + transport = "file" + [outbound.settings] + directory = "out" + validate_directory = true + """, + ) + with pytest.raises(WiringError, match="validate_directory is an inbound-only option"): + load_config(cfg) + + def test_unknown_transport_fails(tmp_path: Path) -> None: cfg = _config( tmp_path, diff --git a/tests/test_wiring.py b/tests/test_wiring.py index 24f26f37..e57337a1 100644 --- a/tests/test_wiring.py +++ b/tests/test_wiring.py @@ -14,9 +14,12 @@ from messagefoundry.config.wiring import ( API_LISTENER_LABEL, MLLP, + ConnectionSpec, File, + Ftp, PortConflictError, Registry, + Sftp, WiringError, build_inbound_connection, build_outbound_connection, @@ -538,3 +541,53 @@ def test_streaming_knobs_accepted() -> None: ) assert ic.stream_threshold_bytes == 1024 assert ic.max_message_bytes == 100 * 1024 * 1024 + + +# --- #114: validate_directory is inbound-only --------------------------------- +# +# File()/Sftp()/Ftp() are single factories serving BOTH directions, so they cannot reject the option +# themselves — only the bind knows the direction. Before this guard an outbound carrying +# validate_directory=True was accepted and silently ignored (no destination reads it, and +# DestinationConnector has no validate_startup hook), so an operator asking for fail-fast validation +# got none and saw no error. + + +def _spec_with_validate_directory(kind: str, directory: str) -> ConnectionSpec: + if kind == "file": + return File(directory=directory, validate_directory=True) + if kind == "sftp": + return Sftp(host="sftp.example.org", remote_dir=directory, validate_directory=True) + return Ftp(host="ftp.example.org", remote_dir=directory, validate_directory=True) + + +@pytest.mark.parametrize("kind", ["file", "sftp", "ftp"]) +def test_outbound_validate_directory_is_rejected(kind: str, tmp_path: Path) -> None: + spec = _spec_with_validate_directory(kind, str(tmp_path / "out")) + with pytest.raises(WiringError, match="validate_directory is an inbound-only option") as exc: + build_outbound_connection("OB_X", spec) + assert "'OB_X'" in str(exc.value) # the message names the offending connection + + +@pytest.mark.parametrize("kind", ["file", "sftp", "ftp"]) +def test_inbound_validate_directory_still_builds(kind: str, tmp_path: Path) -> None: + # The guard must not over-reject: the inbound half (ADR 0031 amendment) still honours the option. + spec = _spec_with_validate_directory(kind, str(tmp_path / "in")) + ic = build_inbound_connection("IB_X", spec, router="r") + assert ic.spec.settings["validate_directory"] is True + + +@pytest.mark.parametrize("kind", ["file", "sftp", "ftp"]) +def test_outbound_without_validate_directory_is_unaffected(kind: str, tmp_path: Path) -> None: + # The factories write validate_directory=False into settings unconditionally, so the guard has to + # be truthy-only — every outbound authored today must keep building byte-identically. + directory = str(tmp_path / "out") + spec = ( + File(directory=directory) + if kind == "file" + else Sftp(host="h", remote_dir=directory) + if kind == "sftp" + else Ftp(host="h", remote_dir=directory) + ) + assert spec.settings["validate_directory"] is False # present, but false + oc = build_outbound_connection("OB_X", spec) + assert oc.name == "OB_X"