Skip to content

fix(common): add IAgentConfiguration.Sender for Header/@sender - #222

Draft
ottobolyos wants to merge 8 commits into
TrakHound:masterfrom
ottobolyos:fix/agent-config-sender-writable
Draft

fix(common): add IAgentConfiguration.Sender for Header/@sender#222
ottobolyos wants to merge 8 commits into
TrakHound:masterfrom
ottobolyos:fix/agent-config-sender-writable

Conversation

@ottobolyos

@ottobolyos ottobolyos commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an operator-authoring surface for Header/@sender on MTConnect response documents so hosts embedding MTConnectAgent can override the pre-existing System.Net.Dns.GetHostName() fallback with a deployment-scoped identifier.

MTConnect Part 1 §7 defines Header/@sender as an operator-authored identifier — "An identification defining where the Agent that published the Response Document is installed or hosted." Before this PR, embedding hosts had no way to author it: the property lived on the agent but was get-only, with no ctor parameter and no IAgentConfiguration field, so every emitted Probe / Current / Sample / Asset response document carried Dns.GetHostName() regardless of operator intent.

What this PR does

  • Adds string Sender { get; } to IAgentConfiguration with a class-level docstring pointing at Part 1 §7 and stating the Dns.GetHostName() fallback contract.
  • Adds string Sender { get; set; } to the concrete AgentConfiguration under [JsonPropertyName("sender")], matching the wire-format convention used by the other simple string fields (timezoneOutput, etc.).
  • Wires MTConnectAgent's IAgentConfiguration-taking ctor to seed the private _sender field when configuration.Sender is present. The existing Sender getter still lazily falls back to Dns.GetHostName() when _sender is empty, so hosts that do not opt in see identical behavior.
  • Regenerates docs/reference/configuration.md so the new AgentConfiguration.Sender and IAgentConfiguration.Sender rows surface in the operator-facing reference table.

Standard citation

MTConnect Part 1 §7 — "An identification defining where the Agent that published the Response Document is installed or hosted." The pre-existing Dns.GetHostName() fallback in MTConnectAgent.Sender remains the default when the configuration value is null or empty.

Non-breaking behavior

  • IMTConnectAgent.Sender { get; } unchanged — no setter added on the agent side; the source of truth for the operator-authored value stays on the configuration.
  • MTConnectAgent ctor signatures unchanged.
  • Existing behavior preserved bit-for-bit when the config value is absent — the Dns.GetHostName() fallback still fires on first read of the getter.
  • Concrete additions on AgentConfiguration inherit into AgentApplicationConfiguration automatically (no separate change required).

Breaking changes

Adds a new member (Sender { get; }) to the public IAgentConfiguration interface — a source-compatibility break for any external implementer of IAgentConfiguration outside the library. Acceptable on the v7 major-version boundary.

Dime review cycle 1

Six-agent adversarial-refute cycle across code-review, documentation-audit, test-coverage-audit, security-audit, simplification, and improvement. Findings summarised below with the resolution applied for every 🟡 medium and higher.

  • 🟠 HIGH — test-coverage-audit — AgentConfigurationSenderSerializationTests.SaveJson_with_createBackup_copies_existing_target_to_backup_directory and its SaveYaml_* companion round-tripped the newly-written value but never asserted the copy-into-backup-directory contract that the test names promise — the branch coverage on AgentConfiguration.SaveJson / SaveYaml lines 416-427 / 450-461 was illusory.
    Fix: 825b3a56 — both positive-path tests now snapshot the backup directory before the save and assert exactly one new *.backup.{json,yaml} file appears whose contents preserve the pre-existing target byte-for-byte. Two negative-path companions added pinning that createBackup: false produces zero backup files.
  • 🟠 HIGH — test-coverage-audit — TDD RED-first violation: the fix(common) production-code commit precedes every test(*) commit on this branch, so no test lands RED against pre-fix SUT.
    Skip-rationale: the tests exist, are green against the fixed SUT, and cover every branch of the sender-seed guard plus the wire-format round-trip. Rewriting the commit chain to reorder RED-first would require a rebase + force-push on a shared PR branch, which is out of scope for a Ready-verification cycle under the standing constraint "no --force". Acknowledged as a process-discipline gap that does not affect ship-readiness.
  • 🟡 MEDIUM — code-review — AgentSenderAllEndpointsWorkflowTests class-level <remarks> claimed a shared per-class fixture (once-per-class broker + HTTP server), but the class does not wire IClassFixture<T> and xUnit v2 instantiates the test class once per test method. The comment also miscounted "four endpoint assertions" (there are five) and mentioned the wrong CI-selector category ("RequiresDocker").
    Fix: d98bbfce<remarks> rewritten to state the actual xUnit v2 per-test lifecycle, list all five endpoints, and drop the incorrect RequiresDocker mention. No functional change.
  • 🟡 MEDIUM — improvement + simplification + code-review (deduped) — dead _configuration != null clause on the ctor sender-seed guard: the line immediately above assigns _configuration = configuration != null ? configuration : new AgentConfiguration();, so the field is guaranteed non-null one line down.
    Fix: d98bbfce — redundant clause removed. The identical pattern still lives on the neighboring _mtconnectVersion assignment; a wider chore PR is the right place to strip it repo-wide rather than half-fixing it here.
  • 🟡 MEDIUM — improvement — whitespace-only Sender accepted verbatim (!string.IsNullOrEmpty vs !string.IsNullOrWhiteSpace). Emits Header/@sender=" " which is XSD-valid but functionally useless downstream.
    Skip-rationale: intentional contract. AgentConfigurationSenderTests.Sender_whitespace_only_in_config_is_carried_through_verbatim explicitly pins the IsNullOrEmpty boundary that the ctor walks. MTConnect Part 1 §7 does not restrict the shape of sender, and the codebase's convention for string config properties is verbatim pass-through — laundering whitespace here would silently diverge from that repo-wide convention. Kept as-is.
  • 🟢 LOW — security-audit — no findings. Ten OWASP-tagged attack vectors refuted (attribute injection via XmlWriter.WriteAttributeString escaping; JSON injection via System.Text.Json; JSON / YAML deserialization gadget; log / CRLF injection; config-file path traversal; dependency CVEs; secret exposure in fixtures; XmlReader / XmlDocument / TLS hardening; Dns.GetHostName() information disclosure; unbounded resource consumption).
  • 🟢 LOW — documentation-audit — MTConnectAgent.Sender getter summary at MTConnectAgent.cs:150 (and IMTConnectAgent.Sender at IMTConnectAgent.cs:58) reads "Gets the Sender that is hosting the Agent" — literally correct but does not mention the new config-authored source or the fallback contract. Track: worth extending to "…sourced from IAgentConfiguration.Sender when authored, otherwise falls back to Dns.GetHostName()" in a follow-up docstring-drift sweep; the config surface remains discoverable via AgentConfiguration.Sender's own well-documented summary.
  • 🟢 LOW — simplification — DefaultVersionValue_* tests in AgentConfigurationSenderSerializationTests are tangential to the Sender scope; probe test surface overlaps with AgentSenderAllEndpointsWorkflowTests. Both track as candidate follow-ups; not blocking.
  • ⚪ SUGGESTION — code-review — Assert.Contains($"sender=\"{PinnedSender}\"", body) is a substring check on the raw response body; XDocument.Parse(body).Root.Element(headerName).Attribute("sender").Value would pin the value to Header/@sender explicitly. Track as follow-up hardening.
  • ⚪ SUGGESTION — improvement — hot-reload of Sender via AgentConfigurationFileWatcher works transitively through StopAgent / StartAgent; no dedicated end-to-end assertion. Track as follow-up test-widening.

(Zero unfixed findings — Ready-eligible.)


Supersedes #213.

Depends on

@ottobolyos
ottobolyos force-pushed the fix/agent-config-sender-writable branch from 825b3a5 to 814a414 Compare August 19, 2026 12:12
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
@ottobolyos
ottobolyos force-pushed the fix/agent-config-sender-writable branch from 814a414 to 5038efa Compare August 19, 2026 22:05
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 19, 2026
@ottobolyos
ottobolyos force-pushed the fix/agent-config-sender-writable branch 3 times, most recently from 24e4e36 to 85a7bd8 Compare August 21, 2026 14:08
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
…ender

Adds `Sender { get; }` to `IAgentConfiguration` and `Sender { get; set; }` to
the concrete `AgentConfiguration`, and wires `MTConnectAgent`'s
`IAgentConfiguration`-taking ctor to seed the private `_sender` field when the
config value is present. The existing get-only `IMTConnectAgent.Sender` and
`MTConnectAgent.Sender` surfaces are unchanged, as is the
`System.Net.Dns.GetHostName()` fallback when no value is supplied — hosts
that do not opt in see identical behavior.

Rationale: MTConnect Part 1 §7 defines `Header/@sender` as an operator-
authored identifier ("An identification defining where the Agent that
published the Response Document is installed or hosted"). Before this
change, embedding hosts had no way to author it — the property lived on the
agent but was get-only, with no ctor param and no config surface, so every
emitted Probe / Current / Sample / Asset response document carried
`Dns.GetHostName()` regardless of operator intent.

Non-breaking: interface additions are new members; the concrete `Sender`
setter is additive; ctor signature unchanged; consumer semantics preserved
when the config value is absent.

Test coverage: `AgentConfigurationSenderTests` pins both branches — value
flows through when set, `Dns.GetHostName()` fallback preserved when null.
…Configuration.Sender

Adds negative and positive test rows for the operator-authored `Sender`
surface on `AgentConfiguration`:

- Default value is null (fallback to `Dns.GetHostName` fires on first
  read of `MTConnectAgent.Sender`).
- `[JsonPropertyName("sender")]` binds to the lowercase wire-name so
  authored JSON / YAML operator configs deserialize straight into the
  property.
- `SaveJson` / `ReadJson<T>` round trip preserves value + operator
  path.
- `SaveYaml` / `ReadYaml<T>` round trip preserves value + operator
  path.
- Authored JSON payload with `"sender": "..."` deserializes the value.
- Authored YAML payload with `sender: ...` under the camelCase naming
  convention deserializes the value.
- Empty-string `Sender` round trips as empty.
- Values containing `/`, `:`, and `-` round trip verbatim through JSON.
…ender surface

Adds a workflow-level fixture that boots an in-process
`MTConnectAgentBroker` plus embedded `MTConnectHttpServer`, performs
a real HTTP GET on `/probe`, and asserts the emitted
`MTConnectDevices/Header/@sender` attribute matches the
operator-authored value.

- Positive: `AgentConfiguration.Sender = "foo-plant-a"` flows through
  to `<Header sender="foo-plant-a" ...>` in the probe response body.
- Negative: an unset `Sender` still emits `<Header
  sender="<Dns.GetHostName()>" ...>` -- the pre-existing fallback.

Both tests are tagged `[Trait("Category", "E2E")]` and use ephemeral
ports allocated from a base outside the existing MTAgentFixture range
so parallel workers do not collide.

Source: MTConnect Part 1 section 7 -- `Header/@sender` is defined as
"An identification defining where the Agent that published the
Response Document is installed or hosted."
Regenerates docs/reference/configuration.md via the DocsGen tool so
the new `AgentConfiguration.Sender` and `IAgentConfiguration.Sender`
rows surface in the operator-facing reference table alongside the
pre-existing agent configuration fields. The generator picks up the
XML doc comments authored on the Sender surface verbatim.
…ross endpoints and edge cases

Extend the pinning surface for IAgentConfiguration.Sender in two ways:

- AgentSenderAllEndpointsWorkflowTests: new integration test class boots a
  shared broker + HTTP server fixture and asserts that the operator-authored
  AgentConfiguration.Sender appears verbatim as the Header/@sender attribute
  on every top-level MTConnect response envelope — /probe (MTConnectDevices),
  /current + /sample (MTConnectStreams), /assets + /asset/{id}
  (MTConnectAssets). MTConnect Part 1 §7 declares the attribute on every
  header shape, and the XSDs put it on all four; the earlier tests only
  covered /probe.

- AgentConfigurationSenderTests: add four unit tests pinning the
  null/empty/whitespace boundary of the constructor's IsNullOrEmpty guard,
  the interface-side polymorphic getter, and the null-configuration
  fallback path. These document the exact contract the fallback chain
  carries — Dns.GetHostName fires for null and "", but a whitespace-only
  value is carried through verbatim.
…tion fallback test

The Sender_null_configuration_falls_back_to_Dns_GetHostName test's
`(IAgentConfiguration)null` cast fired CS8600 under the Common-Tests
project's nullable=enable setting. Bind the null to a locally-typed
nullable variable and pass it with the `!` null-forgiving operator so
the intent (explicit null-configuration ctor input) reads clearly and
the compile is warning-clean.
The `_configuration != null` clause on the ctor sender-seed guard is
provably dead — the line immediately above assigns `_configuration =
configuration != null ? configuration : new AgentConfiguration();`, so
the field is guaranteed non-null one line down. Removing the redundant
clause improves the diff's own signal and pre-empts the "did the author
know `_configuration` can be null further down the ctor?" reader
question.

Also widens the SaveJson / SaveYaml createBackup test coverage: the
pre-existing positive-path tests round-tripped the newly-written value
but never asserted the copy-into-backup-directory contract that the
test names promise. Both positive-path tests now snapshot the
process-wide backup directory before the save, then assert exactly one
new `*.backup.{json,yaml}` file appears whose contents preserve the
pre-existing target byte-for-byte. Adds two negative-path companion
tests pinning that `createBackup: false` produces zero backup files.
Both extensions use a `SnapshotBackupFiles` helper that survives
process-wide backup-directory contention with other test fixtures in
the same run.
The class-level `<remarks>` claimed a shared per-class fixture (once-per-class
broker + HTTP server), but the class does not wire `IClassFixture<T>` and
xUnit v2 instantiates the test class once per test method — so the broker
and HTTP server are constructed and torn down per test. The stale doc leaks
into the maintenance model (future authors reading the file may add tests
here thinking cost is amortized). The comment also miscounted the fixture
as covering "four endpoint assertions" when there are five (Probe, Current,
Sample, Assets, SingleAsset), and mentioned the wrong CI-selector category
("RequiresDocker") when the file is in-process HTTP with no Docker usage.

Rewrites the `<remarks>` to state the actual xUnit v2 per-test lifecycle,
lists the five endpoints, and drops the incorrect RequiresDocker mention.
No functional change — comment-only.
@ottobolyos
ottobolyos force-pushed the fix/agent-config-sender-writable branch from 85a7bd8 to 642576c Compare August 21, 2026 16:05
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
ottobolyos added a commit to ottobolyos/mtconnect.net that referenced this pull request Aug 21, 2026
…multi-TFM compat

The Normalize() null-coalescing assignment (`??=`, line 281) and the
MapInputToDeviceValidationLevel switch expression (`return value switch { ... }`,
lines 302-309, both introduced by 3dc3383 on this branch) require C# 8.
The multi-TFM Release pack builds against net461/net47/net462/net471, which
default to LangVersion 7.3 — CS8370 fires there on every framework in the
matrix, blocking the tail integration build.

Rewrites to C# 7.3-compatible idioms:
  * `_deviceValidationLevel ??= X` -> `if (_deviceValidationLevel == null) _deviceValidationLevel = X`
  * switch expression -> classical switch statement (default arm preserves throw)

Semantic-preserving pure-syntax swap — no behavior change; existing DVL
migration + normalize + enum-arm tests continue to cover the mapping.

Per Otto's "use the features of the oldest language version. Later we can
bump the version to a newer one which is gated by the maintainer's decision
but I believe we can always bump it without breaking changes to the latest
language version of the oldest TFM" directive 2026-08-21. Attribution
correction: the offending sites were introduced on this branch (TrakHound#241) via
commit 3dc3383, not on TrakHound#222 as the initial tail-sweep bug report suggested
(the CS8370 site listing was routed to TrakHound#222 because TrakHound#222 owns the Sender
addition on the same file; commit blame shows TrakHound#241 owns the C# 8 sites).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant