docs(device): document and characterize per-connection thread accounting

[tylerkron]

Summary

• Closes the last open piece of perf(device): idle connected devices burn CPU — producer poll wakeups, per-device port enumeration every second, a new OS thread per SCPI exchange #491: items 1 and 2 (producer poll wakeups, per-device port enumeration) already shipped in perf(device): an idle connected device no longer burns CPU every second #514, and item 3 (a thread per SCPI exchange) was triaged in the issue and found not worth reworking — tens of microseconds of thread-create cost against a 500ms+ exchange, versus a real memory-leak risk from a park-instead-of-exit rework, and overlap with the read-loop redesign perf(device): every SCPI text exchange pays ~700 ms of fixed polling/teardown overhead — make it event-driven #485 needs.
• What's left of the success criteria is documentation: record the steady-state thread count (2 dedicated threads per connected, idle device — producer + protobuf consumer) and add characterization tests that pin down the per-exchange thread churn so it can't silently regress or silently "improve" without anyone noticing the assumption changed.
• Adds <remarks> to DaqifiDevice's class doc summarizing the thread accounting and linking back to perf(device): idle connected devices burn CPU — producer poll wakeups, per-device port enumeration every second, a new OS thread per SCPI exchange #491/perf(device): every SCPI text exchange pays ~700 ms of fixed polling/teardown overhead — make it event-driven #485.
• Adds DaqifiDeviceThreadAccountingTests: idle-connected devices hold exactly 2 running threads; a text exchange restarts the protobuf consumer on a fresh thread; the device settles back to 2 running threads afterward.

Test plan

• dotnet build
• dotnet test src/Daqifi.Core.Tests/Daqifi.Core.Tests.csproj — full suite green (net9.0 + net10.0)
• New DaqifiDeviceThreadAccountingTests pass individually

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Document and test DAQiFi per-connection thread accounting (#491)

📝 Documentation 🧪 Tests 🕐 20-40 Minutes

AI Description

• Document steady-state and per-exchange thread behavior for connected DAQiFi devices.
• Add characterization tests pinning idle thread counts and SCPI exchange thread churn.
• Prevent silent regressions/changes to thread lifecycle assumptions tracked in #491/#485.

Diagram

graph TD
  A["DaqifiDevice"] --> B["MessageProducer"] --> C(("Producer thread"))
  A --> D["StreamMessageConsumer"] --> E(("Protobuf thread (before)"))
  A --> F["TextExchangeEngine"] --> G(("Text consumer thread"))
  F --> H(("Protobuf thread (after)"))

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Park consumer thread across stop/start (no thread churn)

• ➕ Eliminates per-exchange consumer thread recreation
• ➕ Potentially reduces allocation/creation noise in profilers
• ➖ Requires making the consumer thread long-lived and parkable
• ➖ Increases risk of rooting device/transport references and leaking memory
• ➖ Overlaps with the larger read-loop redesign planned in perf(device): every SCPI text exchange pays ~700 ms of fixed polling/teardown overhead — make it event-driven #485

2. Switch I/O loops to async Tasks / thread-pool instead of dedicated Threads

• ➕ Reduces dedicated thread footprint per connected device
• ➕ May simplify thread accounting by relying on the runtime scheduler
• ➖ Non-trivial rewrite across producer/consumer lifecycles and cancellation behavior
• ➖ Harder to ensure consistent blocking semantics (e.g., Stream.Read) across transports
• ➖ Higher regression risk than warranted for a documented/triaged non-issue

Recommendation: Keep the current design and explicitly document + characterize it (as this PR does). The known thread churn during SCPI/text exchanges is a consciously-accepted tradeoff: the cost is negligible relative to exchange latency, and eliminating it introduces meaningful leak risk and design churn that should be addressed, if at all, as part of the broader #485 read-loop redesign.

Files changed (2) +255 / -0

Tests (1) +232 / -0

DaqifiDeviceThreadAccountingTests.csAdd characterization tests for per-device thread counts and exchange churn +232/-0

Add characterization tests for per-device thread counts and exchange churn

• Introduces tests that assert an idle connected device has exactly two dedicated running threads (producer + protobuf consumer). Adds coverage to verify a text exchange restarts the protobuf consumer on a fresh thread and that the device returns to steady-state afterward. Uses a minimal mock transport/stream and reflection to observe private producer/consumer thread fields.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs

Documentation (1) +23 / -0

DaqifiDevice.csDocument steady-state and SCPI exchange thread behavior in class remarks +23/-0

Document steady-state and SCPI exchange thread behavior in class remarks

• Adds detailed <remarks> to DaqifiDevice describing the two-thread steady-state model (producer + protobuf consumer) and the intentional per-exchange thread creation during SCPI/text exchanges. Captures the rationale and links the decision back to #491 triage and the upcoming #485 redesign work.

src/Daqifi.Core/Device/DaqifiDevice.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Flaky thread-id assertion ✓ Resolved 🐞 Bug ☼ Reliability

Description

ExecuteTextCommand_RestartsTheProtobufConsumerOnAFreshThread asserts the consumer thread changed
by comparing ManagedThreadId, but the old thread is stopped before the new one is created so the
runtime can reuse thread IDs, making this intermittently fail despite correct behavior. Compare the
Thread object instances (and optionally assert the old thread exited) instead of comparing IDs.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R70-71]

+        var afterId = GetConsumerThread(afterConsumer)!.ManagedThreadId;
+        Assert.NotEqual(beforeId, afterId);

Relevance

●●● Strong

Recent thread-lifecycle precedents favor asserting actual thread behavior and eliminating
timing-sensitive concurrency assumptions.

PR-#384
PR-#509

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test compares ManagedThreadId before/after, but the consumer restart path creates a fresh
Thread object; using object identity is the stable observable. The repo’s other thread-ID-based
tests count distinct IDs while threads are concurrently alive (blocked in Read), avoiding sequential
ID reuse hazards.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[58-72]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[195-211]
src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerStallingReaderTests.cs[426-471]
PR-#196

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DaqifiDeviceThreadAccountingTests.ExecuteTextCommand_RestartsTheProtobufConsumerOnAFreshThread` uses `ManagedThreadId` to prove the protobuf consumer restarted on a fresh thread. Because the old consumer thread is stopped before the restart, the runtime may legally reuse the old thread’s ID for the new thread, making the test flaky.

### Issue Context
This repo already uses thread IDs only in scenarios where multiple threads are alive concurrently (no reuse possible), but this new test is a sequential stop/start.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[58-72]
- src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[195-211]
- src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerStallingReaderTests.cs[426-471]

### Suggested change
- Capture the `Thread` instance before the exchange (`var beforeThread = GetConsumerThread(consumer!)!;`).
- After the exchange, capture the new thread (`var afterThread = GetConsumerThread(afterConsumer!)!;`).
- Assert `Assert.NotSame(beforeThread, afterThread)`.
- Optionally assert the old thread exits (bounded): `Assert.True(SpinWait.SpinUntil(() => !beforeThread.IsAlive, TimeSpan.FromSeconds(1)))` so the characterization also pins down clean shutdown.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Doesn't assert “exactly two” ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new tests and method names claim “exactly two running threads” and “no leaked text-consumer
thread”, but they only assert that the producer and protobuf consumer threads are alive, not that no
additional thread remains running. This can let regressions (e.g., leaked text consumer thread) slip
through while the characterization test still passes.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R21-23]

+    [Fact]
+    public void Connect_IdleDevice_HasExactlyTwoRunningDedicatedThreads()
+    {

Relevance

●●● Strong

Recent tests were accepted for strengthening assertions so regressions cannot pass through
incomplete coverage.

PR-#509
PR-#384

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests’ names/docs promise exclusivity but the assertions only validate that two specific threads
are alive. Meanwhile, the production text-exchange path explicitly creates a temporary
StreamMessageConsumer<string> (and thus an additional reader thread) whose lifecycle is exactly
what the tests claim to characterize, but they never observe it.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[11-18]
src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[21-47]
src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[76-96]
src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[384-446]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The tests are presented as characterization of steady-state thread counts (“exactly two threads” and “no leaked text consumer”), but they never actually assert those exclusivity properties. They only check that the known threads exist and are alive.

### Issue Context
`TextExchangeEngine` creates a separate temporary `StreamMessageConsumer<string>` (with its own reader thread) for text exchanges; the characterization should prove that this thread is transient and not left running.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[21-47]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[76-96]
- src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[384-446]

### Suggested change
Update the test harness to make the transient thread observable and assert it terminates, e.g.:
- Enhance `ImmediateReplyStream.Read(...)` to record `Thread.CurrentThread` instances that enter `Read` (store `Thread` object references, not IDs) and expose them via the transport (or via an internal getter).
- In `ExecuteTextCommand_LeavesTheDeviceBackAtTwoRunningThreadsAfterward`, assert that any thread that entered `Read` as part of the text consumer is no longer alive after the exchange completes (bounded wait).
- Alternatively, adjust test names/docs to match what is actually asserted if you decide not to validate exclusivity.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Misleading read-thread comment ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

The comment claims the transport’s Read-thread tracking includes the producer thread, but the
producer writes to the stream and does not call Stream.Read, so the test is only tracking
consumer-side read threads. This overstates the coverage of the “no leaked threads” assertion and
can mislead future maintainers about what regressions this test would actually catch.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R86-89]

+        // steady state — no leaked text-consumer thread left running behind it. Proven by
+        // tracking every thread that ever entered the transport's Read (producer/consumer plus
+        // whatever the exchange spun up) and requiring that only the current producer and
+        // consumer threads are still alive afterward.

Relevance

●●● Strong

Recent precedents show team accepts fixing misleading/inaccurate test comments to reflect actual
coverage.

PR-#456
PR-#509

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test comment says it tracks producer/consumer threads via transport.Read, but the producer
implementation only writes to the stream (Write/Flush) and has no stream reads, so it cannot appear
in the Read-observed thread list.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[85-89]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[341-346]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
A comment in `ExecuteTextCommand_LeavesTheDeviceBackAtTwoRunningThreadsAfterward` says the test tracks “producer/consumer” threads via the transport’s `Read` method, but the producer never calls `Read` (it writes to the stream). This misdocuments what the test is verifying.

### Issue Context
The test’s `ObservedReaderThreads` instrumentation only observes `Stream.Read` callers, which will include protobuf/text consumers but not the producer write loop.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[85-90]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Async Wait in mock ✓ Resolved 🐞 Bug ☼ Reliability

Description

ImmediateReplyMockTransport.Connect()/Disconnect() call .Wait() on async methods,
reintroducing the sync-over-async deadlock risk the transport layer explicitly documents as
something to avoid. Implement these sync methods directly or use GetAwaiter().GetResult() if you
must bridge async to sync in tests.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R183-185]

+        public void Connect() => ConnectAsync().Wait();
+
+        public void Disconnect() => DisconnectAsync().Wait();

Relevance

●● Moderate

Sync-over-async is a valid reliability concern, but no close precedent supports rejecting this
trivial completed-task test bridge.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new mock transport explicitly blocks on async tasks with .Wait(). The transport interface
documentation highlights synchronization-context concerns and that thread choice is arbitrary in
status events, which is exactly where .Wait() can cause deadlocks in sync contexts.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[164-186]
src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[33-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test transport’s synchronous wrappers use `.Wait()`, which can deadlock when a `SynchronizationContext` is present.

### Issue Context
`IStreamTransport` remarks call out avoiding UI-thread deadlocks by not resuming on captured sync contexts; tests shouldn’t reintroduce sync-over-async blocking patterns.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[183-186]
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[33-40]

### Suggested change
- Make `Connect()`/`Disconnect()` purely synchronous state flips (since this mock has no real I/O), and keep the async methods delegating to them; OR
- Replace `.Wait()` with `.GetAwaiter().GetResult()` and ensure the async methods never capture a context (still less ideal than a direct sync implementation for a mock).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 23d6f86

Results up to commit 170a7c0 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Flaky thread-id assertion ✓ Resolved 🐞 Bug ☼ Reliability

Description

ExecuteTextCommand_RestartsTheProtobufConsumerOnAFreshThread asserts the consumer thread changed
by comparing ManagedThreadId, but the old thread is stopped before the new one is created so the
runtime can reuse thread IDs, making this intermittently fail despite correct behavior. Compare the
Thread object instances (and optionally assert the old thread exited) instead of comparing IDs.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R70-71]

+        var afterId = GetConsumerThread(afterConsumer)!.ManagedThreadId;
+        Assert.NotEqual(beforeId, afterId);

Relevance

●●● Strong

Recent thread-lifecycle precedents favor asserting actual thread behavior and eliminating
timing-sensitive concurrency assumptions.

PR-#384
PR-#509

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new test compares ManagedThreadId before/after, but the consumer restart path creates a fresh
Thread object; using object identity is the stable observable. The repo’s other thread-ID-based
tests count distinct IDs while threads are concurrently alive (blocked in Read), avoiding sequential
ID reuse hazards.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[58-72]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[195-211]
src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerStallingReaderTests.cs[426-471]
PR-#196

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DaqifiDeviceThreadAccountingTests.ExecuteTextCommand_RestartsTheProtobufConsumerOnAFreshThread` uses `ManagedThreadId` to prove the protobuf consumer restarted on a fresh thread. Because the old consumer thread is stopped before the restart, the runtime may legally reuse the old thread’s ID for the new thread, making the test flaky.

### Issue Context
This repo already uses thread IDs only in scenarios where multiple threads are alive concurrently (no reuse possible), but this new test is a sequential stop/start.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[58-72]
- src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[195-211]
- src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerStallingReaderTests.cs[426-471]

### Suggested change
- Capture the `Thread` instance before the exchange (`var beforeThread = GetConsumerThread(consumer!)!;`).
- After the exchange, capture the new thread (`var afterThread = GetConsumerThread(afterConsumer!)!;`).
- Assert `Assert.NotSame(beforeThread, afterThread)`.
- Optionally assert the old thread exits (bounded): `Assert.True(SpinWait.SpinUntil(() => !beforeThread.IsAlive, TimeSpan.FromSeconds(1)))` so the characterization also pins down clean shutdown.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Doesn't assert “exactly two” ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new tests and method names claim “exactly two running threads” and “no leaked text-consumer
thread”, but they only assert that the producer and protobuf consumer threads are alive, not that no
additional thread remains running. This can let regressions (e.g., leaked text consumer thread) slip
through while the characterization test still passes.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R21-23]

+    [Fact]
+    public void Connect_IdleDevice_HasExactlyTwoRunningDedicatedThreads()
+    {

Relevance

●●● Strong

Recent tests were accepted for strengthening assertions so regressions cannot pass through
incomplete coverage.

PR-#509
PR-#384

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests’ names/docs promise exclusivity but the assertions only validate that two specific threads
are alive. Meanwhile, the production text-exchange path explicitly creates a temporary
StreamMessageConsumer<string> (and thus an additional reader thread) whose lifecycle is exactly
what the tests claim to characterize, but they never observe it.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[11-18]
src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[21-47]
src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[76-96]
src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[384-446]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The tests are presented as characterization of steady-state thread counts (“exactly two threads” and “no leaked text consumer”), but they never actually assert those exclusivity properties. They only check that the known threads exist and are alive.

### Issue Context
`TextExchangeEngine` creates a separate temporary `StreamMessageConsumer<string>` (with its own reader thread) for text exchanges; the characterization should prove that this thread is transient and not left running.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[21-47]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[76-96]
- src/Daqifi.Core/Device/Internal/TextExchangeEngine.cs[384-446]

### Suggested change
Update the test harness to make the transient thread observable and assert it terminates, e.g.:
- Enhance `ImmediateReplyStream.Read(...)` to record `Thread.CurrentThread` instances that enter `Read` (store `Thread` object references, not IDs) and expose them via the transport (or via an internal getter).
- In `ExecuteTextCommand_LeavesTheDeviceBackAtTwoRunningThreadsAfterward`, assert that any thread that entered `Read` as part of the text consumer is no longer alive after the exchange completes (bounded wait).
- Alternatively, adjust test names/docs to match what is actually asserted if you decide not to validate exclusivity.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Async Wait in mock ✓ Resolved 🐞 Bug ☼ Reliability

Description

ImmediateReplyMockTransport.Connect()/Disconnect() call .Wait() on async methods,
reintroducing the sync-over-async deadlock risk the transport layer explicitly documents as
something to avoid. Implement these sync methods directly or use GetAwaiter().GetResult() if you
must bridge async to sync in tests.

Code

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[R183-185]

+        public void Connect() => ConnectAsync().Wait();
+
+        public void Disconnect() => DisconnectAsync().Wait();

Relevance

●● Moderate

Sync-over-async is a valid reliability concern, but no close precedent supports rejecting this
trivial completed-task test bridge.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new mock transport explicitly blocks on async tasks with .Wait(). The transport interface
documentation highlights synchronization-context concerns and that thread choice is arbitrary in
status events, which is exactly where .Wait() can cause deadlocks in sync contexts.

src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[164-186]
src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[33-40]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test transport’s synchronous wrappers use `.Wait()`, which can deadlock when a `SynchronizationContext` is present.

### Issue Context
`IStreamTransport` remarks call out avoiding UI-thread deadlocks by not resuming on captured sync contexts; tests shouldn’t reintroduce sync-over-async blocking patterns.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiDeviceThreadAccountingTests.cs[183-186]
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[33-40]

### Suggested change
- Make `Connect()`/`Disconnect()` purely synchronous state flips (since this mock has no real I/O), and keep the async methods delegating to them; OR
- Replace `.Wait()` with `.GetAwaiter().GetResult()` and ensure the async methods never capture a context (still less ideal than a direct sync implementation for a mock).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 78589aa

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 23d6f86

[tylerkron]

Second agentic_review pass is clean: Bugs (0), Rule violations (0), Skill insights (0), and all three prior findings (flaky thread-id comparison, unproven "exactly two threads" claim, misleading comment) are marked resolved. CI is green. PR is ready for review.