feat(opcua): capture node DataType during browse and metadata reads

[cartercanedy]

Summary

• OpcUaNode gains data_type: Option<OpcUaNodeId> (#[serde(default, skip_serializing_if = "Option::is_none")], so existing serialized trees still deserialize and non-variables don't emit the key).
• OpcUaNode::data_type_name() -> Option<Cow<'static, str>> maps ns=0 builtin type ids (i=1..=29: Boolean, Int32, Double, String, …) to their names and falls back to the canonical node id string for custom types.
• Browse for OpcUaClient::browse_node now issues one batched read_many_attributes(DATATYPE) for the Variable children of each browsed node (one extra request per browse level, zero when a level has no variables). A failed read is logged and leaves data_type = None rather than failing the browse.

Type of change

• New feature (feat)

Verification

cargo check/clippy -p instro-opcua --all-targets --all-features, cargo +nightly fmt --check, cargo test -p instro-opcua: 73 unit + 6 client_harness tests pass. Not exercised against a real server beyond the harness.

Tests

• Unit tests added or updated — serde roundtrip includes data_type; new data_type_name_maps_builtin_and_falls_back_to_node_id.

Checklist

• PR title follows Conventional Commits
• I have read CONTRIBUTING.md
• Documentation updated if user-facing behavior changed (crate-internal API; no user docs affected)
• Code follows the style/conventions of the surrounding code

Notes for reviewers

Adding a public struct field is technically a breaking change for downstream struct literals

[claude]

Claude finished @cartercanedy's task in 2m 30s —— View job

---

Review: capture Variable node DataType during browse

• Gather context (PR body, diff, files)
• Run /code-review --comment
• Post findings

Overview

Clean, well-scoped change. Batching the DATATYPE read per browse level (one extra read_many_attributes call, skipped when a level has no variables) fits the existing recursive-browse shape and avoids a per-node round trip. data_type_name()'s builtin-id table + node-id fallback is a reasonable, self-contained mapping — no shared/base-class creep. #[serde(default, skip_serializing_if = "Option::is_none")] correctly preserves backward-compat deserialization of existing serialized trees.

One thing worth a decision before merge: adding data_type to OpcUaNode (which has no Default impl) breaks any downstream struct-literal construction — the PR description already flags this. Per this repo's Conventional Commits convention, that should probably ship as feat! rather than feat, unless the companion connect-side PR lands atomically with this one.

Two inline notes posted on the diff:

• browse.rs: the variables.zip(values) in fill_variable_data_types silently drops entries on a length mismatch, unlike the sibling read_nodes path which bails on the same condition.
• tests/client_harness.rs: the harness test that browses real variables (exercising the new batched read) doesn't assert data_type on the results, so the mapping logic runs but isn't verified.

No missing-driver-convention, docstring, or docs-sync issues — this is a crate-internal Rust struct, not exposed via PyO3.

[greptile-apps]

Confidence Score: 4/5

The implementation should not merge under a regular feature release because the new public field breaks downstream struct literals; the missing integration assertion is also worth addressing.

Findings

1.  Public field breaks consumers ▶
2.  Enrichment result remains untested ▶

Prompt To Fix All With AI

### Issue 1
crates/instro-opcua/src/types.rs:857-859
Adding `data_type` to the public `OpcUaNode` struct breaks downstream code that constructs this published 1.x type with struct literals: those consumers will no longer compile until they add the field. Because this is recorded as a regular feature, the next release can present the change as semver-compatible. Mark and release it as a breaking change, or provide a forward-compatible construction path.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 2
crates/instro-opcua/tests/client_harness.rs:66
The client harness now reaches the new DataType read while browsing Variable nodes, but it never asserts that `data_type` is populated. An incorrect attribute request or response decoder that leaves every value as `None` would therefore still pass the integration suite. Please assert the expected built-in DataType IDs for the harness variables.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

• Performs one batched DataType attribute read for Variable children at each browse level.
• Preserves browse success when DataType reads fail.
• Adds optional serialized DataType metadata and built-in-name formatting.
• Introduces a source-breaking public-struct change that needs an appropriate release signal.
• The successful browse-enrichment path lacks an integration assertion.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Browse node references] --> B[Build OpcUaNode entries]
  B --> C{Variable children?}
  C -- No --> F[Return browse results]
  C -- Yes --> D[Batch-read DataType attributes]
  D --> E[Decode NodeId into data_type]
  D -- Read failure --> G[Log warning and retain None]
  E --> F
  G --> F

Loading

[greptile-apps]

Public field breaks consumers

Adding data_type to the public OpcUaNode struct breaks downstream code that constructs this published 1.x type with struct literals: those consumers will no longer compile until they add the field. Because this is recorded as a regular feature, the next release can present the change as semver-compatible. Mark and release it as a breaking change, or provide a forward-compatible construction path.

Prompt To Fix With AI

This is a comment left during a code review.
Path: crates/instro-opcua/src/types.rs
Line: 857-859

Comment:
**Public field breaks consumers**

Adding `data_type` to the public `OpcUaNode` struct breaks downstream code that constructs this published 1.x type with struct literals: those consumers will no longer compile until they add the field. Because this is recorded as a regular feature, the next release can present the change as semver-compatible. Mark and release it as a breaking change, or provide a forward-compatible construction path.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[devin-ai-integration]

Agreed it's source-breaking for struct-literal consumers (connect's are updated in nominal-io/connect#3637, which pins this commit). Leaving the feat vs feat! call to @cartercanedy — happy to retitle if you want it released as a major.

[greptile-apps]

Thanks for confirming. The companion PR addresses the known consumer, but the public-field addition remains source-breaking for other downstream struct-literal users. I recommend treating this as feat! and releasing it as a major version if this crate follows semver; otherwise, make the breaking change explicit in the release notes. No further code change is needed for this comment.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.